Re: [PATCH 6/8] reftable/stack: use lock_file when adding table to "tables.list"

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 31, 2024, 23:02 UTC
Message-ID: <fpli326ssfh66sojj4bqn2ztaks3lceqce7nwb4jzrducpvfxz@uoeabcbebaez>
In-Reply-To: <9703246156152e1c8c92b4237c8dc3a9ef59901a.1722435214.git.ps@pks.im>

On 24/07/31 04:15PM, Patrick Steinhardt wrote:

Show 5 quoted lines

> When modifying "tables.list", we need to lock the list before updating
> it to ensure that no concurrent writers modify the list at the same
> point in time. While we do this via the `lock_file` subsystem when
> compacting the stack, we manually handle the lock when adding a new
> table to it. While not wrong, it is at least inconsistent.

Indeed, much more consistent to use the lockfile API here. :)

> 
> Refactor the code to consistenly lock "tables.list" via the `lock_file`

s/consistenly/consistently/

Show 74 quoted lines

> subsytem.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 9ca549294f..9cc91a262c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -567,7 +567,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
>  }
>  
>  struct reftable_addition {
> -	struct tempfile *lock_file;
> +	struct lock_file tables_list_lock;
>  	struct reftable_stack *stack;
>  
>  	char **new_tables;
> @@ -581,13 +581,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  					struct reftable_stack *st)
>  {
>  	struct strbuf lock_file_name = STRBUF_INIT;
> -	int err = 0;
> -	add->stack = st;
> +	int err;
>  
> -	strbuf_addf(&lock_file_name, "%s.lock", st->list_file);
> +	add->stack = st;
>  
> -	add->lock_file = create_tempfile(lock_file_name.buf);
> -	if (!add->lock_file) {
> +	err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,
> +					LOCK_NO_DEREF);
> +	if (err < 0) {
>  		if (errno == EEXIST) {
>  			err = REFTABLE_LOCK_ERROR;
>  		} else {
> @@ -596,7 +596,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  		goto done;
>  	}
>  	if (st->opts.default_permissions) {
> -		if (chmod(add->lock_file->filename.buf, st->opts.default_permissions) < 0) {
> +		if (chmod(get_lock_file_path(&add->tables_list_lock),
> +			  st->opts.default_permissions) < 0) {
>  			err = REFTABLE_IO_ERROR;
>  			goto done;
>  		}
> @@ -635,7 +636,7 @@ static void reftable_addition_close(struct reftable_addition *add)
>  	add->new_tables_len = 0;
>  	add->new_tables_cap = 0;
>  
> -	delete_tempfile(&add->lock_file);
> +	rollback_lock_file(&add->tables_list_lock);
>  	strbuf_release(&nm);
>  }
>  
> @@ -651,7 +652,7 @@ void reftable_addition_destroy(struct reftable_addition *add)
>  int reftable_addition_commit(struct reftable_addition *add)
>  {
>  	struct strbuf table_list = STRBUF_INIT;
> -	int lock_file_fd = get_tempfile_fd(add->lock_file);
> +	int lock_file_fd = get_lock_file_fd(&add->tables_list_lock);
>  	int err = 0;
>  	size_t i;
>  
> @@ -674,13 +675,14 @@ int reftable_addition_commit(struct reftable_addition *add)
>  		goto done;
>  	}
>  
> -	err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);
> +	err = fsync_component(FSYNC_COMPONENT_REFERENCE,
> +			      get_lock_file_fd(&add->tables_list_lock));

I might be missing something, but is there a reason we have to get the lock file fd again instead of just using `lock_file_fd`?

Show 13 quoted lines

>  	if (err < 0) {
>  		err = REFTABLE_IO_ERROR;
>  		goto done;
>  	}
>  
> -	err = rename_tempfile(&add->lock_file, add->stack->list_file);
> +	err = commit_lock_file(&add->tables_list_lock);
>  	if (err < 0) {
>  		err = REFTABLE_IO_ERROR;
>  		goto done;
> -- 
> 2.46.0.dirty
>