storage: Remove rwlocks during virStoragePoolObjListForEach

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

Posted by John Ferlan 5 years, 11 months ago

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);
 }
 
 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 5 years, 11 months ago

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);
>  }
>  
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Privoznik 5 years, 11 months ago

On 05/24/2018 09: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);
>  }
>  
>  
> 

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list