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

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.