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

> This is another attempt on enabling large transactions
> (large in terms of open file descriptors). We keep track of how many
> lock files are opened by the ref_transaction_commit function.
> When more than a reasonable amount of files is open, we close
> the file descriptors to make sure the transaction can continue.
> 
> Another idea I had during implementing this was to move this file
> closing into the lock file API, such that only a certain amount of
> lock files can be open at any given point in time and we'd be 'garbage
> collecting' open fds when necessary in any relevant call to the lock
> file API. This would have brought the advantage of having such
> functionality available in other users of the lock file API as well.
> The downside however is the over complication, you really need to always
> check for (lock->fd != -1) all the time, which may slow down other parts
> of the code, which did not ask for such a feature.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> * Removed unneeded braces in the condition to check if we want to close
>   the lock file.
> * made the counter for the remaining fds an unsigned int. That is what   
>   get_max_fd_limit() returns, so there are no concerns for an overflow.
>   Also it cannot go below 0 any more.
> * moved the initialisation of the remaining_fds a bit down and added a comment  
>   
>  refs.c                | 21 +++++++++++++++++++++
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
>  
>  
> 
> diff --git a/refs.c b/refs.c
> index 4f495bd..34cfcdf 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3041,6 +3041,8 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = EINVAL;
>  		return -1;
>  	}
> +	if (lock->lk->fd == -1)
> +		reopen_lock_file(lock->lk);

>  	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
>  	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
>  	    close_ref(lock) < 0) {
> @@ -3718,6 +3720,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  			   struct strbuf *err)
>  {
>  	int ret = 0, i;
> +	unsigned int remaining_fds;
>  	int n = transaction->nr;
>  	struct ref_update **updates = transaction->updates;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
> @@ -3733,6 +3736,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. 32 should be a reasonable large
> +	 * number though.
> +	 */
> +	remaining_fds = get_max_fd_limit();
> +	if (remaining_fds > 32)
> +		remaining_fds -= 32;
> +	else
> +		remaining_fds = 0;
> +
>  	/* Copy, sort, and reject duplicate refs */
>  	qsort(updates, n, sizeof(*updates), ref_update_compare);
>  	if (ref_update_reject_duplicates(updates, n, err)) {
> @@ -3762,6 +3779,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		if (remaining_fds > 0)
> +			remaining_fds--;
> +		else
> +			close_lock_file(update->lock->lk);