Re: [libvirt] [PATCH] storage: Remove rwlocks during virStoragePoolObjListForEach
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(a)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