Re: [PATCH] Limit file descriptors used by packs

"Shawn O. Pearce" [off-list ref] writes:

In this particular part of C Git, if we are bumping up against the
hard pack_max_fds limit we're already into some pretty difficult
computation. Trying to push the rlimit higher in order to avoid
close/open calls as we cycle through fds isn't really going to make
a huge difference on end-user latency for the command to finish
its task. So maybe we are better off honoring the rlim_cur that we
inherited from the user/environment.

Let's step back a bit.

You are holding too many file descriptors open because you have too many
pack-files in your repository.

I am not going to question why they aren't repacked before their number
gets too large---that is not a valid question to ask in the context of
this discussion.  But it is a valid question to ask why we need an open
file descriptor for each of them to begin with, and if we really need to,
isn't it?

We keep one file descriptor open for each .pack in which we have pack
window(s).  The reason we keep one file descriptor open is because we
might want to mmap() different portions of a .pack file that is already in
use through the file descriptor to open new window(s) on demand.

For a .pack that fits inside a single pack window, however, can't we close
the file descriptor immediately after mmap() it to obtain a sole window
into it?  For such a .pack, we would either have one window into it, or
the .pack is not in use and have no window into it.  When the number of
windows drops to zero, the current code closes the file descriptor, and
upon next use, we already let the caller access the .pack correctly, so
we already should know when to re-open a file descriptor to it as needed.

I am wondering if it is a viable approach to, inside open_packed_git_1(),

 - mark a packfile that is small enough (i.e. st.st_size aka p->pack_size
   is smaller than packed_git_window_size) as "persistently mapped";

 - keep a window that covers the entire thing in p->windows as a single
   and sole window into it for such a pack; and

 - close the file descriptor when we did the above

and have use_pack() notice that single window and use it.

This is not an alternate proposal to your patch, but the approach may
alleviate the resource pressure in the first place, no?

On Tue, Mar 1, 2011 at 06:24, Junio C Hamano [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

In this particular part of C Git, if we are bumping up against the
hard pack_max_fds limit we're already into some pretty difficult
computation. Trying to push the rlimit higher in order to avoid
close/open calls as we cycle through fds isn't really going to make
a huge difference on end-user latency for the command to finish
its task. So maybe we are better off honoring the rlim_cur that we
inherited from the user/environment.

Let's step back a bit.

...

For a .pack that fits inside a single pack window, however, can't we close
the file descriptor immediately after mmap() it to obtain a sole window
into it?

Yes. And its unrelated to this patch. You can still run out of file
descriptors because you have a lot of large packs. :-)

I've considered this in the past, but avoided it because I thought the
unuse_one_window() function might become more complex. But its not, we
can just keep popping windows until the condition is met, which for a
file descriptor is that we are below the limit.

I'll send a follow-up patch that builds on top of this one to close
the pack fd if the entire thing fits into one window.

-- 
Shawn.

If a pack file is small enough that its entire contents fits within
one mmap window, mmap the file and then immediately close its file
descriptor.  This reduces the number of file descriptors that are
needed to read from repositories with many tiny pack files, such
as one that has received 1000 pushes (and created 1000 small pack
files) since its last repack.

Signed-off-by: Shawn O. Pearce <redacted>
---
 cache.h       |    3 ++-
 fast-import.c |    1 +
 sha1_file.c   |   41 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 08a9022..1d362c4 100644
--- a/cache.h
+++ b/cache.h

@@ -914,7 +914,8 @@ extern struct packed_git {
 	time_t mtime;
 	int pack_fd;
 	unsigned pack_local:1,
-		 pack_keep:1;
+		 pack_keep:1,
+		 do_not_close:1;
 	unsigned char sha1[20];
 	/* something like ".git/objects/pack/xxxxx.pack" */
 	char pack_name[FLEX_ARRAY]; /* more */

diff --git a/fast-import.c b/fast-import.c
index 3886a1b..4916a9d 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -872,6 +872,7 @@ static void start_packfile(void)
 	p = xcalloc(1, sizeof(*p) + strlen(tmpfile) + 2);
 	strcpy(p->pack_name, tmpfile);
 	p->pack_fd = pack_fd;
+	p->do_not_close = 1;
 	pack_file = sha1fd(pack_fd, p->pack_name);
 
 	hdr.hdr_signature = htonl(PACK_SIGNATURE);

diff --git a/sha1_file.c b/sha1_file.c
index 7850c18..fbb178a 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -597,7 +597,8 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
 			lru_l->next = lru_w->next;
 		else {
 			lru_p->windows = lru_w->next;
-			if (!lru_p->windows && lru_p->pack_fd != keep_fd) {
+			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;

@@ -813,14 +814,13 @@ unsigned char *use_pack(struct packed_git *p,
 {
 	struct pack_window *win = *w_cursor;
 
-	if (p->pack_fd == -1 && open_packed_git(p))
-		die("packfile %s cannot be accessed", p->pack_name);
-
 	/* Since packfiles end in a hash of their content and it's
 	 * pointless to ask for an offset into the middle of that
 	 * hash, and the in_window function above wouldn't match
 	 * don't allow an offset too close to the end of the file.
 	 */
+	if (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))
+		die("packfile %s cannot be accessed", p->pack_name);
 	if (offset > (p->pack_size - 20))
 		die("offset beyond end of packfile (truncated pack?)");
 

@@ -834,6 +834,10 @@ unsigned char *use_pack(struct packed_git *p,
 		if (!win) {
 			size_t window_align = packed_git_window_size / 2;
 			off_t len;
+
+			if (p->pack_fd == -1 && open_packed_git(p))
+				die("packfile %s cannot be accessed", p->pack_name);
+
 			win = xcalloc(1, sizeof(*win));
 			win->offset = (offset / window_align) * window_align;
 			len = p->pack_size - win->offset;

@@ -851,6 +855,12 @@ unsigned char *use_pack(struct packed_git *p,
 				die("packfile %s cannot be mapped: %s",
 					p->pack_name,
 					strerror(errno));
+			if (!win->offset && win->len == p->pack_size
+				&& !p->do_not_close) {
+				close(p->pack_fd);
+				pack_open_fds--;
+				p->pack_fd = -1;
+			}
 			pack_mmap_calls++;
 			pack_open_windows++;
 			if (pack_mapped > peak_pack_mapped)

@@ -1951,6 +1961,27 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	return 0;
 }
 
+static int is_pack_valid(struct packed_git *p)
+{
+	/* An already open pack is known to be valid. */
+	if (p->pack_fd != -1)
+		return 1;
+
+	/* If the pack has one window completely covering the
+	 * file size, the pack is known to be valid even if
+	 * the descriptor is not currently open.
+	 */
+	if (p->windows) {
+		struct pack_window *w = p->windows;
+
+		if (!w->offset && w->len == p->pack_size)
+			return 1;
+	}
+
+	/* Force the pack to open to prove its valid. */
+	return !open_packed_git(p);
+}
+
 static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 {
 	static struct packed_git *last_found = (void *)1;

@@ -1980,7 +2011,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 			 * it may have been deleted since the index
 			 * was loaded!
 			 */
-			if (p->pack_fd == -1 && open_packed_git(p)) {
+			if (!is_pack_valid(p)) {
 				error("packfile %s cannot be accessed", p->pack_name);
 				goto next;
 			}

-- 
1.7.4.1.249.g4aa7