storage: Drop and reacquire pool obj lock in some backends

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@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 9b94d26cf96a80fe9d151bfe8cbc93d9d75b2e98..f58b7b294cc84e6eee4c7e1ed359536ededfa6b9 100644 (file)
--- 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 d793ac19e8b0c5e019ede81f0491eacd3165c459..c903677950affd361f3d689cbc9ef7a3c71e53fa 100644 (file)
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -317,7 +317,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;
 }
 
 
@@ -456,13 +463,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'"),
@@ -481,6 +489,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 83b5f27151d0be02a59c8ecff47a8078e20c4f90..603a3f9a42ddc94056c5db3584bdd182179c3b14 100644 (file)
--- 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 d446aa2726e7c3476ed4035e4d55c85f53a088fd..cec21dccbf7c6b253e3efeda4f116bcbf38ce6d1 100644 (file)
--- 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 826a95538e9bfd53bdf55e3bc39beb35b509bad0..d172a5a1650e3f19f67f9e2e7a18f40756cab7f0 100644 (file)
--- 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