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

[Michal Privoznik]

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 at 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