[PATCH] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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)
 {
     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);
     return obj;
 }
 
@@ -169,6 +178,13 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
     return virDomainObjListFindByUUIDInternal(doms, uuid, false);
 }
 
+virDomainObjPtr
+virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms,
+                              const unsigned char *uuid)
+{
+    return virDomainObjListFindByUUIDInternalLocked(doms, uuid, true);
+}
+
 
 virDomainObjPtr
 virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
@@ -177,6 +193,23 @@ virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
     return virDomainObjListFindByUUIDInternal(doms, uuid, true);
 }
 
+virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+                                                 const char *name)
+{
+    virDomainObjPtr obj;
+
+    obj = virHashLookup(doms->objsName, name);
+    virObjectRef(obj);
+    if (obj) {
+        virObjectLock(obj);
+        if (obj->removing) {
+            virObjectUnlock(obj);
+            virObjectUnref(obj);
+            obj = NULL;
+        }
+    }
+    return obj;
+}
 
 virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
                                            const char *name)
@@ -312,14 +345,12 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
     return ret;
 }
 
-
 int
-virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms,
                              virDomainObjPtr vm,
                              const char *name)
 {
     int ret = -1;
-    virObjectLock(doms);
 
     /* Add new name into the hash table of domain names. */
     if (virHashAddEntry(doms->objsName, name, vm) < 0)
@@ -332,21 +363,38 @@ virDomainObjListRenameAddNew(virDomainObjListPtr doms,
 
     ret = 0;
  cleanup:
+    return ret;
+}
+
+int
+virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+                             virDomainObjPtr vm,
+                             const char *name)
+{
+    int ret = -1;
+    virObjectLock(doms);
+    ret = virDomainObjListRenameAddNewLocked(doms, vm, name);
     virObjectUnlock(doms);
     return ret;
 }
 
+int
+virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms, const char *name)
+{
+    virHashRemoveEntry(doms->objsName, name);
+    return 0;
+}
 
 int
 virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name)
 {
+    int ret;
     virObjectLock(doms);
-    virHashRemoveEntry(doms->objsName, name);
+    ret = virDomainObjListRenameRemoveLocked(doms, name);
     virObjectUnlock(doms);
-    return 0;
+    return ret;
 }
 
-
 /*
  * The caller must hold a lock on the driver owning 'doms',
  * and must also have locked 'dom', to ensure no one else
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index f479598..bca31cf 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -38,8 +38,12 @@ virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms,
                                            const unsigned char *uuid);
 virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
                                               const unsigned char *uuid);
+virDomainObjPtr virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms,
+                                              const unsigned char *uuid);
 virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
                                            const char *name);
+virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+                                                 const char *name);
 
 enum {
     VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
@@ -54,8 +58,13 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
 int virDomainObjListRenameAddNew(virDomainObjListPtr doms,
                                  virDomainObjPtr vm,
                                  const char *name);
+int virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms,
+                                 virDomainObjPtr vm,
+                                 const char *name);
 int virDomainObjListRenameRemove(virDomainObjListPtr doms,
                                  const char *name);
+int virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms,
+                                 const char *name);
 void virDomainObjListRemove(virDomainObjListPtr doms,
                             virDomainObjPtr dom);
 void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5e05a98..b05d6cc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -881,8 +881,10 @@ virDomainObjListConvert;
 virDomainObjListExport;
 virDomainObjListFindByID;
 virDomainObjListFindByName;
+virDomainObjListFindByNameLocked;
 virDomainObjListFindByUUID;
 virDomainObjListFindByUUIDRef;
+virDomainObjListFindByUUIDRefLocked;
 virDomainObjListForEach;
 virDomainObjListGetActiveIDs;
 virDomainObjListGetInactiveNames;
@@ -892,7 +894,9 @@ virDomainObjListNumOfDomains;
 virDomainObjListRemove;
 virDomainObjListRemoveLocked;
 virDomainObjListRenameAddNew;
+virDomainObjListRenameAddNewLocked;
 virDomainObjListRenameRemove;
+virDomainObjListRenameRemoveLocked;
 
 
 # cpu/cpu.h
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);
+    if (!(vm = qemuDomObjFromDomainLocked(dom)))
         goto cleanup;
 
     if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0)
@@ -19938,7 +19969,7 @@ static int qemuDomainRename(virDomainPtr dom,
      * internal error.  And since new_name != name here, there's no
      * deadlock imminent.
      */
-    tmp_dom = virDomainObjListFindByName(driver->domains, new_name);
+    tmp_dom = virDomainObjListFindByNameLocked(driver->domains, new_name);
     if (tmp_dom) {
         virObjectUnlock(tmp_dom);
         virObjectUnref(tmp_dom);
@@ -19956,7 +19987,7 @@ static int qemuDomainRename(virDomainPtr dom,
         goto endjob;
     }
 
-    if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0)
+    if (virDomainObjListRenameAddNewLocked(driver->domains, vm, new_name) < 0)
         goto endjob;
 
     event_old = virDomainEventLifecycleNewFromObj(vm,
@@ -19980,7 +20011,7 @@ static int qemuDomainRename(virDomainPtr dom,
     }
 
     /* Remove old domain name from table. */
-    virDomainObjListRenameRemove(driver->domains, old_dom_name);
+    virDomainObjListRenameRemoveLocked(driver->domains, old_dom_name);
 
     event_new = virDomainEventLifecycleNewFromObj(vm,
                                               VIR_DOMAIN_EVENT_DEFINED,
@@ -20000,6 +20031,7 @@ static int qemuDomainRename(virDomainPtr dom,
     qemuDomainEventQueue(driver, event_old);
     qemuDomainEventQueue(driver, event_new);
     virObjectUnref(cfg);
+    virObjectUnlock(driver->domains);
     return ret;
 
  rollback:
-- 
1.8.3.1

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]