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

[libvirt] [PATCH 1/2] qemu: Turn closeCallbacks into virObjectLockable



To avoid having to hold the qemu driver lock while iterating through
close callbacks and calling them. This fixes a real deadlock when a
domain which is being migrated from another host gets autodestoyed as a
result of broken connection to the other host.
---
 src/qemu/qemu_conf.c      | 155 ++++++++++++++++++++++++++++++----------------
 src/qemu/qemu_conf.h      |  39 ++++++------
 src/qemu/qemu_driver.c    |  12 ++--
 src/qemu/qemu_migration.c |   6 +-
 src/qemu/qemu_process.c   |  10 +--
 5 files changed, 136 insertions(+), 86 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4ff912d..edadf36 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -57,8 +57,25 @@
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
+typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
+typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
+struct _qemuDriverCloseDef {
+    virConnectPtr conn;
+    virQEMUCloseCallback cb;
+};
+
+struct _virQEMUCloseCallbacks {
+    virObjectLockable parent;
+
+    /* UUID string to qemuDriverCloseDef mapping */
+    virHashTablePtr list;
+};
+
+
 static virClassPtr virQEMUDriverConfigClass;
+static virClassPtr virQEMUCloseCallbacksClass;
 static void virQEMUDriverConfigDispose(void *obj);
+static void virQEMUCloseCallbacksDispose(void *obj);
 
 static int virQEMUConfigOnceInit(void)
 {
@@ -71,13 +88,21 @@ static int virQEMUConfigOnceInit(void)
     return 0;
 }
 
-VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
+static int
+virQEMUCloseCallbacksOnceInit(void)
+{
+    virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(),
+                                             "virQEMUCloseCallbacks",
+                                             sizeof(virQEMUCloseCallbacks),
+                                             virQEMUCloseCallbacksDispose);
+    if (!virQEMUCloseCallbacksClass)
+        return -1;
+    return 0;
+}
 
+VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
+VIR_ONCE_GLOBAL_INIT(virQEMUCloseCallbacks)
 
-struct _qemuDriverCloseDef {
-    virConnectPtr conn;
-    qemuDriverCloseCallback cb;
-};
 
 static void
 qemuDriverLock(virQEMUDriverPtr driver)
@@ -639,44 +664,59 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
 
 
 static void
-qemuDriverCloseCallbackFree(void *payload,
-                            const void *name ATTRIBUTE_UNUSED)
+virQEMUCloseCallbacksFreeData(void *payload,
+                              const void *name ATTRIBUTE_UNUSED)
 {
     VIR_FREE(payload);
 }
 
-int
-qemuDriverCloseCallbackInit(virQEMUDriverPtr driver)
+virQEMUCloseCallbacksPtr
+virQEMUCloseCallbacksNew(void)
 {
-    driver->closeCallbacks = virHashCreate(5, qemuDriverCloseCallbackFree);
-    if (!driver->closeCallbacks)
-        return -1;
+    virQEMUCloseCallbacksPtr closeCallbacks;
 
-    return 0;
+    if (virQEMUCloseCallbacksInitialize() < 0)
+        return NULL;
+
+    if (!(closeCallbacks = virObjectLockableNew(virQEMUCloseCallbacksClass)))
+        return NULL;
+
+    closeCallbacks->list = virHashCreate(5, virQEMUCloseCallbacksFreeData);
+    if (!closeCallbacks->list) {
+        virObjectUnref(closeCallbacks);
+        return NULL;
+    }
+
+    return closeCallbacks;
 }
 
-void
-qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver)
+static void
+virQEMUCloseCallbacksDispose(void *obj)
 {
-    virHashFree(driver->closeCallbacks);
+    virQEMUCloseCallbacksPtr closeCallbacks = obj;
+
+    virHashFree(closeCallbacks->list);
 }
 
 int
-qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
-                           virDomainObjPtr vm,
-                           virConnectPtr conn,
-                           qemuDriverCloseCallback cb)
+virQEMUCloseCallbacksSet(virQEMUDriverPtr driver,
+                         virDomainObjPtr vm,
+                         virConnectPtr conn,
+                         virQEMUCloseCallback cb)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
+    virHashTablePtr list;
     int ret = -1;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p",
               vm->def->name, uuidstr, conn, cb);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(driver->closeCallbacks);
+
+    list = driver->closeCallbacks->list;
+    closeDef = virHashLookup(list, uuidstr);
     if (closeDef) {
         if (closeDef->conn != conn) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -701,7 +741,7 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
 
         closeDef->conn = conn;
         closeDef->cb = cb;
-        if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) {
+        if (virHashAddEntry(list, uuidstr, closeDef) < 0) {
             VIR_FREE(closeDef);
             goto cleanup;
         }
@@ -709,25 +749,28 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
 
     ret = 0;
 cleanup:
-    qemuDriverUnlock(driver);
+    virObjectUnlock(driver->closeCallbacks);
     return ret;
 }
 
 int
-qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
-                             virDomainObjPtr vm,
-                             qemuDriverCloseCallback cb)
+virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
+                           virDomainObjPtr vm,
+                           virQEMUCloseCallback cb)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
+    virHashTablePtr list;
     int ret = -1;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, cb=%p",
               vm->def->name, uuidstr, cb);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(driver->closeCallbacks);
+
+    list = driver->closeCallbacks->list;
+    closeDef = virHashLookup(list, uuidstr);
     if (!closeDef)
         goto cleanup;
 
@@ -738,46 +781,49 @@ qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    ret = virHashRemoveEntry(driver->closeCallbacks, uuidstr);
+    ret = virHashRemoveEntry(list, uuidstr);
 cleanup:
-    qemuDriverUnlock(driver);
+    virObjectUnlock(driver->closeCallbacks);
     return ret;
 }
 
-qemuDriverCloseCallback
-qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
-                           virDomainObjPtr vm,
-                           virConnectPtr conn)
+virQEMUCloseCallback
+virQEMUCloseCallbacksGet(virQEMUDriverPtr driver,
+                         virDomainObjPtr vm,
+                         virConnectPtr conn)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
-    qemuDriverCloseCallback cb = NULL;
+    virQEMUCloseCallback cb = NULL;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, conn=%p",
               vm->def->name, uuidstr, conn);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(driver->closeCallbacks);
+
+    closeDef = virHashLookup(driver->closeCallbacks->list, uuidstr);
     if (closeDef && (!conn || closeDef->conn == conn))
         cb = closeDef->cb;
 
+    virObjectUnlock(driver->closeCallbacks);
+
     VIR_DEBUG("cb=%p", cb);
-    qemuDriverUnlock(driver);
     return cb;
 }
 
-struct qemuDriverCloseCallbackData {
+struct virQEMUCloseCallbacksData {
+    virHashTablePtr list;
     virQEMUDriverPtr driver;
     virConnectPtr conn;
 };
 
 static void
-qemuDriverCloseCallbackRun(void *payload,
-                           const void *name,
-                           void *opaque)
+virQEMUCloseCallbacksRunOne(void *payload,
+                            const void *name,
+                            void *opaque)
 {
-    struct qemuDriverCloseCallbackData *data = opaque;
+    struct virQEMUCloseCallbacksData *data = opaque;
     qemuDriverCloseDefPtr closeDef = payload;
     unsigned char uuid[VIR_UUID_BUFLEN];
     char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -808,20 +854,25 @@ qemuDriverCloseCallbackRun(void *payload,
     if (dom)
         virObjectUnlock(dom);
 
-    virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);
+    virHashRemoveEntry(data->list, uuidstr);
 }
 
 void
-qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
-                              virConnectPtr conn)
+virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
+                         virConnectPtr conn)
 {
-    struct qemuDriverCloseCallbackData data = {
-        driver, conn
+    struct virQEMUCloseCallbacksData data = {
+        .driver = driver,
+        .conn = conn
     };
+
     VIR_DEBUG("conn=%p", conn);
-    qemuDriverLock(driver);
-    virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data);
-    qemuDriverUnlock(driver);
+    virObjectLock(driver->closeCallbacks);
+
+    data.list = driver->closeCallbacks->list;
+    virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data);
+
+    virObjectUnlock(driver->closeCallbacks);
 }
 
 /* Construct the hash key for sharedDisks as "major:minor" */
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 14f1427..72380dc 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -47,8 +47,8 @@
 
 # define QEMUD_CPUMASK_LEN CPU_SETSIZE
 
-typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
-typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
+typedef struct _virQEMUCloseCallbacks virQEMUCloseCallbacks;
+typedef virQEMUCloseCallbacks *virQEMUCloseCallbacksPtr;
 
 typedef struct _virQEMUDriver virQEMUDriver;
 typedef virQEMUDriver *virQEMUDriverPtr;
@@ -216,8 +216,8 @@ struct _virQEMUDriver {
     /* Immutable pointer. lockless access */
     virLockManagerPluginPtr lockManager;
 
-    /* Immutable pointer. Unsafe APIs. XXX */
-    virHashTablePtr closeCallbacks;
+    /* Immutable pointer, self-clocking APIs */
+    virQEMUCloseCallbacksPtr closeCallbacks;
 };
 
 typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
@@ -254,23 +254,22 @@ struct qemuDomainDiskInfo {
     int io_status;
 };
 
-typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver,
-                                                   virDomainObjPtr vm,
-                                                   virConnectPtr conn);
-int qemuDriverCloseCallbackInit(virQEMUDriverPtr driver);
-void qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver);
-int qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
+typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver,
+                                                virDomainObjPtr vm,
+                                                virConnectPtr conn);
+virQEMUCloseCallbacksPtr virQEMUCloseCallbacksNew(void);
+int virQEMUCloseCallbacksSet(virQEMUDriverPtr driver,
+                             virDomainObjPtr vm,
+                             virConnectPtr conn,
+                             virQEMUCloseCallback cb);
+int virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
                                virDomainObjPtr vm,
