Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked

Karthik Nayak <[email protected]> Sun, 23 Aug 2026 10:39:08 -0500
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZT4WuBh2eansJhzMm5F39UCTiOP1vgQ+yfQK0syzbm1uw@mail.gmail.com>
Justin Tobler <[email protected]> writes:

> On 26/08/19 03:19PM, Karthik Nayak wrote:
>> When making modifications to the reftable stack, the stack obtains a
>> lock to the list file and removes the lock after the commit phase. Since
>> most operations reload the stack to ensure we have the latest state, any
>> branched operation during the locked phase could trigger a state reload.
>>
>> To prevent data loss due to concurrent writes, state reload is necessary
>> right after obtaining the lock. But any reloads after that are just a
>> no-op. Now that the struct has access to the lock file status, simply
>> skip reloading if the lock is present.
>
> Makes sense.
>
>> Benchmarking with a fixed, non-symbolic target OID shows a modest but
>> consistent ~1-2% improvement in clock time for `update-ref` across ref
>> counts ranging from 2,000 to 100,000.
>>
>> We can see better improvements in the number of syscall counts. On
>> master, the number of calls to `newfstatat()` grows linearly with the
>> number of refs created. With this patch, the number is now a constant:
>>
>>   refcount   master   patch
>>   --------   ------   ------
>>   1,000      1,059       55
>>   5,000      5,059       55
>>   10,000     10,059      55
>>   20,000     20,059      55
>>
>> Reported-by: Jeff King <[email protected]>
>> Signed-off-by: Karthik Nayak <[email protected]>
>> ---
>>  reftable/stack.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/reftable/stack.c b/reftable/stack.c
>> index e449af9c03..433a611ed1 100644
>> --- a/reftable/stack.c
>> +++ b/reftable/stack.c
>> @@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
>>
>>  /*
>>   * Check whether the given stack is up-to-date with what we have in memory.
>> + * If skip_if_locked is set skip stack reloading if the stack is currently
>> + * locked. Stack reloading must _not_ be skipped right after obtaining the
>> + * lock, to check for concurrent updates which may have happened.
>> + *
>>   * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
>>   * otherwise.
>>   */
>> -static int stack_uptodate(struct reftable_stack *st)
>> +static int stack_uptodate(struct reftable_stack *st, int skip_if_locked)
>>  {
>>  	char **names = NULL;
>>  	int err;
>>
>> +	if (skip_if_locked && st->list_lock.fd != -1)
>> +		return 0;
>> +
>>  	/*
>>  	 * When we have cached stat information available then we use it to
>>  	 * verify whether the file has been rewritten.
>> @@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st)
>>
>>  int reftable_stack_reload(struct reftable_stack *st)
>>  {
>> -	int err = stack_uptodate(st);
>> +	int err = stack_uptodate(st, 1);
>
> Ok, this appears to be the only call site where is actually want to skip
> if there is a lock present. Could we instead just not invoke
> `stack_uptodate()` in such cases? That way we don't have to change its
> function signature and can leave all other existing call sites alone.
>
> -Justin

We want to selectively invoke `stack_uptodate()` based on if the lock
file exists. If we move that logic outside of `stack_uptodate()` further
callees would have to replicate that logic. While that's not an issue,
missing it becomes easier. So I made this explicit choice.
signature.asc (application/pgp-signature, 690 B)