drop the pool lock when allocating fs volumes

* src/libvirt_private.syms src/storage_backend.h
  src/storage_backend_fs.c src/storage_conf.h src/storage_driver.c:
  drop the pool lock when allocating fs volumes, patch by Cole Robinson
daniel

ChangeLog

@@ -1,3 +1,9 @@
+Fri Apr 17 21:10:28 CEST 2009 Daniel Veillard <veillard@redhat.com>
+
+	* src/libvirt_private.syms src/storage_backend.h
+	  src/storage_backend_fs.c src/storage_conf.h src/storage_driver.c:
+	  drop the pool lock when allocating fs volumes, patch by Cole Robinson
+
 Fri Apr 17 18:05:52 CEST 2009 Daniel Veillard <veillard@redhat.com>
 
 	* configure.in include/libvirt/virterror.h src/Makefile.am

src/libvirt_private.syms

@@ -46,6 +46,7 @@ virGetDomain;
 virGetNetwork;
 virGetStoragePool;
 virGetStorageVol;
+virUnrefStorageVol;
 virGetNodeDevice;
 virUnrefDomain;
 virUnrefConnect;

src/storage_backend.h

@@ -34,6 +34,7 @@ typedef int (*virStorageBackendRefreshPool)(virConnectPtr conn, virStoragePoolOb
 typedef int (*virStorageBackendStopPool)(virConnectPtr conn, virStoragePoolObjPtr pool);
 typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags);
 
+typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol);
 typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol);
 typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol);
 typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags);
@@ -52,6 +53,7 @@ struct _virStorageBackend {
     virStorageBackendStopPool stopPool;
     virStorageBackendDeletePool deletePool;
 
+    virStorageBackendBuildVol buildVol;
     virStorageBackendCreateVol createVol;
     virStorageBackendRefreshVol refreshVol;
     virStorageBackendDeleteVol deleteVol;

src/storage_backend_fs.c

@@ -982,16 +982,16 @@ virStorageBackendFileSystemDelete(virConnectPtr conn,
 
 
 /**
- * Allocate a new file as a volume. This is either done directly
+ * Set up a volume definition to be added to a pool's volume list, but
- * for raw/sparse files, or by calling qemu-img/qcow-create for
+ * don't do any file creation or allocation. By separating the two processes,
- * special kinds of files
+ * we allow allocation progress reporting (by polling the volume's 'info'
+ * function), and can drop the parent pool lock during the (slow) allocation.
  */
 static int
 virStorageBackendFileSystemVolCreate(virConnectPtr conn,
                                      virStoragePoolObjPtr pool,
                                      virStorageVolDefPtr vol)
 {
-    int fd;
 
     if (VIR_ALLOC_N(vol->target.path, strlen(pool->def->target.path) +
                     1 + strlen(vol->name) + 1) < 0) {
@@ -1008,6 +1008,20 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
         return -1;
     }
 
+    return 0;
+}
+
+/**
+ * Allocate a new file as a volume. This is either done directly
+ * for raw/sparse files, or by calling qemu-img/qcow-create for
+ * special kinds of files
+ */
+static int
+virStorageBackendFileSystemVolBuild(virConnectPtr conn,
+                                    virStorageVolDefPtr vol)
+{
+    int fd;
+
     if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) {
         if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL,
                        vol->target.perms.mode)) < 0) {
@@ -1017,6 +1031,16 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
             return -1;
         }
 
+        /* Seek to the final size, so the capacity is available upfront
+         * for progress reporting */
+        if (ftruncate(fd, vol->capacity) < 0) {
+            virReportSystemError(conn, errno,
+                                 _("cannot extend file '%s'"),
+                                 vol->target.path);
+            close(fd);
+            return -1;
+        }
+
         /* Pre-allocate any data if requested */
         /* XXX slooooooooooooooooow on non-extents-based file systems */
         /* FIXME: Add in progress bars & bg thread if progress bar requested */
@@ -1039,7 +1063,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
                         virReportSystemError(conn, r,
                                              _("cannot fill file '%s'"),
                                              vol->target.path);
-                        unlink(vol->target.path);
                         close(fd);
                         return -1;
                     }
@@ -1052,22 +1075,12 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
                     virReportSystemError(conn, r,
                                          _("cannot fill file '%s'"),
                                          vol->target.path);
-                    unlink(vol->target.path);
                     close(fd);
                     return -1;
                 }
             }
         }
 
