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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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,

> 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@xxxxxxxxxxxxx>
> ---
> 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.

> {
>     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 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.

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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list