On 11/12/2014 05:17 PM, John Ferlan wrote:

On 11/12/2014 04:51 AM, Pavel Hrdina wrote:

...

Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine} and starting of guest, but this same deadlock is also for

s/is also/exists/

...

updating/attaching network device to domain.

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@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++++++++ src/qemu/qemu_migration.c | 3 +++ src/qemu/qemu_process.c | 4 ++++ 3 files changed, 19 insertions(+)

I thought I had built these yesterday when reviewing, but apparently not as doing a build just now failed because the symbols are not defined in qemu_migration.c and qemu_process.c (I have gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC) on my f20 box)

I squashed the following and it builds...

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 533bb35..9106800 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -50,6 +50,7 @@ #include "viruuid.h" #include "virtime.h" #include "locking/domain_lock.h" +#include "nwfilter_conf.h" #include "rpc/virnetsocket.h" #include "virstoragefile.h" #include "viruri.h" diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0078a70..1c1476c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -57,6 +57,7 @@ #include "domain_nwfilter.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "nwfilter_conf.h" #include "viruuid.h" #include "virprocess.h" #include "virtime.h"

ACK with the squashed in #includes

Thanks, I've already found that build issue and fixed it.

John

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56e8430..716e9a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5902,6 +5902,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_DOMAIN_SAVE_PAUSED, -1);

+ virNWFilterReadLockFilterUpdates(); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, &xml, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, &wrapperFd, false, false); @@ -5979,6 +5981,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); if (vm) virObjectUnlock(vm); + virNWFilterUnlockFilterUpdates(); return ret; }

@@ -7500,6 +7503,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);

+ virNWFilterReadLockFilterUpdates(); + cfg = virQEMUDriverGetConfig(driver);

affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); @@ -7616,6 +7621,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virObjectUnlock(vm); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return ret; }

@@ -7646,6 +7652,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);

+ virNWFilterReadLockFilterUpdates(); + cfg = virQEMUDriverGetConfig(driver);

affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); @@ -7762,6 +7770,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, virObjectUnlock(vm); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return ret; }

@@ -14503,6 +14512,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * and use of FORCE can cause multiple transitions. */

+ virNWFilterReadLockFilterUpdates(); + if (!(vm = qemuDomObjFromSnapshot(snapshot))) return -1;

@@ -14824,6 +14835,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virObjectUnlock(vm); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates();

return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c797206..533bb35 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2525,6 +2525,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (virTimeMillisNow(&now) < 0) return -1;

+ virNWFilterReadLockFilterUpdates(); + if (flags & VIR_MIGRATE_OFFLINE) { if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { @@ -2826,6 +2828,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, qemuDomainEventQueue(driver, event); qemuMigrationCookieFree(mig); virObjectUnref(caps); + virNWFilterUnlockFilterUpdates(); return ret;

stop: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 24e5f0f..0078a70 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3438,6 +3438,8 @@ qemuProcessReconnect(void *opaque)

VIR_FREE(data);

+ virNWFilterReadLockFilterUpdates(); + virObjectLock(obj);

cfg = virQEMUDriverGetConfig(driver); @@ -3589,6 +3591,7 @@ qemuProcessReconnect(void *opaque)

virObjectUnref(conn); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates();

return;

@@ -3624,6 +3627,7 @@ qemuProcessReconnect(void *opaque) } virObjectUnref(conn); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); }

static int