From: Michal Privoznik <mprivozn@redhat.com>
Date: Fri, 24 May 2019 14:35:47 +0000 (+0200)
Subject: storage: Drop and reacquire pool obj lock in some backends
X-Git-Tag: v5.7.0-rc1~104
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=985f035fbfd074e71ccc1e3cb1177482d77e1537;p=thirdparty%2Flibvirt.git

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 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 d793ac19e8..c903677950 100644
--- 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 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