Re: [libvirt] [PATCH v2] nwfilter: fix deadlock caused updating network device and nwfilter

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(a)redhat.com&gt; > --- > 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

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 >