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(a)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 :|