Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure

Brandon Casey [off-list ref] writes:

From: Brandon Casey <redacted>

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

An interesting find.  The patch from a cursory look reads OK.

Thanks.

This is not likely to occur during upload-pack since upload-pack
reads each object from the pack so that it can peel tags and
advertise the exposed object.

Another interesting find.  Perhaps there is a room for improvements,
as packed-refs file knows what objects the tags peel to?  I vaguely
recall Peff was actively reducing the object access during ref
enumeration in not so distant past...

On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:

Brandon Casey [off-list ref] writes:

quoted

From: Brandon Casey <redacted>

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

An interesting find.  The patch from a cursory look reads OK.

Yeah. I wonder if unuse_one_window() should actually leave the pack fd
open now in general.

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).

We had our core.packedGitWindowSize lowered on GitHub for a while, and
we ran into this warning on busy repositories when we were running "git
gc" on the server. We solved it by bumping the window size so we never
release memory.

But just not closing the descriptor wouldn't work until Brandon's patch,
because we used the same function to release memory and descriptor
pressure. Now we could do them separately (and progressively if we need
to).

quoted

This is not likely to occur during upload-pack since upload-pack
reads each object from the pack so that it can peel tags and
advertise the exposed object.

Another interesting find.  Perhaps there is a room for improvements,
as packed-refs file knows what objects the tags peel to?  I vaguely
recall Peff was actively reducing the object access during ref
enumeration in not so distant past...

Yeah, we should be reading almost no objects these days due to the
packed-refs peel lines. I just did a double-check on what "git
upload-pack . </dev/null >/dev/null" reads on my git.git repo, and it is
only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.
In other words, the tags I got since the last time I ran "git gc". So I
think all is working as designed.

We could give receive-pack the same treatment; I've spent less time
micro-optimizing it because because we (and most sites, I would think)
get an order of magnitude more fetches than pushes.

-Peff

On Tue, Jul 30, 2013 at 12:52 PM, Jeff King [off-list ref] wrote:

On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:

quoted

Brandon Casey [off-list ref] writes:

quoted

From: Brandon Casey <redacted>

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

An interesting find.  The patch from a cursory look reads OK.

Yeah. I wonder if unuse_one_window() should actually leave the pack fd
open now in general.

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).

We had our core.packedGitWindowSize lowered on GitHub for a while, and
we ran into this warning on busy repositories when we were running "git
gc" on the server. We solved it by bumping the window size so we never
release memory.

But just not closing the descriptor wouldn't work until Brandon's patch,
because we used the same function to release memory and descriptor
pressure. Now we could do them separately (and progressively if we need
to).

I had thought about whether to stop closing the pack file in
unuse_one_window(), but didn't have a reason to do so.  I think the
scenario you described provides a justification.  If we're not under
file descriptor pressure and we can possibly avoid rescanning the pack
directory, it sounds like a net win.

quoted

quoted

This is not likely to occur during upload-pack since upload-pack
reads each object from the pack so that it can peel tags and
advertise the exposed object.

Another interesting find.  Perhaps there is a room for improvements,
as packed-refs file knows what objects the tags peel to?  I vaguely
recall Peff was actively reducing the object access during ref
enumeration in not so distant past...

Yeah, we should be reading almost no objects these days due to the
packed-refs peel lines. I just did a double-check on what "git
upload-pack . </dev/null >/dev/null" reads on my git.git repo, and it is
only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.
In other words, the tags I got since the last time I ran "git gc". So I
think all is working as designed.

Ok, looks like this has been the case since your 435c8332 which taught
upload-pack to use peel_ref().  So looks like we do avoid reaching
into the pack for any ref that was read from a (modern) packed-refs
file.  The repository I was testing with had mostly loose refs.
Indeed, after packing refs, upload-pack encounters the same problem as
receive-pack and runs out of file descriptors.

So my comment about upload-pack is not completely accurate.
Upload-pack _can_ run into this problem, but the refs must be packed,
as well as there being enough of them that exist in enough different
pack files to exceed the processes fd limit.

We could give receive-pack the same treatment; I've spent less time
micro-optimizing it because because we (and most sites, I would think)
get an order of magnitude more fetches than pushes.

