[libvirt] [PATCH v1 11/11] storage: Drop and reacquire pool obj lock in some backends

[Michal Privoznik]

https://bugzilla.redhat.com/show_bug.cgi?id=1711789

Starting up or building some types of pools may take a very long
time (e.g. a misconfigured NFS). Holding the pool object locked
throughout the whole time hurts concurrency, e.g. if there's
another thread that is listing all the pools.

Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
---
 src/storage/storage_backend_disk.c     | 11 ++++++++++-
 src/storage/storage_backend_fs.c       | 13 +++++++++++--
 src/storage/storage_backend_logical.c  | 15 ++++++++++-----
 src/storage/storage_backend_vstorage.c |  9 ++++++++-
 src/storage/storage_backend_zfs.c      |  7 ++++++-
 5 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 9b94d26cf9..f58b7b294c 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -461,7 +461,10 @@ virStorageBackendDiskStartPool(virStoragePoolObjPtr pool)
     const char *format;
     const char *path = def->source.devices[0].path;
 
+    /* This can take a significant amount of time. */
+    virObjectUnlock(pool);
     virWaitForDevices();
+    virObjectLock(pool);
 
     if (!virFileExists(path)) {
         virReportError(VIR_ERR_INVALID_ARG,
@@ -490,6 +493,7 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
     int format = def->source.format;
     const char *fmt;
     VIR_AUTOPTR(virCommand) cmd = NULL;
+    int ret = -1;
 
     virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
                   VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
@@ -523,7 +527,12 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
                                "--script",
                                fmt,
                                NULL);
-    return virCommandRun(cmd, NULL);
+
+    virObjectUnlock(pool);
+    ret = virCommandRun(cmd, NULL);
+    virObjectLock(pool);
+
+    return ret;
 }
 
 
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index ae4e9a03a3..1257419760 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -318,7 +318,14 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
         return -1;
 
     cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src);
-    return virCommandRun(cmd, NULL);
+
+    /* Mounting a shared FS might take a long time. Don't hold
+     * the pool locked meanwhile. */
+    virObjectUnlock(pool);
+    rc = virCommandRun(cmd, NULL);
+    virObjectLock(pool);
+
+    return rc;
 }
 
 
@@ -457,13 +464,14 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("No source device specified when formatting pool '%s'"),
                        def->name);
-        goto error;
+        return -1;
     }
 
     device = def->source.devices[0].path;
     format = virStoragePoolFormatFileSystemTypeToString(def->source.format);
     VIR_DEBUG("source device: '%s' format: '%s'", device, format);
 
+    virObjectUnlock(pool);
     if (!virFileExists(device)) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("Source device does not exist when formatting pool '%s'"),
@@ -482,6 +490,7 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
         ret = virStorageBackendExecuteMKFS(device, format);
 
  error:
+    virObjectLock(pool);
     return ret;
 }
 
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index 83b5f27151..603a3f9a42 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -50,9 +50,15 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     VIR_AUTOPTR(virCommand) cmd = NULL;
+    int ret;
 
     cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on);
-    return virCommandRun(cmd, NULL);
+
+    virObjectUnlock(pool);
+    ret = virCommandRun(cmd, NULL);
+    virObjectLock(pool);
+
+    return ret;
 }
 
 
@@ -723,11 +729,10 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool,
         virCommandAddArg(vgcmd, path);
     }
 
+    virObjectUnlock(pool);
     /* Now create the volume group itself */
-    if (virCommandRun(vgcmd, NULL) < 0)
-        goto cleanup;
-
-    ret = 0;
+    ret = virCommandRun(vgcmd, NULL);
+    virObjectLock(pool);
 
  cleanup:
     /* On any failure, run through the devices that had pvcreate run in
diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
index d446aa2726..cec21dccbf 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -42,6 +42,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
     VIR_AUTOFREE(char *) usr_name = NULL;
     VIR_AUTOFREE(char *) mode = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
+    int ret;
 
     /* Check the permissions */
     if (def->target.perms.mode == (mode_t)-1)
@@ -69,7 +70,13 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
                                "-g", grp_name, "-u", usr_name,
                                NULL);
 
-    return virCommandRun(cmd, NULL);
+    /* Mounting a shared FS might take a long time. Don't hold
+     * the pool locked meanwhile. */
+    virObjectUnlock(pool);
+    ret = virCommandRun(cmd, NULL);
+    virObjectLock(pool);
+
+    return ret;
 }
 
 
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 826a95538e..d172a5a165 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -385,6 +385,7 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool,
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     size_t i;
     VIR_AUTOPTR(virCommand) cmd = NULL;
+    int ret = -1;
 
     virCheckFlags(0, -1);
 
@@ -400,7 +401,11 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool,
     for (i = 0; i < def->source.ndevice; i++)
         virCommandAddArg(cmd, def->source.devices[i].path);
 
-    return virCommandRun(cmd, NULL);
+    virObjectUnlock(pool);
+    ret = virCommandRun(cmd, NULL);
+    virObjectLock(pool);
+
+    return ret;
 }
 
 static int
-- 
2.21.0