[libvirt] [PATCH v5 1/3] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

[Michal Privoznik]

On 02/09/2018 02:00 PM, John Ferlan wrote:
> 
> 
> On 02/09/2018 03:41 AM, Michal Privoznik wrote:
>> On 02/08/2018 04:06 PM, John Ferlan wrote:
>>> [...]
>>>
>>>>>>> +static void
>>>>>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>>>>>> +{
>>>>>>> +    virObjectRWUnlock(obj);
>>>>>>> +    virObjectRWLockWrite(obj);
>>>>>>> +}
>>>>>>
>>>>>> How can this not deadlock? This will work only if @obj is locked exactly
>>>>>> once. But since we allow recursive locks any lock() that happens in the
>>>>>> 2nd level must deadlock with this. On the other hand, there's no such
>>>>>> case in the code currently. But that doesn't stop anybody from calling
>>>>>> PromoteWrite() without understanding its limitations.
>>>>>>
>>>>>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>>>>>> not a good candidate for virObjectRWLocable? It is still a good
>>>>>> candidate for virObject though.
>>>>>>
>>>>>> Or if you want to spend extra time on this, maybe introducing
>>>>>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>>>>>
>>>>>>
>>>>>
>>>>> I dunno, worked for me. The helper is being called from a thread that
>>>>> has taken out a READ lock at most one time and needs to get the WRITE
>>>>> lock in order to change things. If something else has the READ lock that
>>>>> thread waits until the other thread releases the READ lock as far as I
>>>>> understand things.
>>>>
>>>> Yes, I can see that. It's just that since the original lock is recursive
>>>> I expected things to get recursive and thus I've been thinking how would
>>>> this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
>>>> places where lock promoting is used are kind of safe since all the
>>>> locking is done at the first level.
>>>>
>>>
>>> The reason I chose rwlocks and taking a read lock by default in the
>>> FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
>>> return a write locked object) is because I know there's the potential
>>> for other code to be doing Reads and rwlocks allowed for that without
>>> the need to create any sort of recursive lockable object or any sort of
>>> API to perform "trylock"'s (all things I tried until the epiphany to use
>>> rwlocks was reached when you added them for other drivers and their list
>>> protection).
>>>
>>> In the long/short run I figured that having a common way to get the lock
>>> and then deciding to change it from Read to Write as necessary would
>>> work just fine for the purpose that was needed.
>>
>> This is where I disagree. I don't think it's safe to promote a lock by
>> unlocking it. The moment the lock is unlocked another thread will lock
>> the object and all the assumptions we made/checked about the object we
>> made when we had the object locked are gone. So we would need to
>> reiterate the decision making process.
>>
> 
> OK - so let's think about where/why/when we promote the lock and what
> the checks/balances/consequences are. 

I know. I am saying that all along that with the current situation it is
safe to have lock promoting. But in general (and possibly to save future
us from nasty deadlocks) it is not. And as I say in comment for 2/3 this
code is guarded by nwfilter driver lock anyway. So there is no race
possible anyway. However, I think I have some patches ready that
implement what is needed here. I'm gonna post them shortly and give you
all the credit - they are heavily based on your work.

Michal