Re: [PATCH] Limit file descriptors used by packs

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2011, 14:24 UTC
Message-ID: <7vwrkiex62.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110228204727.GB26052@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 7 quoted lines

> 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?