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

[Michal Privoznik]

On 25.01.2016 10:16, 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
> 
> 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e46404b..b012516 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -206,6 +206,36 @@ struct qemuAutostartData {
>      virConnectPtr conn;
>  };
>  
> +/**
> + * qemuDomObjFromDomainLocked:
> + * @domain: Domain pointer that has to be looked up
> + *
> + * This function looks up @domain and returns the appropriate virDomainObjPtr
> + * that has to be released by calling virDomainObjEndAPI().
> + * It assumes that driver->domains list is locked already, thus it uses Locked
> + * variant of virDomainObjListFindByUUIDRef function.
> + *
> + * Returns the domain object with incremented reference counter which is locked
> + * on success, NULL otherwise.
> + */
> +static virDomainObjPtr
> +qemuDomObjFromDomainLocked(virDomainPtr domain)
> +{
> +    virDomainObjPtr vm;
> +    virQEMUDriverPtr driver = domain->conn->privateData;
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +    vm = virDomainObjListFindByUUIDRefLocked(driver->domains, domain->uuid);
> +    if (!vm) {
> +        virUUIDFormat(domain->uuid, uuidstr);
> +        virReportError(VIR_ERR_NO_DOMAIN,
> +                       _("no domain with matching uuid '%s' (%s)"),
> +                       uuidstr, domain->name);
> +        return NULL;
> +    }
> +
> +    return vm;
> +}
>  
>  /**
>   * qemuDomObjFromDomain:
> @@ -19892,7 +19922,8 @@ static int qemuDomainRename(virDomainPtr dom,
>  
>      virCheckFlags(0, ret);
>  
> -    if (!(vm = qemuDomObjFromDomain(dom)))
> +    virObjectLock(driver->domains);

This is rather ugly. While driver->domains is a lockable object, it's
internals are intentionally hidden from the rest of the code so that
nobody from outside should touch them. That's why we have locking APIs
over virDomainObjList object. I know this will work, I just find it hackish.

However, I find your approach understandable and kind of agree with it.
What we should do is:

1) lock list
2) lookup domain
3) begin MODIFY job on the domain (*)

4) rename the domain (here are hidden all the checks, moving the XML
file, etc. - basically everything from the qemuDomainRename())

5) end job
6) unlock domain
7) unlock list

* - here we will need BeginJob() without timeout - we don't want to keep
the list locked longer than needed. Either setting the job is done
instantly or not at all.


What if, instead of introducing bunch of Locked() functions we introduce
a new internal API to virDomainObjList that will look like this:

int virDomainObjListRename(virDomainObjListPtr domains,
                           virDomainObjPtr dom,
                           const char *new_name,
                           int (*driverRename)(virDomainObjPtr, ...),
                           int (*rollBack)(virDomainObjPtr, ...));

This function will encapsulate the renaming and at the correct spot it
will call driverRename() so that driver can adjust its internal state.
If the driver is successful, just remove the old entry. If it is not,
call rollBack() callback which will cancel partially performed operation
and restore original state of the driver.
Moreover, in the virDomainObjListRename() we can ensure the locking
order and we don't need to introduce new BeginJob() API with no timeout.

Michal