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

Re: [PATCH] storage: only fallocate when allocation matches capacity



On Wed, Sep 02, 2020 at 03:58:14PM +0200, Christian Ehrhardt wrote:
> In c9ec7088 "storage: extend preallocation flags support for qemu-img"
> the option to fallocate was added and meant to be active when (quote):
> "the XML described storage <allocation> matches its <capacity>"
> 
> Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation"
> the compared allocation size was an order of magnitude too small, but still
> it does use fallocate too often unless capacity>allocation.
> 
> This change fixes the comparison to match the intended description
> of the feature.
> 
> Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
> Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105
> 
> Signed-off-by: Christian Ehrhardt <christian ehrhardt canonical com>
> ---
>  src/storage/storage_util.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index cf82ea0a87..85bed76863 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -710,10 +710,10 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo,
>          virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias);
>  
>      if (info->preallocate) {
> -        if (info->size_arg > info->allocation)
> -            virBufferAddLit(&buf, "preallocation=metadata,");
> -        else
> +        if (info->size_arg == info->allocation)
>              virBufferAddLit(&buf, "preallocation=falloc,");
> +        else
> +            virBufferAddLit(&buf, "preallocation=metadata,");
>      }

This is still wrong, as preallocation=falloc is inside the
"info->preallocate" check.

The "info->preallocate" field is set if VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA
was passed as a flag to virStorageVolCreate. By its very name that flag only
refers to metadata preallocation, not payload.

IOW Pre-allocating  image payload should not be dependendant on the
VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA flag, it should be driven
by the capacity/allocation values in the XML, as it done for raw
volumes.

I regret that we ever used "allocation" in the XML as a sign to preallocate.
We should really have treated it as an output only field, and had an explicit
flag.

It isn't too late for us to introduce two new flags:

  VIR_STORAGE_VOL_CREATE_PREALLOC_PAYLOAD
  VIR_STORAGE_VOL_CREATE_PREALLOC_NONE

If either of the three PREALLOC flags are present, we should declare that
"allocation" in the XML is ignored. So at least we'll have sane behaviour
for apps that pass the flags at that point.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


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