-        /* Now seek to final size, possibly making the file sparse */
-        if (ftruncate(fd, vol->capacity) < 0) {
-            virReportSystemError(conn, errno,
-                                 _("cannot extend file '%s'"),
-                                 vol->target.path);
-            unlink(vol->target.path);
-            close(fd);
-            return -1;
-        }
     } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) {
         if (mkdir(vol->target.path, vol->target.perms.mode) < 0) {
             virReportSystemError(conn, errno,
@@ -1127,7 +1140,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
         snprintf(size, sizeof(size), "%llu", vol->capacity/1024);
 
         if (virRun(conn, imgargv, NULL) < 0) {
-            unlink(vol->target.path);
             return -1;
         }
 
@@ -1135,7 +1147,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
             virReportSystemError(conn, errno,
                                  _("cannot read path '%s'"),
                                  vol->target.path);
-            unlink(vol->target.path);
             return -1;
         }
 #elif HAVE_QCOW_CREATE
@@ -1168,7 +1179,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
         imgargv[3] = NULL;
 
         if (virRun(conn, imgargv, NULL) < 0) {
-            unlink(vol->target.path);
             return -1;
         }
 
@@ -1176,7 +1186,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
             virReportSystemError(conn, errno,
                                  _("cannot read path '%s'"),
                                  vol->target.path);
-            unlink(vol->target.path);
             return -1;
         }
 #else
@@ -1193,7 +1202,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
             virReportSystemError(conn, errno,
                                  _("cannot set file owner '%s'"),
                                  vol->target.path);
-            unlink(vol->target.path);
             close(fd);
             return -1;
         }
@@ -1202,7 +1210,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
         virReportSystemError(conn, errno,
                              _("cannot set file mode '%s'"),
                              vol->target.path);
-        unlink(vol->target.path);
         close(fd);
         return -1;
     }
@@ -1211,7 +1218,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
     if (virStorageBackendUpdateVolTargetInfoFD(conn, &vol->target, fd,
                                                &vol->allocation,
                                                NULL) < 0) {
-        unlink(vol->target.path);
         close(fd);
         return -1;
     }
@@ -1220,7 +1226,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
         virReportSystemError(conn, errno,
                              _("cannot close file '%s'"),
                              vol->target.path);
-        unlink(vol->target.path);
         return -1;
     }
 
@@ -1268,6 +1273,7 @@ virStorageBackend virStorageBackendDirectory = {
     .buildPool = virStorageBackendFileSystemBuild,
     .refreshPool = virStorageBackendFileSystemRefresh,
     .deletePool = virStorageBackendFileSystemDelete,
+    .buildVol = virStorageBackendFileSystemVolBuild,
     .createVol = virStorageBackendFileSystemVolCreate,
     .refreshVol = virStorageBackendFileSystemVolRefresh,
     .deleteVol = virStorageBackendFileSystemVolDelete,
@@ -1282,6 +1288,7 @@ virStorageBackend virStorageBackendFileSystem = {
     .refreshPool = virStorageBackendFileSystemRefresh,
     .stopPool = virStorageBackendFileSystemStop,
     .deletePool = virStorageBackendFileSystemDelete,
+    .buildVol = virStorageBackendFileSystemVolBuild,
     .createVol = virStorageBackendFileSystemVolCreate,
     .refreshVol = virStorageBackendFileSystemVolRefresh,
     .deleteVol = virStorageBackendFileSystemVolDelete,
@@ -1295,6 +1302,7 @@ virStorageBackend virStorageBackendNetFileSystem = {
     .refreshPool = virStorageBackendFileSystemRefresh,
     .stopPool = virStorageBackendFileSystemStop,
     .deletePool = virStorageBackendFileSystemDelete,
+    .buildVol = virStorageBackendFileSystemVolBuild,
     .createVol = virStorageBackendFileSystemVolCreate,
     .refreshVol = virStorageBackendFileSystemVolRefresh,
     .deleteVol = virStorageBackendFileSystemVolDelete,

src/storage_conf.h

@@ -84,6 +84,8 @@ struct _virStorageVolDef {
     char *key;
     int type; /* virStorageVolType enum */
 
+    unsigned int building;
+
     unsigned long long allocation;
     unsigned long long capacity;
 
@@ -238,6 +240,7 @@ struct _virStoragePoolObj {
     char *autostartLink;
     int active;
     int autostart;
+    unsigned int asyncjobs;
 
     virStoragePoolDefPtr def;
     virStoragePoolDefPtr newDef;

src/storage_driver.c

@@ -564,6 +564,13 @@ storagePoolUndefine(virStoragePoolPtr obj) {
         goto cleanup;
     }
 
+    if (pool->asyncjobs > 0) {
+        virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+                              _("pool '%s' has asynchronous jobs running."),
+                              pool->def->name);
+        goto cleanup;
+    }
+
     if (virStoragePoolObjDeleteDef(obj->conn, pool) < 0)
         goto cleanup;
 
@@ -696,6 +703,13 @@ storagePoolDestroy(virStoragePoolPtr obj) {
         goto cleanup;
     }
 
+    if (pool->asyncjobs > 0) {
+        virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+                              _("pool '%s' has asynchronous jobs running."),
+                              pool->def->name);
+        goto cleanup;
+    }
+
     if (backend->stopPool &&
         backend->stopPool(obj->conn, pool) < 0)
         goto cleanup;
@@ -745,6 +759,13 @@ storagePoolDelete(virStoragePoolPtr obj,
         goto cleanup;
     }
 
+    if (pool->asyncjobs > 0) {
+        virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+                              _("pool '%s' has asynchronous jobs running."),
+                              pool->def->name);
+        goto cleanup;
+    }
+
     if (!backend->deletePool) {
         virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT,
                               "%s", _("pool does not support volume delete"));
@@ -788,6 +809,13 @@ storagePoolRefresh(virStoragePoolPtr obj,
         goto cleanup;
     }
 
+    if (pool->asyncjobs > 0) {
+        virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+                              _("pool '%s' has asynchronous jobs running."),
+                              pool->def->name);
+        goto cleanup;
+    }
+
     virStoragePoolObjClearVols(pool);
     if (backend->refreshPool(obj->conn, pool) < 0) {
         if (backend->stopPool)
@@ -1161,6 +1189,8 @@ cleanup:
     return ret;
 }
 
+static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags);
+
 static virStorageVolPtr
 storageVolumeCreateXML(virStoragePoolPtr obj,
                        const char *xmldesc,
@@ -1168,8 +1198,8 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
     virStorageDriverStatePtr driver = obj->conn->storagePrivateData;
     virStoragePoolObjPtr pool;
     virStorageBackendPtr backend;
-    virStorageVolDefPtr vol = NULL;
+    virStorageVolDefPtr voldef = NULL;
-    virStorageVolPtr ret = NULL;
+    virStorageVolPtr ret = NULL, volobj = NULL;
 
     storageDriverLock(driver);
     pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
@@ -1190,11 +1220,11 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
     if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
         goto cleanup;
 
-    vol = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL);
+    voldef = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL);
-    if (vol == NULL)
+    if (voldef == NULL)
         goto cleanup;
 
