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

2018-04-09 Thread Wim ten Have
On Wed, 4 Apr 2018 13:31:39 +0200
Michal Privoznik  wrote:

> On 04/03/2018 04:14 PM, Wim Ten Have wrote:
> > From: Wim ten Have 
> > 
> > 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  matches its .  For all other
> > cases the filesystem stored objects are built with preallocation=metadata.
> > 
> > Signed-off-by: Wim ten Have 
...
> >  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.
...

  right, let me send v2. (applying and propagating XML target.sparse)

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

  ?! ... sure I ran 'make check'.  Anyways ... corrections under v2 coming 
forth.

Rgds,
- Wim.

> Michal
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


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

2018-04-04 Thread Michal Privoznik
On 04/03/2018 04:14 PM, Wim Ten Have wrote:
> From: Wim ten Have 
> 
> 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  matches its .  For all other
> cases the filesystem stored objects are built with preallocation=metadata.
> 
> Signed-off-by: Wim ten Have 
> ---
>  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;
>  
>  virStorageVolPtrvirStorageVolCreateXML  (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 =