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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/22/2015 03:32 AM, Stefan Beller wrote:
> By closing the file descriptors after creating the lock file we are not
> limiting the size of the transaction by the number of available file
> descriptors.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c                | 17 +++++++++++++----
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 2013d37..9d01102 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)
>  static int write_sha1_to_lock_file(struct ref_lock *lock,
>  				   const unsigned char *sha1)
>  {
> -	if (fdopen_lock_file(lock->lk, "w") < 0
> -	    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
> +	if (lock->lk->fd == -1) {
> +		if (reopen_lock_file(lock->lk) < 0
> +		    || fdopen_lock_file(lock->lk, "w") < 0
> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
> +		    || close_lock_file(lock->lk) < 0)
> +		    return -1;
> +	} else {
> +		if (fdopen_lock_file(lock->lk, "w") < 0
> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
>  		return -1;
> -	else
> -		return 0;
> +	}
> +	return 0;
>  }

I can't figure out where to apply this series or where to fetch it from,
so I can't see these changes in context, so maybe I'm misunderstanding
something. It looks like this code is doing

    open(), close(), open(), fdopen(), write(), fclose(), rename()

on each lockfile. But don't we have enough information to write the
SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
reduce this to

    open(), fdopen(), write(), fclose(), rename()

, where the first four calls all happen in the initial loop? If a
problem is discovered when writing a later reference, we would roll back
the transaction anyway.

I understand that this would require a bigger rewrite, so maybe it is
not worth it.

>  /*
> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		/* Do not keep all lock files open at the same time. */
> +		close_lock_file(update->lock->lk);
>  	}
>  
>  	/* Perform updates first so live commits remain referenced */
> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu