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

Karthik Nayak <[email protected]> writes:

> 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?