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.ehrha...@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 :|
