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

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

 

On 11/11/2014 04:13 PM, John Ferlan wrote:



On 11/05/2014 09:02 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
updating/attaching network device to domain.


The referenced commit has a roadmap of sorts of what the lock is -
perhaps you could add similar details here for before and after.


In that case I can do copy&paste of that roadmap as it's the same and
this patch only extend the referenced one.



While more of a pain logistically - separating out the patches into
single changes may make it easier to list the bad locking order followed
by the order the commit changes.

The ones that are most odd are the places where you take out the lock in
the middle of a function.  That seems to go against the original commit
which takes them out right after the flags check. In particular that's
the qemuDomainRestoreFlags and qemuMigrationPrepareAny changes.


The order of the nwfilter lock and flags could be the same in all
functions, that's a good point. My intention was to lock the nwfilterRead right before the vm is locked because the deadlock is between the vm and nwfilter objects.
However I don't quiet understand why you would like to split the patch into a series. All the paths leads to the same nwfilter call "virDomainConfNWFilterInstantiate" and that's the only function callable outside of nwfilter that could cause the deadlock. 





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



--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]