Re: [PATCH] Limit file descriptors used by packs

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 28, 2011, 20:47 UTC
Message-ID: <20110228204727.GB26052@spearce.org>
In-Reply-To: <7vwrkjhp27.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > ...  The output file is opened by pack-objects after
> > object enumeration and delete compression are done, ...
> 
> s/delete/deflate/, I guess.

s/delete/delta/ is what I meant. "Compressing objects" is the
delta compression phase. The fact that we save some small deltas in
deflated format during this phase is an uninteresting implementation
detail not worth mentioning in the comments.

Show 18 quoted lines

> > diff --git a/sha1_file.c b/sha1_file.c
> > index d949b35..8863ff6 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -708,9 +713,35 @@ static int open_packed_git_1(struct packed_git *p)
> >  	if (!p->index_data && open_pack_index(p))
> >  		return error("packfile %s index unavailable", p->pack_name);
> >  
> > +	if (!pack_max_fds) {
> > + ...
> > +		if (lim.rlim_cur < lim.rlim_max) {
> > +			lim.rlim_cur = lim.rlim_max;
> > +			if (!setrlimit(RLIMIT_NOFILE, &lim))
> > +				max_fds = lim.rlim_max;
> > +		}
> 
> This is somewhat questionable, isn't it?  We don't know why the user chose
> to ulimit the process yet forcibly bust that limit without telling him?

Maybe you are right.

In network server code is somewhat common to push the rlim_cur to rlim_max if its not already there, since you might need to use a lot of fds to handle a lot of concurrent clients. So habit sort of caused me to just do this out of instinct.

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.

I'll respin a v2 for you.

-- 
Shawn.