I don't think it would need the 435c8332 treatment since receive-pack
doesn't peel refs when it advertises them to the client and hence does
not need to load the ref object from the pack file during ref
advertisement, but possibly some of the other stuff you did would be
applicable.  But like you said, the number of fetches far exceed the
number of pushes.

-Brandon

From: Brandon Casey <redacted>

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

This latter situation can occur in show-ref or receive-pack during ref
advertisement.  During ref advertisement, receive-pack will iterate
over every ref in the repository and advertise it to the client after
ensuring that the ref exists in the local repository.  If the ref is
located inside a pack, then the pack is opened to ensure that it
exists, but since the object is not actually read from the pack, no
mmap windows are allocated.  When the number of open packs exceeds
pack_max_fds, unuse_one_window() will not be able to find any windows to
free and will not be able to close any packs.  Once the per-process
file descriptor limit is exceeded, receive-pack will produce a warning,
not an error, for each pack it cannot open, and will then most likely
fail with an error to spawn rev-list or index-pack like:

   error: cannot create standard input pipe for rev-list: Too many open files
   error: Could not run 'git rev-list'

This may also occur during upload-pack when refs are packed (in the
packed-refs file) and the number of packs that must be opened to
verify that these packed refs exist exceeds the file descriptor limit.
If the refs are loose, then upload-pack will read each ref from the
pack (allocating one or more mmap windows) so it can peel tags and
advertise the underlying object.  If the refs are packed and peeled,
then upload-pack will use the peeled sha1 in the packed-refs file and
will not need to read from the pack files, so no mmap windows will be
allocated and just like with receive-pack, unuse_one_window() will
never select these opened packs to close.

When we have file descriptor pressure, in contrast to memory pressure,
we need to free all windows and close the pack file descriptor so that
a new pack can be opened.  Let's introduce a new function
close_one_pack() designed specifically for this purpose to search
for and close the least-recently-used pack, where LRU is defined as

   * pack with oldest mtime and no allocated mmap windows or
   * pack with the least-recently-used windows, i.e. the pack
     with the oldest most-recently-used window

Signed-off-by: Brandon Casey <redacted>
---

The commit message was updated to fix the grammatical error that Eric
Sunshine pointed out, and to correct the paragraph about upload-pack.

-Brandon

 sha1_file.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 8e27db1..7731ab1 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -682,6 +682,67 @@ void close_pack_windows(struct packed_git *p)
 	}
 }
 
+/*
+ * The LRU pack is the one with the oldest MRU window or the oldest mtime
+ * if it has no windows allocated.
+ */
+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w)
+{
+	struct pack_window *w, *this_mru_w;
+
+	/*
+	 * Reject this pack if it has windows and the previously selected
+	 * one does not.  If this pack does not have windows, reject
+	 * it if the pack file is newer than the previously selected one.
+	 */
+	if (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))
+		return;
+
+	for (w = this_mru_w = p->windows; w; w = w->next) {
+		/* Reject this pack if any of its windows are in use */
+		if (w->inuse_cnt)
+			return;
+		/*
+		 * Reject this pack if it has windows that have been
+		 * used more recently than the previously selected pack.
+		 */
+		if (*mru_w && w->last_used > (*mru_w)->last_used)
+			return;
+		if (w->last_used > this_mru_w->last_used)
+			this_mru_w = w;
+	}
+
+	/*
+	 * Select this pack.
+	 */
+	*mru_w = this_mru_w;
+	*lru_p = p;
+}
+
+static int close_one_pack(void)
+{
+	struct packed_git *p, *lru_p = NULL;
+	struct pack_window *mru_w = NULL;
+
+	for (p = packed_git; p; p = p->next) {
+		if (p->pack_fd == -1)
+			continue;
+		find_lru_pack(p, &lru_p, &mru_w);
+	}
+
+	if (lru_p) {
+		close_pack_windows(lru_p);
+		close(lru_p->pack_fd);
+		pack_open_fds--;
+		lru_p->pack_fd = -1;
+		if (lru_p == last_found_pack)
+			last_found_pack = NULL;
+		return 1;
+	}
+
+	return 0;
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
 	struct pack_window *w = *w_cursor;

@@ -777,7 +838,7 @@ static int open_packed_git_1(struct packed_git *p)
 			pack_max_fds = 1;
 	}
 
