Re: [PATCHv3] refs.c: enable large transactions

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

Stefan Beller <sbeller@google.com> writes:

> diff --git a/refs.c b/refs.c
> index 4f495bd..7ce7b97 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3041,6 +3041,13 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = EINVAL;
>  		return -1;
>  	}
> +	if (lock->lk->fd == -1 && reopen_lock_file(lock->lk) == -1) {

Granted, we explicitly assign -1 to lk->fd when we know it is
closed, and the return value from reopen_lock_file() can come only
from the return value from open(2), which is defined to return -1
(i.e. not just any negative value) upon failure, but still, isn't it
customary to check with "< 0" rather than "== -1" for errors?

> +		int save_errno = errno;
> +		error("Couldn't reopen %s", lock->lk->filename.buf);

No need to change this line, but I noticed that we might want to do
something about the first one of the following two:

    $ git grep -e '[ 	]error(_*"[A-Z]' | wc -l
    146
    $ git grep -e '[ 	]error(_*"[a-z]' | wc -l
    390

> +		unlock_ref(lock);
> +		errno = save_errno;
> +		return -1;
> +	}

Is this the only place in the entire codebase where a lockfile,
which may have been closed to save number of open file descriptors,
needs to be reopened?  If I understand correctly, lockfile API is
not for sole use of refs (don't the index and the pack codepaths use
it, too?), so it may give us a better abstraction to have a helper
function in lockfile.[ch] that takes a lock object, i.e.

	int make_lock_fd_valid(struct lock_file *lk)
        {
		if (lk->fd < 0 && reopen_lock_file(lk) < 0) {
			... error ...
                        return -1;
		}
                return 0;
	}

and to call it at this point, i.e.

	if (make_lock_fd_valid(lock->lk) < 0)
        	return -1;

perhaps?

> @@ -3733,6 +3741,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		return 0;
>  	}
>  
> +	/*
> +	 * We need to open many files in a large transaction, so come up with
> +	 * a reasonable maximum. We still keep some spares for stdin/out and
> +	 * other open files. Experiments determined we need more fds when
> +	 * running inside our test suite than directly in the shell. It's
> +	 * unclear where these fds come from. 25 should be a reasonable large
> +	 * number though.
> +	 */

> +	remaining_fds = get_max_fd_limit();
> +	if (remaining_fds > 25)
> +		remaining_fds -= 25;
> +	else
> +		remaining_fds = 0;

Is the value of pack_open_fds visible from here? I am wondering if
we might want "scratch fds Git can use for its own use" accounting
so that the two subsystems can collectively say "it is still safe
to use one more fd and leave 25 for other people". With the code
structure proposed here, the pack reader can grab all but 25 fds,
which would leave the rest of Git including this code only 25,
and the value in remaining_fds would become totally meaningless.

It certainly can wait to worry about that and we do not have to do
anything about it in this patch, but it may still be a good idea to
leave "NEEDSWORK:" comment here (and point at open_packed_git_1() in
sha1_file.c in the comment).

I do not think the other side needs to know about the fd consumption
in this function, as what is opened in here will be closed before
this function returns.

> @@ -3762,6 +3784,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		if (!(flags & REF_HAVE_NEW) ||
> +		    is_null_sha1(update->new_sha1) ||
> +		    remaining_fds == 0)
> +			close_lock_file(update->lock->lk);
> +		else
> +			remaining_fds--;
>  	}