Re: [libvirt] [PATCH] qemu: Unlock NW filter lock earlier when not necessary

On 30.04.2015 14:04, Erik Skultety wrote: > The lock is dropped always at the end of an API, but for example when > attaching devices, there's no point in having the NW filter locked if the > device being attached isn't a network interface. It's always a nice > practice to drop unnecessary locks as soon as possible. > --- > src/qemu/qemu_driver.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > BZ 1181074 reported the daemon to hang (not completely true btw) after trying > to attach /dev/ttyX to a running domain. When going through the code I realized > we acquire a read lock for NW filter, although the device itself is not a network > interface and then releasing it at the end. First clue was that this lock which > won't be unlocked as the thread is blocked in reading header of /dev/ttyX causes > the freeze. As it turned out, the problem resides in calling virsh list afterwards > (Peter already does have some patches prepared), but I think the NW filter might > be released earlier in this case with no harm done. > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 74dcb0a..04887e0 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8764,6 +8764,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, > virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; > int ret = -1; > unsigned int affect, parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; > + bool nwfilter_locked = false; > virQEMUCapsPtr qemuCaps = NULL; > qemuDomainObjPrivatePtr priv; > virQEMUDriverConfigPtr cfg = NULL; > @@ -8773,6 +8774,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, > VIR_DOMAIN_AFFECT_CONFIG, -1); > > virNWFilterReadLockFilterUpdates(); > + nwfilter_locked = true; Wait a while. Whenever a programmer needs to know if a lock is locked, something is wrong with the design. That's why POSIX doesn't define such function. Then, this is a Read lock, so it won't hold back other readers, only writers. I don't think there's a real bottle neck unless you are doing a testcase and attaching thousands of devices and defining another thousands of NWFilters. What I'm more curious is - why the heck does this lock needs to be on this level? It's very high level. We lock the lock even when we don't know yet if the XML is right, and what device type we will work on. Can't the lock be moved to lower layers?