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