Re: [PATCH 2/3] reftable/stack: move list lock to `struct reftable_stack`

Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

The struct `reftable_addition` is used to modify a given stack, as such,
it also includes a `struct reftable_flock` used to obtain the lock to
the list file. While the scope of the field lies within this struct, it
doesn't allow for optimizations to be made on `struct reftable_stack`
itself.

Move the field to `struct reftable_stack`, allowing us to make a simple
optimization around avoiding a stack reload when we have already
obtained a lock. While this is currently possible in the write path, the
write path also contains multiple branches to reads which only work
on top of `struct reftable_stack`, and we would miss the optimization in
such paths.

As long as nobody tries to open a nested or concurrent addition on
the same 'struct reftable_stack', this should be safe, but do we
give enough tools to help the API users avoid doing so?

I may be misreading the code completely, but when a caller already
holds a lock after calling reftable_stack_init_addition() on an
instance of reftable_stack, and then adds another reftable_addition
on the same reftable_stack, flock_acquire(add->stack->list_lock)
would fail because the lock is per stack now, unlike the original
code where the lock was per reftable_addition.  We jump to the
done: label and call reftable_addition_close(), which would release
the lock, which is now shared with other reftable_addition
instances that work on the same stack, which in turn would get the
holders of the lock into trouble, no?

That's a good line of thought and something I didn't think of.

With the current version, this wouldn't work, as you mentioned, the
second `reftable_addition` would free the first's lock. The only way I
can think of is each `reftable_addition` also holding it's own bit
indicating if it acquired the lock and only release the stack lock based
on this bit. This works, I will write a unit test to also validate this
behavior. But I'm wondering if there is a better design.