Re: [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)

@@ -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)