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

Junio C Hamano <[email protected]> writes:

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

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.

-----BEGIN PGP SIGNATURE-----

iQHKBAEBCgA0FiEEV85Mf2N1cQ/LZcYGPtWfJI5GjH8FAmqLEY4WHGthcnRoaWsu
MTg4QGdtYWlsLmNvbQAKCRA+1Z8kjkaMf9bbC/9IEGi3Y8mMfosVeSFRhU182Y0E
PyB3gPhSAEubjdIy8/7ym1d0h8QOhkbzKnO8UcpSNKpnlUb5IbG4Co2rgTuVw8r1
kOOlUQmY0N66epQhyTPIfaxt6GPj5bt5s6nzlfXnfs9rdwH7Z/FEk0uRMGECyzOI
eH8SjF3UVdOoyWQMY9r8lIfXLJgvccFdONzk+2kyD0Ekt6GubudWt7gI0mxbArJ4
SqCesn3bdDfizBnTtBKANq3XWTCEyxS7BxX3kix4QRSiNk9SK2pXSWUllbATbDCc
4pXrCZfY/vywqtLoMummb2X0BoJh4w4JLpncJ4c1GunTF03QCCa19K/nF9K8FUyy
+lUTVbVv8lhLvK/EvhgxhmpaMnMn1kyAiao7cl7zH7B/vyS6n86PsCRhzFQoH/J3
tkPLZ3GMJtQk7gqf8h9/Xr90pHYDM5nfI2gNKQ+BS20R4OkwIFPvLxPLLkNGnBTh
aAsfAaI0YDWLoYxIfvosTXeD8EijNpUwslKiiN4=
=oBLj
-----END PGP SIGNATURE-----