[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/2] storage: allow metadata preallocation when creating qcow2 images



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]