[libvirt] [PATCH v3 1/2] storage: allow metadata preallocation when creating qcow2 images
Michal Privoznik
mprivozn at redhat.com
Wed Dec 5 16:29:08 UTC 2012
On 05.12.2012 11:48, Ján Tomko wrote:
> Add VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA flag to virStorageVolCreateXML
> and virStorageVolCreateXMLFrom. This flag requests metadata
> preallocation when creating/cloning qcow2 images, resulting in creating
> a sparse file with qcow2 metadata. It has only slightly larger disk usage
> compared to new image with no allocation, but offers higher performance.
> ---
> include/libvirt/libvirt.h.in | 4 +++
> src/libvirt.c | 16 ++++++++++--
> src/storage/storage_backend.c | 46 ++++++++++++++++++++++++++-----------
> src/storage/storage_backend.h | 3 +-
> src/storage/storage_backend_fs.c | 16 ++++++++-----
> src/storage/storage_driver.c | 6 ++--
> 6 files changed, 64 insertions(+), 27 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index a19f37a..17804ca 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2906,6 +2906,10 @@ virStorageVolPtr virStorageVolLookupByPath (virConnectPtr conn,
> const char* virStorageVolGetName (virStorageVolPtr vol);
> const char* virStorageVolGetKey (virStorageVolPtr vol);
>
> +typedef enum {
> + VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA = 1 << 0,
> +} virStorageVolCreateFlags;
> +
> virStorageVolPtr virStorageVolCreateXML (virStoragePoolPtr pool,
> const char *xmldesc,
> unsigned int flags);
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 4b7baab..6a7a817 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -13382,11 +13382,16 @@ virStorageVolGetKey(virStorageVolPtr vol)
> * virStorageVolCreateXML:
> * @pool: pointer to storage pool
> * @xmlDesc: description of volume to create
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStorageVolCreateFlags
> *
> * Create a storage volume within a pool based
> * on an XML description. Not all pools support
> - * creation of volumes
> + * creation of volumes.
> + *
> + * Since 1.0.1 VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA
> + * in flags can be used to get higher performance with
> + * qcow2 image files which don't support full preallocation,
> + * by creating a sparse image file with metadata.
The 'Since 1.0.1' is not needed as users can simply 'git blame
include/libvirt/libvirt.h.in'. Moreover, we are not using it anywhere
among code. I'd reword this as 'The VIR_STORAGE_..._METADATA flag can be
used to ...'.
> *
> * Returns the storage volume, or NULL on error
> */
> @@ -13433,13 +13438,18 @@ error:
> * @pool: pointer to parent pool for the new volume
> * @xmlDesc: description of volume to create
> * @clonevol: storage volume to use as input
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStorageVolCreateFlags
> *
> * Create a storage volume in the parent pool, using the
> * 'clonevol' volume as input. Information for the new
> * volume (name, perms) are passed via a typical volume
> * XML description.
> *
> + * Since 1.0.1 VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA
> + * in flags can be used to get higher performance with
> + * qcow2 image files which don't support full preallocation,
> + * by creating a sparse image file with metadata.
> + *
Likewise.
> * Returns the storage volume, or NULL on error
> */
> virStorageVolPtr
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 83c4755..083028d 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -668,8 +668,11 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> virCommandPtr cmd = NULL;
> bool do_encryption = (vol->target.encryption != NULL);
> unsigned long long int size_arg;
> + bool preallocate = false;
>
> - virCheckFlags(0, -1);
> + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1);
> +
> + preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA);
>
> const char *type = virStorageFileFormatTypeToString(vol->target.format);
> const char *backingType = vol->backingStore.path ?
> @@ -697,11 +700,23 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> inputvol->target.format);
> return -1;
> }
> + if (preallocate && vol->target.format != VIR_STORAGE_FILE_QCOW2) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata preallocation only available with qcow2"));
> + return -1;
> + }
>
> if (vol->backingStore.path) {
> int accessRetCode = -1;
> char *absolutePath = NULL;
>
> + if (preallocate) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata preallocation conflicts with backing"
> + " store"));
> + return -1;
> + }
> +
> /* XXX: Not strictly required: qemu-img has an option a different
> * backing store, not really sure what use it serves though, and it
> * may cause issues with lvm. Untested essentially.
> @@ -796,14 +811,15 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type,
> inputPath, vol->target.path, NULL);
>
> - if (do_encryption) {
> - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) {
> - virCommandAddArgList(cmd, "-o", "encryption=on", NULL);
> - } else {
> - virCommandAddArg(cmd, "-e");
> - }
> + if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS &&
> + (do_encryption || preallocate)) {
> + virCommandAddArg(cmd, "-o");
> + virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "",
> + (do_encryption && preallocate) ? "," : "",
> + preallocate ? "preallocation=metadata" : "");
> + } else if (do_encryption) {
> + virCommandAddArg(cmd, "-e");
> }
> -
> } else if (vol->backingStore.path) {
> virCommandAddArgList(cmd, "create", "-f", type,
> "-b", vol->backingStore.path, NULL);
> @@ -840,12 +856,14 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> vol->target.path, NULL);
> virCommandAddArgFormat(cmd, "%lluK", size_arg);
>
> - if (do_encryption) {
> - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) {
> - virCommandAddArgList(cmd, "-o", "encryption=on", NULL);
> - } else {
> - virCommandAddArg(cmd, "-e");
> - }
> + if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS &&
> + (do_encryption || preallocate)) {
> + virCommandAddArg(cmd, "-o");
> + virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "",
> + (do_encryption && preallocate) ? "," : "",
> + preallocate ? "preallocation=metadata" : "");
> + } else if (do_encryption) {
> + virCommandAddArg(cmd, "-e");
> }
> }
>
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 29cad9d..c991015 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -37,7 +37,8 @@ typedef int (*virStorageBackendStopPool)(virConnectPtr conn, virStoragePoolObjPt
> typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags);
>
> typedef int (*virStorageBackendBuildVol)(virConnectPtr conn,
> - virStoragePoolObjPtr pool, virStorageVolDefPtr vol);
> + virStoragePoolObjPtr pool, virStorageVolDefPtr vol,
> + unsigned int flags);
> 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);
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 4e6ebbf..7a54ead 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -1045,7 +1045,8 @@ static int
> _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> virStoragePoolObjPtr pool,
> virStorageVolDefPtr vol,
> - virStorageVolDefPtr inputvol)
> + virStorageVolDefPtr inputvol,
> + unsigned int flags)
> {
> virStorageBackendBuildVolFrom create_func;
> int tool_type;
> @@ -1078,7 +1079,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> return -1;
> }
>
> - if (create_func(conn, pool, vol, inputvol, 0) < 0)
> + if (create_func(conn, pool, vol, inputvol, flags) < 0)
> return -1;
> return 0;
> }
> @@ -1091,8 +1092,11 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> static int
> virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> virStoragePoolObjPtr pool,
> - virStorageVolDefPtr vol) {
> - return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL);
> + virStorageVolDefPtr vol,
> + unsigned int flags) {
> + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1);
> +
> + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL, flags);
> }
>
> /*
> @@ -1105,9 +1109,9 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn,
> virStorageVolDefPtr inputvol,
> unsigned int flags)
> {
> - virCheckFlags(0, -1);
> + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1);
>
> - return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol);
> + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol, flags);
> }
>
> /**
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 3cb2312..c567fff 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1348,7 +1348,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
> virStorageVolDefPtr voldef = NULL;
> virStorageVolPtr ret = NULL, volobj = NULL;
>
> - virCheckFlags(0, NULL);
> + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>
> storageDriverLock(driver);
> pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
> @@ -1425,7 +1425,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
> voldef->building = 1;
> virStoragePoolObjUnlock(pool);
>
> - buildret = backend->buildVol(obj->conn, pool, buildvoldef);
> + buildret = backend->1nl(obj->conn, pool, buildvoldef, flags);
>
> storageDriverLock(driver);
> virStoragePoolObjLock(pool);
> @@ -1473,7 +1473,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
> virStorageVolPtr ret = NULL, volobj = NULL;
> int buildret;
>
> - virCheckFlags(0, NULL);
> + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>
> storageDriverLock(driver);
> pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
>
Otherwise looking good. ACK.
Michal
More information about the libvir-list
mailing list