-                               virConnectPtr conn,
-                               qemuDriverCloseCallback cb);
-int qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
-                                 virDomainObjPtr vm,
-                                 qemuDriverCloseCallback cb);
-qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
-                                                   virDomainObjPtr vm,
-                                                   virConnectPtr conn);
-void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
-                                   virConnectPtr conn);
+                               virQEMUCloseCallback cb);
+virQEMUCloseCallback virQEMUCloseCallbacksGet(virQEMUDriverPtr driver,
+                                              virDomainObjPtr vm,
+                                              virConnectPtr conn);
+void virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
+                              virConnectPtr conn);
 
 int qemuAddSharedDisk(virQEMUDriverPtr driver,
                       const char *disk_path)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 23499ef..257728d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -753,7 +753,7 @@ qemuStartup(bool privileged,
         cfg->hugepagePath = mempath;
     }
 
-    if (qemuDriverCloseCallbackInit(qemu_driver) < 0)
+    if (!(qemu_driver->closeCallbacks = virQEMUCloseCallbacksNew()))
         goto error;
 
     /* Get all the running persistent or transient configs first */
@@ -954,7 +954,7 @@ qemuShutdown(void) {
 
     virSysinfoDefFree(qemu_driver->hostsysinfo);
 
-    qemuDriverCloseCallbackShutdown(qemu_driver);
+    virObjectUnref(qemu_driver->closeCallbacks);
 
     VIR_FREE(qemu_driver->qemuImgBinary);
 
@@ -1052,7 +1052,7 @@ static int qemuClose(virConnectPtr conn) {
     virQEMUDriverPtr driver = conn->privateData;
 
     /* Get rid of callbacks registered for this conn */
-    qemuDriverCloseCallbackRunAll(driver, conn);
+    virQEMUCloseCallbacksRun(driver, conn);
 
     conn->privateData = NULL;
 
@@ -9626,8 +9626,8 @@ qemuDomainMigrateBegin3(virDomainPtr domain,
          * This prevents any other APIs being invoked while migration is taking
          * place.
          */
-        if (qemuDriverCloseCallbackSet(driver, vm, domain->conn,
-                                       qemuMigrationCleanup) < 0)
+        if (virQEMUCloseCallbacksSet(driver, vm, domain->conn,
+                                     qemuMigrationCleanup) < 0)
             goto endjob;
         if (qemuMigrationJobContinue(vm) == 0) {
             vm = NULL;
@@ -9853,7 +9853,7 @@ qemuDomainMigrateConfirm3(virDomainPtr domain,
         phase = QEMU_MIGRATION_PHASE_CONFIRM3;
 
     qemuMigrationJobStartPhase(driver, vm, phase);
-    qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+    virQEMUCloseCallbacksUnset(driver, vm, qemuMigrationCleanup);
 
     ret = qemuMigrationConfirm(driver, domain->conn, vm,
                                cookiein, cookieinlen,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 36e55d2..11d7d6c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3155,7 +3155,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
     }
 
     qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3);
-    qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+    virQEMUCloseCallbacksUnset(driver, vm, qemuMigrationCleanup);
 
     resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
     ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen,
@@ -3186,8 +3186,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
 
     qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE);
 
-    if (qemuDriverCloseCallbackSet(driver, vm, conn,
-                                   qemuMigrationCleanup) < 0)
+    if (virQEMUCloseCallbacksSet(driver, vm, conn,
+                                 qemuMigrationCleanup) < 0)
         goto endjob;
 
 endjob:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4251c34..f987618 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4670,22 +4670,22 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
                               virConnectPtr conn)
 {
     VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
-    return qemuDriverCloseCallbackSet(driver, vm, conn,
-                                      qemuProcessAutoDestroy);
+    return virQEMUCloseCallbacksSet(driver, vm, conn,
+                                    qemuProcessAutoDestroy);
 }
 
 int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver,
                                  virDomainObjPtr vm)
 {
     VIR_DEBUG("vm=%s", vm->def->name);
-    return qemuDriverCloseCallbackUnset(driver, vm, qemuProcessAutoDestroy);
+    return virQEMUCloseCallbacksUnset(driver, vm, qemuProcessAutoDestroy);
 }
 
 bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver,
                                   virDomainObjPtr vm)
 {
-    qemuDriverCloseCallback cb;
+    virQEMUCloseCallback cb;
     VIR_DEBUG("vm=%s", vm->def->name);
-    cb = qemuDriverCloseCallbackGet(driver, vm, NULL);
+    cb = virQEMUCloseCallbacksGet(driver, vm, NULL);
     return cb == qemuProcessAutoDestroy;
 }
-- 
1.8.1.2


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