[libvirt] [PATCH v1 1/2] storage: extend preallocation flags support for qemu-img

[Michal Privoznik]

On 04/03/2018 04:14 PM, Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have at oracle.com>
> 
> This patch adds support to qcow2 formatted filesystem object storage by
> instructing qemu-img to build them with preallocation=falloc whenever the
> XML described storage <allocation> matches its <capacity>.  For all other
> cases the filesystem stored objects are built with preallocation=metadata.
> 
> Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> ---
>  include/libvirt/libvirt-storage.h |  5 ++++-
>  src/storage/storage_util.c        | 21 +++++++++++++++++----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index 413d9f6c4..2f22b388c 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -337,8 +337,11 @@ const char*             virStorageVolGetName            (virStorageVolPtr vol);
>  const char*             virStorageVolGetKey             (virStorageVolPtr vol);
>  
>  typedef enum {
> +    VIR_STORAGE_VOL_CREATE_PREALLOC_NONE     = 0 << 0,

This is okay.

>      VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA = 1 << 0,
> -    VIR_STORAGE_VOL_CREATE_REFLINK = 1 << 1, /* perform a btrfs lightweight copy */
> +    VIR_STORAGE_VOL_CREATE_PREALLOC_FALLOC   = 1 << 1,
> +    VIR_STORAGE_VOL_CREATE_PREALLOC_FULL     = 1 << 2,
> +    VIR_STORAGE_VOL_CREATE_REFLINK           = 1 << 3, /* perform a btrfs lightweight copy */

This is not. Imagine there's a mgmt application already written and
compiled which calls:

virStorageVolCreateXML(flags = VIR_STORAGE_VOL_CREATE_REFLINK);

Because it is already compiled it is effectively calling:

virStorageVolCreateXML(flags = 1);

and everything works. However, if this change would be merged, the mgmt
application would be still making the same call but now it would have
different semantic, because you are changing the numbering. So
effectively mgmt app would be calling

virStorageVolCreateXML(flags = VIR_STORAGE_VOL_CREATE_PREALLOC_FALLOC)

which is obviously wrong. We can not expect mgmt applications to be
recompiled every time there's a new release of libvirt.

>  } virStorageVolCreateFlags;
>  
>  virStorageVolPtr        virStorageVolCreateXML          (virStoragePoolPtr pool,
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index b4aed0f70..7728fb63e 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -852,7 +852,7 @@ struct _virStorageBackendQemuImgInfo {
>      const char *path;
>      unsigned long long size_arg;
>      bool encryption;
> -    bool preallocate;
> +    unsigned int preallocate;
>      const char *compat;
>      virBitmapPtr features;
>      bool nocow;
> @@ -884,8 +884,15 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc,
>                                virStorageFileFormatTypeToString(info.backingFormat));
>          if (info.encryption)
>              virBufferAddLit(&buf, "encryption=on,");
> -        if (info.preallocate)
> +
> +        /* Handle various types of file-system storage pre-allocate sets.
> +         */
> +        if (info.preallocate & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA)
>              virBufferAddLit(&buf, "preallocation=metadata,");
> +        else if (info.preallocate & VIR_STORAGE_VOL_CREATE_PREALLOC_FALLOC)
> +            virBufferAddLit(&buf, "preallocation=falloc,");
> +        else if (info.preallocate & VIR_STORAGE_VOL_CREATE_PREALLOC_FULL)
> +            virBufferAddLit(&buf, "preallocation=full,");
>      }
>  
>      if (info.nocow)
> @@ -1183,7 +1190,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>          .format = vol->target.format,
>          .path = vol->target.path,
>          .encryption = vol->target.encryption != NULL,
> -        .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
> +        .preallocate = VIR_STORAGE_VOL_CREATE_PREALLOC_NONE,
>          .compat = vol->target.compat,
>          .features = vol->target.features,
>          .nocow = vol->target.nocow,
> @@ -1192,7 +1199,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>      };
>      virStorageEncryptionInfoDefPtr enc = NULL;
>  
> -    virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
> +    if (flags) {
> +        info.preallocate = (vol->target.capacity == vol->target.allocation) ?
> +            VIR_STORAGE_VOL_CREATE_PREALLOC_FALLOC : VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> +        virCheckFlags((VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA|
> +                       VIR_STORAGE_VOL_CREATE_PREALLOC_FALLOC|
> +                       VIR_STORAGE_VOL_CREATE_PREALLOC_FULL), NULL);
> +    }

virCheckFlags() should be checked outside of this if() statement.

But hold on. How can PREALLOC_FULL be used? How can any flag be
specified in the first place? I mean

virStorageVolCreateXML(flags = VIR_STORAGE_VOL_CREATE_PREALLOC_FULL)

boils down to

virStorageBackendCreateQemuImgCmdFromVol(flags =
VIR_STORAGE_VOL_CREATE_PREALLOC_FULL)

which completely ignores the flag. This is wrong.

Also, storagevolxml2argvtest is failing after this change. See 'make check'.

Michal