Re: [PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2013, 16:26 UTC
Message-ID: <7v38qsqcs8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1375421793-32224-1-git-send-email-drafnel@gmail.com>

Brandon Casey <drafnel@gmail.com> writes:

Show 45 quoted lines

> +/*
> + * The LRU pack is the one with the oldest MRU window, preferring packs
> + * with no used windows, or the oldest mtime if it has no windows allocated.
> + */
> +static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w, int *accept_windows_inuse)
> +{
> +	struct pack_window *w, *this_mru_w;
> +	int has_windows_inuse = 0;
> +
> +	/*
> +	 * Reject this pack if it has windows and the previously selected
> +	 * one does not.  If this pack does not have windows, reject
> +	 * it if the pack file is newer than the previously selected one.
> +	 */
> +	if (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))
> +		return;
> +
> +	for (w = this_mru_w = p->windows; w; w = w->next) {
> +		/*
> +		 * Reject this pack if any of its windows are in use,
> +		 * but the previously selected pack did not have any
> +		 * inuse windows.  Otherwise, record that this pack
> +		 * has windows in use.
> +		 */
> +		if (w->inuse_cnt) {
> +			if (*accept_windows_inuse)
> +				has_windows_inuse = 1;
> +			else
> +				return;
> +		}
> +
> +		if (w->last_used > this_mru_w->last_used)
> +			this_mru_w = w;
> +
> +		/*
> +		 * Reject this pack if it has windows that have been
> +		 * used more recently than the previously selected pack.
> +		 * If the previously selected pack had windows inuse and
> +		 * we have not encountered a window in this pack that is
> +		 * inuse, skip this check since we prefer a pack with no
> +		 * inuse windows to one that has inuse windows.
> +		 */
> +		if (*mru_w && *accept_windows_inuse == has_windows_inuse &&
> +		    this_mru_w->last_used > (*mru_w)->last_used)
> +			return;

The "*accept_windows_inuse == has_windows_inuse" part is hard to grok, together with the fact that this statement is evaluated for each and every "w", even though it is about this_mru_w and that variable is not updated in every iteration of the loop. Can you clarify/simplify this part of the code a bit more?

For example, would the above be equivalent to this?

		if (w->last_used < this_mru_w->last_used)
			continue;

		this_mru_w = w;
                if (has_windows_inuse && *mru_w &&
                    w->last_used > (*mru_w)->last_used)
			return;

That is, if we already know a more recently used window in this pack, we do not have to do anything to maintain mru_w. Otherwise, remember that this window is the most recently used one in this pack, and if it is newer than the newest one from the pack we are going to pick, we refrain from picking this pack.

But we do not reject ourselves if we haven't seen a window that is in use (yet).

Show 44 quoted lines

> +	}
> +
> +	/*
> +	 * Select this pack.
> +	 */
> +	*mru_w = this_mru_w;
> +	*lru_p = p;
> +	*accept_windows_inuse = has_windows_inuse;
> +}
> +
> +static int close_one_pack(void)
> +{
> +	struct packed_git *p, *lru_p = NULL;
> +	struct pack_window *mru_w = NULL;
> +	int accept_windows_inuse = 1;
> +
> +	for (p = packed_git; p; p = p->next) {
> +		if (p->pack_fd == -1)
> +			continue;
> +		find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
> +	}
> +
> +	if (lru_p) {
> +		close(lru_p->pack_fd);
> +		pack_open_fds--;
> +		lru_p->pack_fd = -1;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  void unuse_pack(struct pack_window **w_cursor)
>  {
>  	struct pack_window *w = *w_cursor;
> @@ -768,7 +845,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);