The deadlock was introduced by removing global QEMU driver lock because
nwfilter was counting on this lock and ensure that all driver locks are
locked inside of nwfilter{Define,Undefine}.
This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
the deadlock for all possible paths in QEMU driver. LXC and UML drivers
still have global lock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143780
Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
---
This is temporary fix for the deadlock issue as I'm planning to create global
libvirt jobs (similar to QEMU domain jobs) and use it for other drivers and
for example for nwfilters too.
src/qemu/qemu_driver.c | 12 ++++++++++++
src/qemu/qemu_migration.c | 3 +++
src/qemu/qemu_process.c | 4 ++++
3 files changed, 19 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6acaea8..9e6f505 100644
[1] Usage around virQEMUDriverGetConfig() calls isn't consistent. In
particular qemuDomainAttachDeviceFlags() will lock/unlock in different
order.
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5937,6 +5937,8 @@ qemuDomainRestoreFlags(virConnectPtr conn,
def = tmp;
}
+ virNWFilterReadLockFilterUpdates();
+
So no other locks up to this point are taken? And no need perhaps to
lock earlier to play follow the leader (eg, the original commit) where
the lock was taken after the various flags checks.
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
@@ -5978,6 +5980,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
virFileWrapperFdFree(wrapperFd);
if (vm)
virObjectUnlock(vm);
+ virNWFilterUnlockFilterUpdates();
So this could get called without ever having set the lock - is that
correct? We can get to cleanup without calling the ReadLock - probably
not a good thing since it's indeterminate what happens according to the
man page.
return ret;
}
@@ -7502,6 +7505,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+ virNWFilterReadLockFilterUpdates();
+
[1] This one is done after the GetConfig
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -7614,6 +7619,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
virObjectUnlock(vm);
virObjectUnref(caps);
virObjectUnref(cfg);
[1] But the release is done after the cfg unref
+ virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -7644,6 +7650,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
VIR_DOMAIN_AFFECT_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+ virNWFilterReadLockFilterUpdates();
+
[1] This one is done before the GetConfig
cfg = virQEMUDriverGetConfig(driver);
affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
@@ -7760,6 +7768,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
virObjectUnlock(vm);
virObjectUnref(caps);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
[1] Release done after cfg unref
return ret;
}
@@ -14510,6 +14519,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
* and use of FORCE can cause multiple transitions.
*/
+ virNWFilterReadLockFilterUpdates();
+
[1] This one is done before GetConfig
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
return -1;
@@ -14831,6 +14842,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virObjectUnlock(vm);
virObjectUnref(caps);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
[1] Release done after cfg unref
return ret;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 94a4cf6..18242ae 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2666,6 +2666,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
goto cleanup;
}
+ virNWFilterReadLockFilterUpdates();
+
Lots of code before we get here - what's the locking
if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
@@ -2825,6 +2827,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
qemuDomainEventQueue(driver, event);
qemuMigrationCookieFree(mig);
virObjectUnref(caps);
+ virNWFilterUnlockFilterUpdates();
Yet another case where we could reach the cleanup without having ever
obtained the ReadLock
return ret;
stop:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 26d4948..409a672 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
This one seems OK - although I think I'd make it a separate patch and of
course explain the lock ordering change
John
@@ -3422,6 +3422,8 @@ qemuProcessReconnect(void *opaque)
VIR_FREE(data);
+ virNWFilterReadLockFilterUpdates();
+
virObjectLock(obj);
cfg = virQEMUDriverGetConfig(driver);
@@ -3573,6 +3575,7 @@ qemuProcessReconnect(void *opaque)
virObjectUnref(conn);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
return;
@@ -3608,6 +3611,7 @@ qemuProcessReconnect(void *opaque)
}
virObjectUnref(conn);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
}
static int