-	while (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))
+	while (pack_max_fds <= pack_open_fds && close_one_pack())
 		; /* nothing */
 
 	p->pack_fd = git_open_noatime(p->pack_name);

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

From: Brandon Casey <redacted>

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 <redacted>
---
 builtin/pack-objects.c |  2 +-
 git-compat-util.h      |  2 +-
 sha1_file.c            | 21 +++++++--------------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..4eb0521 100644
--- 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();
-	release_pack_memory(size, -1);
+	release_pack_memory(size);
 	read_unlock();
 }
 

diff --git a/git-compat-util.h b/git-compat-util.h
index cc4ba4d..29b1ee3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -517,7 +517,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
 
-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);

diff --git a/sha1_file.c b/sha1_file.c
index 7731ab1..d26121a 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -614,7 +614,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;

@@ -628,15 +628,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;
-		else {
+		else
 			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;

@@ -644,10 +637,10 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
 	return 0;
 }
 
-void release_pack_memory(size_t need, int fd)
+void release_pack_memory(size_t need)
 {
 	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 */
 }
 

@@ -658,7 +651,7 @@ void *xmmap(void *start, size_t length,
 	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");

@@ -954,7 +947,7 @@ unsigned char *use_pack(struct packed_git *p,
 			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,

@@ -1000,7 +993,7 @@ static struct packed_git *alloc_packed_git(int extra)
 
 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)

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey [off-list ref] wrote:

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?
I remember a video of Greg Kroah-Hartman where he talked about that
(the video was posted by Junio on G+).

On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:

On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey [off-list ref] wrote:

quoted

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?

Talking about legal, is it a problem if a commit isn't signed-off by
it's committer or author e-mail? Like in this case where the sign-off is
from gmail.com and the committer from nvidia.com?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

On Wed, Jul 31, 2013 at 2:08 PM, Antoine Pelisse [off-list ref] wrote:

On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey [off-list ref] wrote:

quoted

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?
I remember a video of Greg Kroah-Hartman where he talked about that
(the video was posted by Junio on G+).

Me either thank God.  Are those footers even enforceable?  I mean,
really, if someone mistakenly sends me their corporate financial
numbers am I supposed to be under some legal obligation not to share
it?  I always assumed it was a scare tactic that lawyers like to use.

To address the text of the footer, I'd say the "intended recipient(s)"
are those on the "to" line which includes git@vger.kernel.org and the
implicit use is for inclusion and distribution in the git source code.

Anyway, I doubt I would have any influence on getting the footer
removed.  If Junio would rather me not submit patches with that
footer, then I'd try to find a workaround.

-Brandon

On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson [off-list ref] wrote:

On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:

quoted

On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey [off-list ref] wrote:

quoted

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?

Talking about legal, is it a problem if a commit isn't signed-off by
it's committer or author e-mail? Like in this case where the sign-off is
from gmail.com and the committer from nvidia.com?

It never has been.  My commits should have the author and committer
set to my gmail address actually.

Others have sometimes used the two fields to distinguish between a
corporate identity (i.e. me@somecompany.com) that represents the
funder of the work and a canonical identity (me@personalemail.com)
that identifies the person that performed the work.

-Brandon

On Wed, Jul 31, 2013 at 02:31:34PM -0700, Brandon Casey wrote:

On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson [off-list ref] wrote:

quoted

On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:

quoted

On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey [off-list ref] wrote:

quoted

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?

Talking about legal, is it a problem if a commit isn't signed-off by
it's committer or author e-mail? Like in this case where the sign-off is
from gmail.com and the committer from nvidia.com?

It never has been.  My commits should have the author and committer
set to my gmail address actually.

Oh, that's why the extra "From: " - field below the header is for.

Others have sometimes used the two fields to distinguish between a
corporate identity (i.e. me@somecompany.com) that represents the
funder of the work and a canonical identity (me@personalemail.com)
that identifies the person that performed the work.

In some contries your work when you're employed does not belong
to you but to your employer and when you're acting for your employer
you're representing the corporate legal person. Therefore two different
e-mails can be seen as two different (legal not physical) persons.

At least that's how I understand those "legal tips for developers" I've
got.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com