Don't close pack fd when free'ing pack windows

Now that close_one_pack() has been introduced to handle file
descriptor pressure, it is not strictly necessary to close the
pack file descriptor in unuse_one_window() when we're under memory
pressure.

Jeff King provided a justification for leaving the pack file open:

If you close packfile descriptors, you can run into racy situations
where somebody else is repacking and deleting packs, and they go away
while you are trying to access them. If you keep a descriptor open,
you're fine; they last to the end of the process. If you don't, then
they disappear from under you.

For normal object access, this isn't that big a deal; we just rescan
the packs and retry. But if you are packing yourself (e.g., because
you are a pack-objects started by upload-pack for a clone or fetch),
it's much harder to recover (and we print some warnings).

Let's do so (or uh, not do so).

Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462cb03bbae46dae8d6b97420b3726690c47..4eb0521c815870abb670e8ddb035caa9c6507e67 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1809,7 +1809,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 static void try_to_free_from_threads(size_t size)
 {
        read_lock();
 static void try_to_free_from_threads(size_t size)
 {
        read_lock();

-       release_pack_memory(size, -1);
+       release_pack_memory(size);

        read_unlock();
 }
 
        read_unlock();
 }
 

diff --git a/git-compat-util.h b/git-compat-util.h
index ad4762499ef66336ed8a7ac17cf0a6cb36da5d15..9a564841060be0e8789e6ed8e4a7bdeaa78fff00 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -498,7 +498,7 @@ int inet_pton(int af, const char *src, void *dst);
 const char *inet_ntop(int af, const void *src, char *dst, size_t size);
 #endif
 
 const char *inet_ntop(int af, const void *src, char *dst, size_t size);
 #endif
 

-extern void release_pack_memory(size_t, int);
+extern void release_pack_memory(size_t);

 
 typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);
 
 typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);

diff --git a/sha1_file.c b/sha1_file.c
index 263cf71c27fa81f60da5a8688d899b66b1967d23..cc7278dd3a659e5c85364fd3fd6adcba598aa9f8 100644 (file)
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -605,7 +605,7 @@ static void scan_windows(struct packed_git *p,
        }
 }
 
        }
 }
 

-static int unuse_one_window(struct packed_git *current, int keep_fd)
+static int unuse_one_window(struct packed_git *current)

 {
        struct packed_git *p, *lru_p = NULL;
        struct pack_window *lru_w = NULL, *lru_l = NULL;
 {
        struct packed_git *p, *lru_p = NULL;
        struct pack_window *lru_w = NULL, *lru_l = NULL;


@@ -619,15 +619,8 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
                pack_mapped -= lru_w->len;
                if (lru_l)
                        lru_l->next = lru_w->next;
                pack_mapped -= lru_w->len;
                if (lru_l)
                        lru_l->next = lru_w->next;

-               else {
+               else

                        lru_p->windows = lru_w->next;
                        lru_p->windows = lru_w->next;

-                       if (!lru_p->windows && lru_p->pack_fd != -1
-                               && lru_p->pack_fd != keep_fd) {
-                               close(lru_p->pack_fd);
-                               pack_open_fds--;
-                               lru_p->pack_fd = -1;
-                       }
-               }

                free(lru_w);
                pack_open_windows--;
                return 1;
                free(lru_w);
                pack_open_windows--;
                return 1;


@@ -635,10 +628,10 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
        return 0;
 }
 
        return 0;
 }
 

-void release_pack_memory(size_t need, int fd)
+void release_pack_memory(size_t need)

 {
        size_t cur = pack_mapped;
 {
        size_t cur = pack_mapped;

-       while (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))
+       while (need >= (cur - pack_mapped) && unuse_one_window(NULL))

                ; /* nothing */
 }
 
                ; /* nothing */
 }
 


@@ -649,7 +642,7 @@ void *xmmap(void *start, size_t length,
        if (ret == MAP_FAILED) {
                if (!length)
                        return NULL;
        if (ret == MAP_FAILED) {
                if (!length)
                        return NULL;

-               release_pack_memory(length, fd);
+               release_pack_memory(length);

                ret = mmap(start, length, prot, flags, fd, offset);
                if (ret == MAP_FAILED)
                        die_errno("Out of memory? mmap failed");
                ret = mmap(start, length, prot, flags, fd, offset);
                if (ret == MAP_FAILED)
                        die_errno("Out of memory? mmap failed");


@@ -961,7 +954,7 @@ unsigned char *use_pack(struct packed_git *p,
                        win->len = (size_t)len;
                        pack_mapped += win->len;
                        while (packed_git_limit < pack_mapped
                        win->len = (size_t)len;
                        pack_mapped += win->len;
                        while (packed_git_limit < pack_mapped

-                               && unuse_one_window(p, p->pack_fd))
+                               && unuse_one_window(p))

                                ; /* nothing */
                        win->base = xmmap(NULL, win->len,
                                PROT_READ, MAP_PRIVATE,
                                ; /* nothing */
                        win->base = xmmap(NULL, win->len,
                                PROT_READ, MAP_PRIVATE,


@@ -1007,7 +1000,7 @@ static struct packed_git *alloc_packed_git(int extra)
 
 static void try_to_free_pack_memory(size_t size)
 {
 
 static void try_to_free_pack_memory(size_t size)
 {

-       release_pack_memory(size, -1);
+       release_pack_memory(size);

 }
 
 struct packed_git *add_packed_git(const char *path, int path_len, int local)
 }
 
 struct packed_git *add_packed_git(const char *path, int path_len, int local)