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

[Junio C Hamano <gitster@pobox.com>]

Brandon Casey <bcasey@nvidia.com> writes:

> 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

Even though what it says is not incorrect, the phrasing around here,
especially "so it can", confused me in my first reading.  It reads
objects "in order to" peel and advertise (and as a side-effect it
can lead to windows into packs that eventually help relieaving the
fd pressure), but a quick scan led me to misread it as "so it can do
peel and advertise just fine", which misses the point, because it is
not like we are having trouble peeling and advertising.

Also, the objects at the tips of refs and the objects they point at
may be loose objects, which is very likely for branch tips.  The fd
pressure will not be relieved in such a case even if these refs were
packed.

I've tentatively reworded the above section like so:

    ... If the refs are loose, then upload-pack will read each ref
    from the object database (if the object is in a pack, allocating
    one or more mmap windows for it) in order to peel tags and
    advertise the underlying object.  But when the refs are packed
    and peeled, 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 ...

> +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;
> +}

OK, so in this codepath where we know we are under fd pressure, we
find the pack that is least recently used that can be closed, and
use close_pack_windows() to reclaim all of its open windows (if
any), which takes care of the accounting for pack_mapped and
pack_open_windows, but we need to do the pack_open_fds accounting
here ourselves.  Makes sense to me.

Thanks.

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