[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 4/4] Push nwfilter update locking up to top level



On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
The NWFilter code has as a deadlock race condition between
the virNWFilter{Define,Undefine} APIs and starting of guest
VMs due to mis-matched lock ordering.

In the virNWFilter{Define,Undefine} codepaths the lock ordering
is

   1. nwfilter driver lock
   2. virt driver lock
   3. nwfilter update lock
   4. domain object lock

In the VM guest startup paths the lock ordering is

   1. virt driver lock
   2. domain object lock
   3. nwfilter update lock

As can be seen the domain object and nwfilter update locks are
not acquired in a consistent order.

The fix used is to push the nwfilter update lock upto the top
level resulting in a lock ordering for virNWFilter{Define,Undefine}
of

   1. nwfilter driver lock
   2. nwfilter update lock

Insert: 3. nwfilter callback drivers lock

This is then used in this order also by nwfilterStateReload

   3. virt driver lock
   4. domain object lock

and VM start using

   1. nwfilter update lock
   2. virt driver lock
   3. domain object lock

This has the effect of serializing VM startup once again, even if
no nwfilters are applied to the guest. There is also the possibility
of deadlock due to a call graph loop via virNWFilterInstantiate
and virNWFilterInstantiateFilterLate.

These two problems mean the lock must be turned into a read/write
lock instead of a plain mutex at the same time. The lock is used to
serialize changes to the "driver->nwfilters" hash, so the write lock
only needs to be held by the define/undefine methods. All other
methods can rely on a read lock which allows good concurrency.

Signed-off-by: Daniel P. Berrange<berrange redhat com>
---
  src/conf/nwfilter_conf.c               | 23 +++++++++++------------
  src/conf/nwfilter_conf.h               |  3 ++-
  src/libvirt_private.syms               |  3 ++-
  src/lxc/lxc_driver.c                   |  6 ++++++
  src/nwfilter/nwfilter_driver.c         | 11 +++++++----
  src/nwfilter/nwfilter_gentech_driver.c |  4 +---
  src/qemu/qemu_driver.c                 |  6 ++++++
  src/uml/uml_driver.c                   |  4 ++++
  8 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 112e8cb..80030c8 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -285,12 +285,15 @@ nwfilterStateReload(void)
      virNWFilterLearnThreadsTerminate(true);

      nwfilterDriverLock(driverState);
+    virNWFilterWriteLockFilterUpdates();
      virNWFilterCallbackDriversLock();

+

stray newline here

      virNWFilterLoadAllConfigs(&driverState->nwfilters,
                                driverState->configDir);

      virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
      nwfilterDriverUnlock(driverState);

      virNWFilterInstFiltersOnAllVMs();

Modulo the above stray line and then comment in the introductory text, I think this patch is good. I am just a bit concerned about 3/4.

   Stefan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]