[libvirt] [PATCH] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix

[Maxim Nestratov]

26.01.2016 12:29, Martin Kletzander пишет:
> On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
>> A pretty nasty deadlock occurs while trying to rename a VM in parallel
>> with virDomainObjListNumOfDomains.
>> The short description of the problem is as follows:
>>
>> Thread #1:
>>
>> qemuDomainRename:
>>    ------> aquires domain lock by qemuDomObjFromDomain
>>       ---------> waits for domain list lock in any of the listed 
>> functions:
>>          - virDomainObjListFindByName
>>          - virDomainObjListRenameAddNew
>>          - virDomainObjListRenameRemove
>>
>> Thread #2:
>>
>> virDomainObjListNumOfDomains:
>>    ------> aquires domain list lock
>>       ---------> waits for domain lock in virDomainObjListCount
>>
>
> Ouch, the rename API should hold the list lock all the time it's doing
> something and not acquire it when any domain is locked.
>
> But you are duplicating a lot of code,
Not that much

>
>> The patch establishes a single order of taking locks: driver->domains 
>> list
>> first, then a particular VM. This is done by implementing a set of
>> virDomainObjListXxxLocked functions working with driver->domains that 
>> assume
>> that list lock is already aquired by calling functions.
>>
>> Signed-off-by: Maxim Nestratov <mnestratov at virtuozzo.com>
>> ---
>> src/conf/virdomainobjlist.c | 74 
>> +++++++++++++++++++++++++++++++++++++--------
>> src/conf/virdomainobjlist.h |  9 ++++++
>> src/libvirt_private.syms    |  4 +++
>> src/qemu/qemu_driver.c      | 40 +++++++++++++++++++++---
>> 4 files changed, 110 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 7568f93..89e28a8 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -132,21 +132,19 @@ virDomainObjPtr 
>> virDomainObjListFindByID(virDomainObjListPtr doms,
>>
>>
>> static virDomainObjPtr
>> -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>> +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
>>                                    const unsigned char *uuid,
>>                                    bool ref)
>
> Indentation is off for renamed functions.

Ok. I'll fix it if we decide to go on with my approach.
>
>> {
>>     char uuidstr[VIR_UUID_STRING_BUFLEN];
>>     virDomainObjPtr obj;
>>
>> -    virObjectLock(doms);
>>     virUUIDFormat(uuid, uuidstr);
>>
>>     obj = virHashLookup(doms->objs, uuidstr);
>> -    if (ref) {
>> +    if (ref)
>>         virObjectRef(obj);
>> -        virObjectUnlock(doms);
>> -    }
>> +
>>     if (obj) {
>>         virObjectLock(obj);
>>         if (obj->removing) {
>> @@ -156,8 +154,19 @@ 
>> virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>>             obj = NULL;
>>         }
>>     }
>> -    if (!ref)
>> -        virObjectUnlock(doms);
>> +    return obj;
>> +}
>> +
>> +static virDomainObjPtr
>> +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>> +                                   const unsigned char *uuid,
>> +                                   bool ref)
>> +{
>> +    virDomainObjPtr obj;
>> +
>> +    virObjectLock(doms);
>> +    obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
>> +    virObjectUnlock(doms);
>
> This won't work for ref == true as in that case the unlock is not the
> last thing done in virDomainObjListFindByUUIDInternalLocked().
I still think that this won't cause any problem as far as list lock is 
aquired.
Otherwise we would have seen them in virDomainObjListNumOfDomains

>
> I would rather we didn't invent bunch of new functions whose names
> aren't readable, but mayb even preferred open-coding some of them (or
> their parts) in the rename itself.  The reasoning behind it is that
> rename is special API and it touches code that's not exposed normally.
> Or the other way would be moving parts of the rename code into this file
> to make it available for other drivers as well.
I don't insist on the way I'm proposing, let the patch be the reference 
of the problem.

>
> Adding Michal to Cc as the original idea-haver =)  to see if we can
> agree on that or I am mistaken and your approach is the way to go.
>
> Martin