Re: [libvirt] [PATCH] storage: Remove rwlocks during virStoragePoolObjListForEach

29 May 2018

ping?

This resolves a hang

Tks,

John


On 05/24/2018 03:52 PM, John Ferlan wrote:
...
Remove the locks since they are unnecessary and would cause
a hang for a driver reload/restart when a transient pool was
previously active as a result of the call:
virStoragePoolUpdateInactive:
...
    if (!virStoragePoolObjGetConfigFile(obj)) {
        virStoragePoolObjRemove(driver->pools, obj);
...
stack trace:
Thread 17 (Thread 0x7fffcc574700 (LWP 12465)):
...pthread_rwlock_wrlock
...virRWLockWrite
...virObjectRWLockWrite
...virStoragePoolObjRemove
...virStoragePoolUpdateInactive
...storagePoolUpdateStateCallback
...virStoragePoolObjListForEachCb
...virHashForEach
...virStoragePoolObjListForEach
...storagePoolUpdateAllState
...storageStateInitialize
...virStateInitialize
...daemonRunStateInit
...virThreadHelper
...start_thread
...clone
Introduced by commit id 4b2e0ed6e3.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virstorageobj.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 6c937f105c..e66b2ebfb2 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -438,6 +438,12 @@ virStoragePoolObjListForEachCb(void *payload,
  * not as it's being used for Autostart and UpdateAllState callers
  * that want to iterate over all the @pools objects not stopping if
  * one happens to fail.
+ *
+ * NB: We cannot take the Storage Pool lock here because it's possible
+ *     that some action as part of Autostart or UpdateAllState will need
+ *     to modify/destroy a transient pool. Since these paths only occur
+ *     during periods in which the storageDriverLock is held (Initialization,
+ *     AutoStart, or Reload) this is OK.
  */
 void
 virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
@@ -447,9 +453,7 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
     struct _virStoragePoolObjListForEachData data = { .iter = iter,
                                                       .opaque = opaque };
-    virObjectRWLockRead(pools);
     virHashForEach(pools->objs, virStoragePoolObjListForEachCb, &data);
-    virObjectRWUnlock(pools);
 }


    