On 04/16/2009 12:24 PM, Daniel P. Berrange wrote:
> On Thu, Apr 16, 2009 at 03:57:52PM +0200, Daniel Veillard wrote:
>> On Wed, Apr 15, 2009 at 03:26:44PM -0400, Cole Robinson wrote:
>>> Implement non-pool-blocking volume allocation for FS volumes.
>>>
>>> We setup the volume object, then drop the pool lock, and start the
>>> actual storage allocation. Certain pool operations will be blocked
>>> (start, stop, destroy, refresh), but the basic things like volume
>>> listings will still work.
>>>
>>> This also allows storage allocation progress reporting: the API user can
>>> watch the volume object in a separate thread, polling it's 'info' for
>>> update to date capacity/allocation reporting.
>> Okay, that one is far more complex :-)
>
> It looks sane to me though. This is pretty much spot on how I
> imagined it would all work out.
>
>> [...]
>>> + /* 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));
>>> + voldef = NULL;
>>> +
>>> + /* Drop the pool lock during volume allocation */
>>> + pool->asyncjobs++;
>>> + virStoragePoolObjUnlock(pool);
>>> +
>>> + buildvoldef->building = 1;
>>> + buildret = backend->buildVol(obj->conn, buildvoldef);
>>> + buildvoldef->building = 0;
>>> +
>>> + virStoragePoolObjLock(pool);
>>> + pool->asyncjobs--;
>> since buildvoldef->building is a condition, maybe it's safer to (un)set
>> it while the lock is taken.
>
> This bit doesn't entirely make sense to me.
>
> The 'buildvoldef' object here is a copy of the real volume defintion
> that only this thread is using. Anyone else polling on virStorageVolGetInfo
> will be accessing th real def and thus not see the fact that you changed
> thee 'building' flag.
>
> I'm thinking you instead meant to set 'voldef->building', and setting
> that flag should be protected by the pool lock as DV mentions. Would
> also need to remove the 'voldef= NULL' line if we're going to set the
> flag on it
>
Okay, attached patch sets the flag on the original volume definition,
and makes sure we have the pool lock when we do so. This should address
the above (and DV's) comments.
Thanks,
Cole
commit 4fb92849c5d41db06b09139dd5ced1781e32d3bc
Author: Cole Robinson <crobi...@redhat.com>
Date: Tue Apr 14 16:17:43 2009 -0400
Implement non-pool-blocking volume allocation for FS volumes.
We setup the volume object, then drop the pool lock, and start the actual
storage allocation. Certain pool operations will be blocked (start, stop,
destroy, refresh), but the basic things like volume listings will still work.
This also allows storage allocation progress reporting: the API user can
watch the volume object in a separate thread, polling it's 'info' for
update to date capacity/allocation reporting.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 350a931..b6ac8e0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -46,6 +46,7 @@ virGetDomain;
virGetNetwork;
virGetStoragePool;
virGetStorageVol;
+virUnrefStorageVol;
virGetNodeDevice;
virUnrefDomain;
virUnrefConnect;
diff --git a/src/storage_backend.h b/src/storage_backend.h
index 7fab384..c9c1e35 100644
--- a/src/storage_backend.h
+++ b/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;
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
index 5000f43..7a1bba8 100644
--- a/src/storage_backend_fs.c
+++ b/src/storage_backend_fs.c
@@ -982,16 +982,16 @@ virStorageBackendFileSystemDelete(virConnectPtr conn,
/**
- * 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
+ * Set up a volume definition to be added to a pool's volume list, but
+ * don't do any file creation or allocation. By separating the two processes,
+ * 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,
diff --git a/src/storage_driver.c b/src/storage_driver.c
index 8cb8d4b..1524805 100644
--- a/src/storage_driver.c
+++ b/src/storage_driver.c
@@ -1189,6 +1189,8 @@ cleanup:
return ret;
}
+static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags);
+
static virStorageVolPtr
storageVolumeCreateXML(virStoragePoolPtr obj,
const char *xmldesc,
@@ -1196,8 +1198,8 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
virStorageDriverStatePtr driver = obj->conn->storagePrivateData;
virStoragePoolObjPtr pool;
virStorageBackendPtr backend;
- virStorageVolDefPtr vol = NULL;
- virStorageVolPtr ret = NULL;
+ virStorageVolDefPtr voldef = NULL;
+ virStorageVolPtr ret = NULL, volobj = NULL;
storageDriverLock(driver);
pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
@@ -1218,11 +1220,11 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
goto cleanup;
- vol = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL);
- if (vol == NULL)
+ voldef = virStorageVolDefParse(obj->conn, pool->def, xmldesc, 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;
@@ -1236,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, vol) < 0) {
+ if (backend->createVol(obj->conn, pool, voldef) < 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);
- vol = NULL;
+ if (0) {
+ 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;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
