[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#6104) race condition with cancel operation

hyc@symas.com writes:
>> Bitflags should be unnecessary as such, just need a set of values.
>> Though it wouldn't hurt if they were chosen so one could use bit
>> operations to reduce checks for "o_abandon == A or B".  "Suppress
>> response" value mentioned in ITS#6138 also needed, maybe that must be a
>> bitflag when combined with Cancel stuff.
> Yes, it still seems some bitflags will be more appropriate.

As long as we decide which flag combinations are (not) valid.  Problem
with "unrestricted" bitflags compared to a list of values is that they
allow and thus require support for flag combinations we have no real
need to support.

>> Question:
>>    syncprov_op_abandon() holds&si->si_ops_mutex when setting
>>    so->s_op->o_abandon.  Does it need that?  The function needs to
>>    grab op->o_conn->c_mutex when called as Cancel, but must not do
>>    that while holding&si->si_ops_mutex since Abandon grabs the
>>    locks in the opposite order.
> syncprov_op_abandon() can remove ops from the list, so yes, it must
> hold si_ops_mutex.

Right, but does it need si_ops_mutex while setting o_abandon?  Cancel
could remove the op, unlock si_ops_mutex, lock c_mutex, update
o_abandon.  Alternative: Lock c_mutex first (that must be safe since
Abandon is doing it anyway), then lock si_ops_mutex.  So it's not
a critical difference, just that a lock must be held a bit longer.