-    if (virStorageVolDefFindByName(pool, vol->name)) {
+    if (virStorageVolDefFindByName(pool, voldef->name)) {
         virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
                               "%s", _("storage vol already exists"));
         goto cleanup;
@@ -1208,22 +1238,72 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
 
     if (!backend->createVol) {
         virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT,
-                              "%s", _("storage pool does not support volume creation"));
+                              "%s", _("storage pool does not support volume "
+                                      "creation"));
         goto cleanup;
     }
 
-    /* XXX ought to drop pool lock while creating instance */
+    if (backend->createVol(obj->conn, pool, voldef) < 0) {
-    if (backend->createVol(obj->conn, pool, vol) < 0) {
         goto cleanup;
     }
 
-    pool->volumes.objs[pool->volumes.count++] = vol;
+    pool->volumes.objs[pool->volumes.count++] = voldef;
+    volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
+                              voldef->key);
 
-    ret = virGetStorageVol(obj->conn, pool->def->name, vol->name, vol->key);
+    if (0) {
-    vol = NULL;
+        printf("after vol lookup.\n");
+        virReportOOMError(obj->conn);
+        goto cleanup;
+    }
+
+    if (volobj && backend->buildVol) {
+        int buildret;
+        virStorageVolDefPtr buildvoldef = NULL;
+
+        if (VIR_ALLOC(buildvoldef) < 0) {
+            virReportOOMError(obj->conn);
+            voldef = NULL;
+            goto cleanup;
+        }
+
+        /* Make a shallow copy of the 'defined' volume definition, since the
+         * original allocation value will change as the user polls 'info',
+         * but we only need the initial requested values
+         */
+        memcpy(buildvoldef, voldef, sizeof(*voldef));
+
+        /* Drop the pool lock during volume allocation */
+        pool->asyncjobs++;
+        voldef->building = 1;
+        virStoragePoolObjUnlock(pool);
+
+        buildret = backend->buildVol(obj->conn, buildvoldef);
+
+        virStoragePoolObjLock(pool);
+        voldef->building = 0;
+        pool->asyncjobs--;
+
+        voldef = NULL;
+        VIR_FREE(buildvoldef);
+
+        if (buildret < 0) {
+            virStoragePoolObjUnlock(pool);
+            storageVolumeDelete(volobj, 0);
+            pool = NULL;
+            goto cleanup;
+        }
+
+    }
+
+    ret = volobj;
+    volobj = NULL;
+    voldef = NULL;
 
 cleanup:
-    virStorageVolDefFree(vol);
+    if (volobj)
+        virUnrefStorageVol(volobj);
+    virStorageVolDefFree(voldef);
     if (pool)
         virStoragePoolObjUnlock(pool);
     return ret;
@@ -1266,6 +1346,13 @@ storageVolumeDelete(virStorageVolPtr obj,
         goto cleanup;
     }
 
+    if (vol->building) {
+        virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+                              _("volume '%s' is still being allocated."),
+                              vol->name);
+        goto cleanup;
+    }
+
     if (!backend->deleteVol) {
         virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT,
                               "%s", _("storage pool does not support vol deletion"));