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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2013, 17:12 UTC
Message-ID: <7vsiyts5bb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1375300297-6744-1-git-send-email-bcasey@nvidia.com>

Brandon Casey <bcasey@nvidia.com> writes:

Show 6 quoted lines

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

Show 23 quoted lines

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

Show 12 quoted lines

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