On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> When creating a new addition via either `reftable_stack_new_addition()`
> or its convenince wrapper `reftable_stack_add()` we:
>
> 1. Create the "tables.list.lock" file.
>
> 2. Verify that the current version of the "tables.list" file is
> up-to-date.
>
> 3. Write the new table records if so.
>
> By default, the second step would cause us to bail out if we see that
> there has been a concurrent write to the stack that made our in-memory
> copy of the stack out-of-date. This is a safety mechanism to not write
> records to the stack based on outdated information.
>
> The downside though is that concurrent writes may now cause us to bail
> out, which is not a good user experience. In addition, this isn't even
> necessary for us, as Git knows to perform all checks for the old state
> of references under the lock. (Well, in all except one case: when we
> expire the reflog we first create the log iterator before we create the
> lock, but this ordering is fixed as part of this commit.)
>
> Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
> flag. The effect of this flag is that we reload the stack after having
> acquired the lock in case the stack is out-of-date. This plugs the race
> with concurrent writers, but we continue performing the verifications of
> the expected old state to catch actual conflicts in the references we
> are about to write.
>
> Adapt the remaining callsites that don't yet pass this flag to do so.
> While at it, drop a needless manual reload.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> refs/reftable-backend.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 3f0deab338..66d25411f1 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
> if (!arg) {
> struct reftable_addition *addition;
>
> - ret = reftable_stack_reload(be->stack);
> - if (ret)
> - return ret;
Here we don't need to reload because `reftable_stack_new_addition()`
immediately following already does this for us.
> -
> ret = reftable_stack_new_addition(&addition, be->stack,
> REFTABLE_STACK_NEW_ADDITION_RELOAD);
> if (ret) {
> @@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
> ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
> if (ret)
> goto done;
> - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> + REFTABLE_STACK_NEW_ADDITION_RELOAD);
>
> done:
> assert(ret != REFTABLE_API_ERROR);
> @@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
> ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
> if (ret)
> goto done;
> - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> + REFTABLE_STACK_NEW_ADDITION_RELOAD);
>
> done:
> assert(ret != REFTABLE_API_ERROR);
> @@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
> goto done;
> arg.stack = be->stack;
>
> - ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
> + ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
> + REFTABLE_STACK_NEW_ADDITION_RELOAD);
>
> done:
> return ret;
> @@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
> return ret;
> arg.stack = be->stack;
>
> - ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
> + ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
> + REFTABLE_STACK_NEW_ADDITION_RELOAD);
>
> assert(ret != REFTABLE_API_ERROR);
> return ret;
> @@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
> if (ret < 0)
> goto done;
>
> - ret = reftable_stack_init_log_iterator(be->stack, &it);
> + ret = reftable_stack_new_addition(&add, be->stack,
> + REFTABLE_STACK_NEW_ADDITION_RELOAD);
Here we change the order so that we now acquire the lock first.
This patch looks good to me :)
-Justin
> if (ret < 0)
> goto done;
>
> - ret = reftable_iterator_seek_log(&it, refname);
> + ret = reftable_stack_init_log_iterator(be->stack, &it);
> if (ret < 0)
> goto done;
>
> - ret = reftable_stack_new_addition(&add, be->stack, 0);
> + ret = reftable_iterator_seek_log(&it, refname);
> if (ret < 0)
> goto done;
>
>
> --
> 2.51.0.rc1.163.g2494970778.dirty
>
