The attached patch implements storage volume allocation progress
reporting for file volumes.
Currently, the volume creation process looks like:
- Grab the pool lock
- Fully allocated the volume
- 'define' the volume (so it shows up in 'virsh vol-list', etc)
- Lookup the volume object to return
- Drop the pool lock
The new sequence is:
- Grab the pool lock
- 'define' the volume (even though nothing is on disk yet)
- Drop the pool lock
- Allocate the volume as needed
- Regrab the pool lock
- Lookup the volume object to return
- Drop the pool lock (again)
Since the volume is 'defined', the user can fetch an object reference in
a separate thread, and continually poll the 'info' command for up to
date numbers. This also has the benefit of dropping the pool lock during
the potentially lengthy allocation, as currently 'virsh pool-list' etc.
will block while any volume is being allocated.
Non file volumes maintain existing behavior.
I tried to make the implementation resistant to user error: say if the
pool is deactivated or deleted while the volume is being allocated. The
creation process may bail out, but I couldn't produce any bad errors
(crashing).
There are a few other small fixes in this patch:
- Refresh volume info when doing volume dumpxml
- Update volume capacity when doing a refresh
I've also attached an ugly python script that can test this. Presuming
you have a pool named 'default', running
python sparse.py --vol-create-info
Will launch an allocation, print vol.info in a loop.
Feedback appreciated.
Thanks,
Cole
diff --git a/src/storage_backend.h b/src/storage_backend.h
index 7fab384..6073d06 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 (*virStorageBackendDefineVol)(virConnectPtr conn, virStoragePoolObjPtr pool, 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;
+ virStorageBackendDefineVol defineVol;
virStorageBackendCreateVol createVol;
virStorageBackendRefreshVol refreshVol;
virStorageBackendDeleteVol deleteVol;
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
index 5000f43..8c7dca5 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,
+virStorageBackendFileSystemVolDefine(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,21 @@ 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
+virStorageBackendFileSystemVolCreate(virConnectPtr conn,
+ virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
+ 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 +1032,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 +1064,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
virReportSystemError(conn, r,
_("cannot fill file '%s'"),
vol->target.path);
- unlink(vol->target.path);
close(fd);
return -1;
}
@@ -1052,22 +1076,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 +1141,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 +1148,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 +1180,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
imgargv[3] = NULL;
if (virRun(conn, imgargv, NULL) < 0) {
- unlink(vol->target.path);
return -1;
}
@@ -1176,7 +1187,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
virReportSystemError(conn, errno,
_("cannot read path '%s'"),
vol->target.path);
- unlink(vol->target.path);
return -1;
}
#else
@@ -1193,7 +1203,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 +1211,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 +1219,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
if (virStorageBackendUpdateVolTargetInfoFD(conn, &vol->target, fd,
&vol->allocation,
NULL) < 0) {
- unlink(vol->target.path);
close(fd);
return -1;
}
@@ -1220,7 +1227,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
virReportSystemError(conn, errno,
_("cannot close file '%s'"),
vol->target.path);
- unlink(vol->target.path);
return -1;
}
@@ -1259,7 +1265,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn,
virStorageVolDefPtr vol)
{
/* Refresh allocation / permissions info in case its changed */
- return virStorageBackendUpdateVolInfo(conn, vol, 0);
+ return virStorageBackendUpdateVolInfo(conn, vol, 1);
}
virStorageBackend virStorageBackendDirectory = {
@@ -1268,6 +1274,7 @@ virStorageBackend virStorageBackendDirectory = {
.buildPool = virStorageBackendFileSystemBuild,
.refreshPool = virStorageBackendFileSystemRefresh,
.deletePool = virStorageBackendFileSystemDelete,
+ .defineVol = virStorageBackendFileSystemVolDefine,
.createVol = virStorageBackendFileSystemVolCreate,
.refreshVol = virStorageBackendFileSystemVolRefresh,
.deleteVol = virStorageBackendFileSystemVolDelete,
@@ -1282,6 +1289,7 @@ virStorageBackend virStorageBackendFileSystem = {
.refreshPool = virStorageBackendFileSystemRefresh,
.stopPool = virStorageBackendFileSystemStop,
.deletePool = virStorageBackendFileSystemDelete,
+ .defineVol = virStorageBackendFileSystemVolDefine,
.createVol = virStorageBackendFileSystemVolCreate,
.refreshVol = virStorageBackendFileSystemVolRefresh,
.deleteVol = virStorageBackendFileSystemVolDelete,
@@ -1295,6 +1303,7 @@ virStorageBackend virStorageBackendNetFileSystem = {
.refreshPool = virStorageBackendFileSystemRefresh,
.stopPool = virStorageBackendFileSystemStop,
.deletePool = virStorageBackendFileSystemDelete,
+ .defineVol = virStorageBackendFileSystemVolDefine,
.createVol = virStorageBackendFileSystemVolCreate,
.refreshVol = virStorageBackendFileSystemVolRefresh,
.deleteVol = virStorageBackendFileSystemVolDelete,
diff --git a/src/storage_driver.c b/src/storage_driver.c
index b261843..c30f9ca 100644
--- a/src/storage_driver.c
+++ b/src/storage_driver.c
@@ -1161,6 +1161,8 @@ cleanup:
return ret;
}
+static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags);
+
static virStorageVolPtr
storageVolumeCreateXML(virStoragePoolPtr obj,
const char *xmldesc,
@@ -1168,8 +1170,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);
@@ -1190,11 +1192,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;
@@ -1208,22 +1210,87 @@ 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->defineVol) {
+ if (backend->defineVol(obj->conn, pool, voldef) < 0)
+ goto cleanup;
+
+ } else 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 (backend->defineVol) {
+ virStorageVolDefPtr tmpvoldef = NULL;
+ if (VIR_ALLOC(tmpvoldef) < 0) {
+ virReportOOMError(obj->conn);
+ goto cleanup;
+ }
+
+ // FIXME: free on error
+ /* Make a copy of the 'defined' volume definition, since the
+ * original's allocation value will change as the user polls 'info',
+ * but we only care about the initial requested values
+ */
+ memcpy(tmpvoldef, voldef, sizeof(*voldef));
+
+ /* This way we can drop the pool lock after assigning the vol
+ * definintion, but before vol allocation */
+ virStoragePoolObjUnlock(pool);
+
+ if (backend->createVol(obj->conn, pool, tmpvoldef) < 0) {
+ /* Since we don't have the pool lock, we can call out to
+ * the public driver delete */
+ storageVolumeDelete(volobj, 0);
+ voldef = NULL; /* Delete frees this */
+
+ VIR_FREE(tmpvoldef);
+ goto cleanup;
+ }
+
+ /* Lookup latest pool information */
+ storageDriverLock(driver);
+ pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
+ storageDriverUnlock(driver);
+
+ if (!pool) {
+ virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
+ _("could not find parent storage pool "
+ "after creating volume '%s'"),
+ tmpvoldef->name);
+ virStorageVolDefFree(tmpvoldef);
+ goto cleanup;
+ }
+
+ if (!virStoragePoolObjIsActive(pool)) {
+ virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
+ _("storage pool is no longer active after"
+ "creating volume '%s'"), tmpvoldef->name);
+ virStorageVolDefFree(tmpvoldef);
+ goto cleanup;
+ }
+
+ ret = virGetStorageVol(obj->conn, pool->def->name,
+ tmpvoldef->name, tmpvoldef->key);
+
+ } else {
+ ret = volobj;
+ volobj = NULL;
+ }
+
+ voldef = NULL;
cleanup:
- virStorageVolDefFree(vol);
+ if (volobj)
+ virStorageVolFree(volobj);
+ virStorageVolDefFree(voldef);
if (pool)
virStoragePoolObjUnlock(pool);
return ret;
@@ -1389,6 +1456,10 @@ storageVolumeGetXMLDesc(virStorageVolPtr obj,
if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
goto cleanup;
+ if (backend->refreshVol &&
+ backend->refreshVol(obj->conn, pool, vol) < 0)
+ goto cleanup;
+
ret = virStorageVolDefFormat(obj->conn, pool->def, vol);
cleanup:
import libvirt
import threading
import time
import sys
import optparse
poolname = "default"
volname = "nonsparsetest"
testvol = "testvol"
uri = "qemu:///system"
testvolxml = """
<volume>
<name>%s</name>
<capacity>1048576000</capacity>
<allocation>0</allocation>
<target>
<format type='raw'/>
</target>
</volume>
""" % testvol
volxml = """
<volume>
<name>%s</name>
<capacity>1048576000</capacity>
<allocation>800000000</allocation>
<target>
<format type='raw'/>
</target>
</volume>
""" % volname
failvol = """
<volume>
<name>%s</name>
<capacity>1048576000</capacity>
<allocation>1048576000</allocation>
<target>
<format type='bochs'/>
</target>
</volume>
""" % volname
poolxml = """
<pool type='dir'>
<name>default</name>
<target>
<path>/var/lib/libvirt/images</path>
</target>
</pool>
"""
# Helper functions
def exception_wrapper(cmd, args):
try:
cmd(*args)
except Exception, e:
print str(e)
def make_test_vol():
pool = get_pool()
pool.createXML(testvolxml, 0)
def del_vol(name=volname):
pool = get_pool()
vol = pool.storageVolLookupByName(name)
vol.delete(0)
def del_pool():
pool = get_pool()
pool.destroy()
pool.undefine()
def define_pool():
conn = libvirt.open(uri)
pool = conn.storagePoolDefineXML(poolxml, 0)
pool.create(0)
pool.setAutostart(True)
def allocate_thread(xml=volxml):
try:
del_vol()
except:
pass
pool = get_pool()
print "creating vol in thread"
vol = pool.createXML(xml, 0)
print "creating vol complete."
def info_thread(vol):
for i in range(0, 40):
time.sleep(.5)
print vol.info()
def get_pool(name=poolname):
conn = libvirt.open(uri)
return conn.storagePoolLookupByName(name)
def get_vol(name=volname):
pool = get_pool()
return pool.storageVolLookupByName(name)
def cmd_vol_create(xml=volxml):
exception_wrapper(define_pool, ())
pool = get_pool()
print pool.listVolumes()
t = threading.Thread(target=allocate_thread, name="Allocating",
args=(xml,))
t.start()
time.sleep(5)
print "\nRefreshing pool and dumping list"
pool.refresh(0)
print pool.listVolumes()
def cmd_vol_fail():
cmd_vol_create(failvol)
def cmd_vol_poll():
cmd_vol_create()
vol = get_vol()
t = threading.Thread(target=info_thread, name="Getting info",
args=(vol,))
t.start()
def main():
import optparse
parser = optparse.OptionParser()
parser.add_option("", "--vol-create", action="store_true",
dest="vol_create",
help="Create a volume nonsparse volume that should "
"succeed")
parser.add_option("", "--vol-create-info", action="store_true",
dest="vol_create_info",
help="Create a volume nonsparse volume that should "
"succeed, and list info in a loop")
parser.add_option("", "--vol-create-fail", action="store_true",
dest="vol_fail",
help="Create a volume that will fail at the allocate "
"stage")
options, ignore = parser.parse_args()
if options.vol_create:
cmd_vol_create()
elif options.vol_create_info:
cmd_vol_poll()
elif options.vol_fail:
cmd_vol_fail()
else:
parser.print_help()
sys.exit(1)
if __name__ == "__main__":
main()
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list