[libvirt] [PATCH 0/2] qemu: Fix deadlocks in autodestroy
After the recent removal of qemu driver locks, implicit driver locks were introduced in places that previously relied on being called from functions that alread had the driver locked. Thus calling anything complex (such as close callbacks) must not be called when the driver is locked... Jiri Denemark (2): qemu: Turn closeCallbacks into virObjectLockable qemu: Avoid deadlock in autodestroy src/qemu/qemu_conf.c | 172 ++++++++++++++++++++++++++++------------------ src/qemu/qemu_conf.h | 43 ++++++------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 12 ++-- src/qemu/qemu_migration.c | 6 +- src/qemu/qemu_process.c | 20 ++++-- 6 files changed, 153 insertions(+), 101 deletions(-) -- 1.8.1.2
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
On 18.02.2013 17:07, Jiri Denemark wrote:
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(-)
ACK Michal
On Mon, Feb 18, 2013 at 05:07:44PM +0100, Jiri Denemark wrote:
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.
Since you're turning this into a full object, I think it would be nice to move it to a standalone file, so it can be reused by the LXC driver, which currently re-invents this same kind of code.
--- 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)
No need for two initializers, just make the virQEMUConfigOnceInit method create both classes.
@@ -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,
I was rather expecting to see the virQEMUDriverPtr replacd with a virQEMUCloseCallbacksPtr in all the methods now it becomes a full object.
+ 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)
ACK to the general approach, but I think there's a few tweaks needed still. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Feb 19, 2013 at 14:26:37 +0000, Daniel P. Berrange wrote:
On Mon, Feb 18, 2013 at 05:07:44PM +0100, Jiri Denemark wrote:
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.
Since you're turning this into a full object, I think it would be nice to move it to a standalone file, so it can be reused by the LXC driver, which currently re-invents this same kind of code.
Good idea, could it be done by a follow-up patch? What do you think would be the best name for such a shared object? I guess something like virConnectCloseCallbacks could make sense...
-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)
No need for two initializers, just make the virQEMUConfigOnceInit method create both classes.
OK, they will need to be separated again once this is decoupled from qemu driver but that's fine.
-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,
I was rather expecting to see the virQEMUDriverPtr replacd with a virQEMUCloseCallbacksPtr in all the methods now it becomes a full object.
I guess I was just lazy and didn't want to change callers too much :-) I would need to do that when moving the code out of qemu driver anyway, though.
ACK to the general approach, but I think there's a few tweaks needed still.
OK, v2 is coming soon. Jirka
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. --- Notes: Version 2: - merge initializers - replace virQEMUDriverPtr with virQEMUCloseCallbacksPtr in all virQEMUCloseCallbacks* APIs src/qemu/qemu_conf.c | 158 +++++++++++++++++++++++++++++----------------- src/qemu/qemu_conf.h | 41 ++++++------ src/qemu/qemu_driver.c | 16 +++-- src/qemu/qemu_migration.c | 7 +- src/qemu/qemu_process.c | 11 ++-- 5 files changed, 140 insertions(+), 93 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4ff912d..f3c3cf3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -57,28 +57,47 @@ #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) { - if (!(virQEMUDriverConfigClass = virClassNew(virClassForObject(), - "virQEMUDriverConfig", - sizeof(virQEMUDriverConfig), - virQEMUDriverConfigDispose))) - return -1; + virQEMUDriverConfigClass = virClassNew(virClassForObject(), + "virQEMUDriverConfig", + sizeof(virQEMUDriverConfig), + virQEMUDriverConfigDispose); - return 0; + virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(), + "virQEMUCloseCallbacks", + sizeof(virQEMUCloseCallbacks), + virQEMUCloseCallbacksDispose); + + if (!virQEMUDriverConfigClass || !virQEMUCloseCallbacksClass) + return -1; + else + return 0; } VIR_ONCE_GLOBAL_INIT(virQEMUConfig) -struct _qemuDriverCloseDef { - virConnectPtr conn; - qemuDriverCloseCallback cb; -}; - static void qemuDriverLock(virQEMUDriverPtr driver) { @@ -639,44 +658,57 @@ 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 (virQEMUConfigInitialize() < 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(virQEMUCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn, + virQEMUCloseCallback cb) { char uuidstr[VIR_UUID_STRING_BUFLEN]; qemuDriverCloseDefPtr closeDef; 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(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); if (closeDef) { if (closeDef->conn != conn) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -701,7 +733,7 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver, closeDef->conn = conn; closeDef->cb = cb; - if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) { + if (virHashAddEntry(closeCallbacks->list, uuidstr, closeDef) < 0) { VIR_FREE(closeDef); goto cleanup; } @@ -709,25 +741,26 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver, ret = 0; cleanup: - qemuDriverUnlock(driver); + virObjectUnlock(closeCallbacks); return ret; } int -qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver, - virDomainObjPtr vm, - qemuDriverCloseCallback cb) +virQEMUCloseCallbacksUnset(virQEMUCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virQEMUCloseCallback cb) { char uuidstr[VIR_UUID_STRING_BUFLEN]; qemuDriverCloseDefPtr closeDef; 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(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); if (!closeDef) goto cleanup; @@ -738,46 +771,49 @@ qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver, goto cleanup; } - ret = virHashRemoveEntry(driver->closeCallbacks, uuidstr); + ret = virHashRemoveEntry(closeCallbacks->list, uuidstr); cleanup: - qemuDriverUnlock(driver); + virObjectUnlock(closeCallbacks); return ret; } -qemuDriverCloseCallback -qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virConnectPtr conn) +virQEMUCloseCallback +virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks, + 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(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); if (closeDef && (!conn || closeDef->conn == conn)) cb = closeDef->cb; + virObjectUnlock(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 +844,26 @@ qemuDriverCloseCallbackRun(void *payload, if (dom) virObjectUnlock(dom); - virHashRemoveEntry(data->driver->closeCallbacks, uuidstr); + virHashRemoveEntry(data->list, uuidstr); } void -qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, - virConnectPtr conn) +virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, + virConnectPtr conn, + virQEMUDriverPtr driver) { - 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(closeCallbacks); + + data.list = closeCallbacks->list; + virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data); + + virObjectUnlock(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 ea4f393..c6bc883 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,24 @@ 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(virQEMUCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn, + virQEMUCloseCallback cb); +int virQEMUCloseCallbacksUnset(virQEMUCloseCallbacksPtr closeCallbacks, 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(virQEMUCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn); +void virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, + virConnectPtr conn, + virQEMUDriverPtr driver); int qemuAddSharedDisk(virQEMUDriverPtr driver, const char *disk_path) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dc35b91..76fbb51 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -757,7 +757,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 */ @@ -958,7 +958,7 @@ qemuShutdown(void) { virSysinfoDefFree(qemu_driver->hostsysinfo); - qemuDriverCloseCallbackShutdown(qemu_driver); + virObjectUnref(qemu_driver->closeCallbacks); VIR_FREE(qemu_driver->qemuImgBinary); @@ -1052,11 +1052,12 @@ cleanup: return ret; } -static int qemuClose(virConnectPtr conn) { +static int qemuClose(virConnectPtr conn) +{ virQEMUDriverPtr driver = conn->privateData; /* Get rid of callbacks registered for this conn */ - qemuDriverCloseCallbackRunAll(driver, conn); + virQEMUCloseCallbacksRun(driver->closeCallbacks, conn, driver); conn->privateData = NULL; @@ -9630,8 +9631,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->closeCallbacks, vm, domain->conn, + qemuMigrationCleanup) < 0) goto endjob; if (qemuMigrationJobContinue(vm) == 0) { vm = NULL; @@ -9857,7 +9858,8 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, phase = QEMU_MIGRATION_PHASE_CONFIRM3; qemuMigrationJobStartPhase(driver, vm, phase); - qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup); + virQEMUCloseCallbacksUnset(driver->closeCallbacks, 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..8485b20 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3155,7 +3155,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, } qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3); - qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup); + virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuMigrationCleanup); resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, @@ -3186,8 +3187,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE); - if (qemuDriverCloseCallbackSet(driver, vm, conn, - qemuMigrationCleanup) < 0) + if (virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn, + qemuMigrationCleanup) < 0) goto endjob; endjob: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 67362af..0fcc14f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4667,22 +4667,23 @@ 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->closeCallbacks, vm, conn, + qemuProcessAutoDestroy); } int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver, virDomainObjPtr vm) { VIR_DEBUG("vm=%s", vm->def->name); - return qemuDriverCloseCallbackUnset(driver, vm, qemuProcessAutoDestroy); + return virQEMUCloseCallbacksUnset(driver->closeCallbacks, 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->closeCallbacks, vm, NULL); return cb == qemuProcessAutoDestroy; } -- 1.8.1.2
On 02/19/2013 02:22 PM, Jiri Denemark wrote:
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. ---
Notes: Version 2: - merge initializers - replace virQEMUDriverPtr with virQEMUCloseCallbacksPtr in all virQEMUCloseCallbacks* APIs
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Since closeCallbacks were turned into virObjectLockable, we can no longer call virQEMUCloseCallbacks APIs from within a registered close callback. --- src/qemu/qemu_conf.c | 19 ++++--------------- src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index edadf36..fc8e152 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -825,8 +825,6 @@ virQEMUCloseCallbacksRunOne(void *payload, { struct virQEMUCloseCallbacksData *data = opaque; qemuDriverCloseDefPtr closeDef = payload; - unsigned char uuid[VIR_UUID_BUFLEN]; - char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom; VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", @@ -835,18 +833,9 @@ virQEMUCloseCallbacksRunOne(void *payload, if (data->conn != closeDef->conn || !closeDef->cb) return; - if (virUUIDParse(name, uuid) < 0) { - VIR_WARN("Failed to parse %s", (const char *)name); - return; - } - /* We need to reformat uuidstr, because closeDef->cb - * might cause the current hash entry to be removed, - * which means 'name' will have been free()d - */ - virUUIDFormat(uuid, uuidstr); - - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) { - VIR_DEBUG("No domain object with UUID %s", uuidstr); + if (!(dom = virDomainObjListFindByUUID(data->driver->domains, name))) { + VIR_DEBUG("No domain object with UUID %s", + (const char *) name); return; } @@ -854,7 +843,7 @@ virQEMUCloseCallbacksRunOne(void *payload, if (dom) virObjectUnlock(dom); - virHashRemoveEntry(data->list, uuidstr); + virHashRemoveEntry(data->list, name); } void diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 72380dc..b5a3281 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -254,6 +254,10 @@ struct qemuDomainDiskInfo { int io_status; }; +/* + * To avoid a certain deadlock this callback must never call any + * virQEMUCloseCallbacks* API. + */ typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 905b099..c9d5f8b 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -137,6 +137,7 @@ struct _qemuDomainObjPrivate { bool gotShutdown; bool beingDestroyed; + bool autoDestroyed; char *pidfile; int nvcpupids; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f987618..74406a6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4263,7 +4263,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainCleanupRun(driver, vm); /* Stop autodestroy in case guest is restarted */ - qemuProcessAutoDestroyRemove(driver, vm); + if (!priv->autoDestroyed) + qemuProcessAutoDestroyRemove(driver, vm); /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { @@ -4647,8 +4648,15 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver, goto cleanup; VIR_DEBUG("Killing domain"); + + /* We need to prevent qemuProcessStop from removing this function from + * closeCallbacks since that would cause a deadlock. + */ + priv->autoDestroyed = true; qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED, VIR_QEMU_PROCESS_STOP_MIGRATED); + priv->autoDestroyed = false; + virDomainAuditStop(dom, "destroyed"); event = virDomainEventNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, -- 1.8.1.2
On 18.02.2013 17:07, Jiri Denemark wrote:
Since closeCallbacks were turned into virObjectLockable, we can no longer call virQEMUCloseCallbacks APIs from within a registered close callback. --- src/qemu/qemu_conf.c | 19 ++++--------------- src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index edadf36..fc8e152 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -825,8 +825,6 @@ virQEMUCloseCallbacksRunOne(void *payload, { struct virQEMUCloseCallbacksData *data = opaque; qemuDriverCloseDefPtr closeDef = payload; - unsigned char uuid[VIR_UUID_BUFLEN]; - char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom;
VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", @@ -835,18 +833,9 @@ virQEMUCloseCallbacksRunOne(void *payload, if (data->conn != closeDef->conn || !closeDef->cb) return;
- if (virUUIDParse(name, uuid) < 0) { - VIR_WARN("Failed to parse %s", (const char *)name); - return; - } - /* We need to reformat uuidstr, because closeDef->cb - * might cause the current hash entry to be removed, - * which means 'name' will have been free()d - */ - virUUIDFormat(uuid, uuidstr); - - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) { - VIR_DEBUG("No domain object with UUID %s", uuidstr); + if (!(dom = virDomainObjListFindByUUID(data->driver->domains, name))) { + VIR_DEBUG("No domain object with UUID %s", + (const char *) name);
No. Please s/name/uuid/ as storing UUID into variable named @name is insane. Or do we want to confuse an enemy? :) I know you haven't introduced it, but since you are touching the body anyway, it's worth changing the parameter name as well.
return; }
@@ -854,7 +843,7 @@ virQEMUCloseCallbacksRunOne(void *payload, if (dom) virObjectUnlock(dom);
- virHashRemoveEntry(data->list, uuidstr); + virHashRemoveEntry(data->list, name); }
ACK if you change the variable name. Michal
On Mon, Feb 18, 2013 at 05:07:45PM +0100, Jiri Denemark wrote:
Since closeCallbacks were turned into virObjectLockable, we can no longer call virQEMUCloseCallbacks APIs from within a registered close callback. --- src/qemu/qemu_conf.c | 19 ++++--------------- src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index edadf36..fc8e152 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -825,8 +825,6 @@ virQEMUCloseCallbacksRunOne(void *payload, { struct virQEMUCloseCallbacksData *data = opaque; qemuDriverCloseDefPtr closeDef = payload; - unsigned char uuid[VIR_UUID_BUFLEN]; - char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom;
VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", @@ -835,18 +833,9 @@ virQEMUCloseCallbacksRunOne(void *payload, if (data->conn != closeDef->conn || !closeDef->cb) return;
- if (virUUIDParse(name, uuid) < 0) { - VIR_WARN("Failed to parse %s", (const char *)name); - return; - } - /* We need to reformat uuidstr, because closeDef->cb - * might cause the current hash entry to be removed, - * which means 'name' will have been free()d - */ - virUUIDFormat(uuid, uuidstr); - - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) { - VIR_DEBUG("No domain object with UUID %s", uuidstr); + if (!(dom = virDomainObjListFindByUUID(data->driver->domains, name))) { + VIR_DEBUG("No domain object with UUID %s", + (const char *) name); return; }
@@ -854,7 +843,7 @@ virQEMUCloseCallbacksRunOne(void *payload, if (dom) virObjectUnlock(dom);
- virHashRemoveEntry(data->list, uuidstr); + virHashRemoveEntry(data->list, name); }
void diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 72380dc..b5a3281 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -254,6 +254,10 @@ struct qemuDomainDiskInfo { int io_status; };
+/* + * To avoid a certain deadlock this callback must never call any + * virQEMUCloseCallbacks* API. + */ typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 905b099..c9d5f8b 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -137,6 +137,7 @@ struct _qemuDomainObjPrivate {
bool gotShutdown; bool beingDestroyed; + bool autoDestroyed; char *pidfile;
int nvcpupids; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f987618..74406a6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4263,7 +4263,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainCleanupRun(driver, vm);
/* Stop autodestroy in case guest is restarted */ - qemuProcessAutoDestroyRemove(driver, vm); + if (!priv->autoDestroyed) + qemuProcessAutoDestroyRemove(driver, vm);
/* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { @@ -4647,8 +4648,15 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver, goto cleanup;
VIR_DEBUG("Killing domain"); + + /* We need to prevent qemuProcessStop from removing this function from + * closeCallbacks since that would cause a deadlock. + */ + priv->autoDestroyed = true; qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED, VIR_QEMU_PROCESS_STOP_MIGRATED); + priv->autoDestroyed = false; + virDomainAuditStop(dom, "destroyed"); event = virDomainEventNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED,
Slightly gross, but it'll do for now. ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Feb 18, 2013 at 17:07:43 +0100, Jiri Denemark wrote:
After the recent removal of qemu driver locks, implicit driver locks were introduced in places that previously relied on being called from functions that alread had the driver locked. Thus calling anything complex (such as close callbacks) must not be called when the driver is locked...
Jiri Denemark (2): qemu: Turn closeCallbacks into virObjectLockable qemu: Avoid deadlock in autodestroy
I pushed this series (with v2 of the first patch). Thanks for the review. Jirka
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Michal Privoznik