Merge ~paelzer/ubuntu/+source/libvirt:lp-1847105-allocation-falloc-instead-metadata-GROOVY into ubuntu/+source/libvirt:ubuntu/groovy-devel

[Christian Ehrhardt ]

[Christian Ehrhardt  (paelzer)]

PPA: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4238

[Richard Laager (rlaager)]

If I'm understanding this correctly, the old approach was to use preallocation=falloc when capacity (size_arg) <= allocation, and now you've made the condition capacity == allocation. So the difference is that previously, if capacity < allocation, it would use falloc but will now use metadata.

Is it possible for capacity to be < allocation? If not, this is irrelevant.

But if it is possible for capacity < allocation, the change seems wrong. If I am overprovisioning, it seems like it would be desirable to use fallocate() to do that. That seems like a weird thing to want to do for a VM disk, but it's not unusual to do in general; fallocate(2) has the FALLOC_FL_KEEP_SIZE specifically for that mode.

[Christian Ehrhardt  (paelzer)]

Testing turned out that this change alone doesn't help without a matching virt-manager change.
Self-rejecting the MP for now.

[Christian Ehrhardt  (paelzer)]

HI Richard, I was seeing your comment only now.
In the past it was "always metadata", then the change we try to fix came along and defined it
as "if capacity == allocation, then fallocate, otherwise metadata" (well up until my patch it didn't do what the commit meant to achieve, but that was the thought.

I was mostly looking at the wording vs the code and tried to fix it.
But revisiting it you might be right in that it might be an error on it's own if capacity<allocation.

Possible conditions:
- capacity==allocation now and before my change falloc
- capacity>allocation now and before my change metadata
- capacity<allocation (seems invalid anyway)

We are back at myself wanting virt-manager to submit a different XML (less allocation) to get metadata - I've already requested that on the upstream bug for it.

I'll carry your thoughts forward, maybe the wording in the initial commit was wrong?
We will see.
If you want to continue to discuss this it would be great to do so on the ML:
https://www.redhat.com/archives/libvir-list/2020-September/msg00105.html

Unmerged commits

d80ab31... by Christian Ehrhardt  on 2020-09-01

changelog: fix slow disk allocation (LP: #1847105)

Signed-off-by: Christian Ehrhardt <email address hidden>

5f881dd... by Christian Ehrhardt  on 2020-09-01

* fix slow disk allocation (LP: #1847105)

- d/p/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
  fix allocation size
- d/p/u/lp-1847105-storage-only-fallocate-when-allocation-matches.patch:
  fix comparison

Signed-off-by: Christian Ehrhardt <email address hidden>

diff --git a/debian/changelog b/debian/changelog
index 90e66db..cdcbd55 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+libvirt (6.6.0-1ubuntu3) groovy; urgency=medium
+
+  * fix slow disk allocation (LP: #1847105)
+    - d/p/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
+      fix allocation size
+    - d/p/u/lp-1847105-storage-only-fallocate-when-allocation-matches.patch:
+      fix comparison
+
+ -- Christian Ehrhardt <[e-mail]>  Tue, 01 Sep 2020 09:36:24 +0200
+
 libvirt (6.6.0-1ubuntu2) groovy; urgency=medium
 
   * d/p/u/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch: avoid clashes
diff --git a/debian/patches/series b/debian/patches/series
index e8a58b2..11f8777 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -42,3 +42,5 @@ ubuntu-aa/apparmor-profiles-are-meant-to-allow-adding-permanen.patch
 ubuntu-aa/apparmor-allow-unmounting-.dev-entries.patch
 ubuntu/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch
 ubuntu-aa/lp-1892736-apparmor-allow-libvirtd-to-call-virtiofsd.patch
+ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
+ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch
diff --git a/debian/patches/ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch b/debian/patches/ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch
 new file mode 100644
index 0000000..024947e
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch
@@ -0,0 +1,51 @@
+From da88ddbe16e4eb02734bd8ced3b27b3a11538445 Mon Sep 17 00:00:00 2001
+From: Christian Ehrhardt <[e-mail]>
+Date: Wed, 2 Sep 2020 15:00:45 +0200
+Subject: [PATCH] storage: only fallocate when allocation matches capacity
+
+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 <[e-mail]>
+
+Forwarded: https://www.redhat.com/archives/libvir-list/2020-September/msg00105.html
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1847105
+Last-Update: 2020-09-03
+
+---
+ 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,");
+     }
+ 
+     if (info->nocow)
+-- 
+2.28.0
+
diff --git a/debian/patches/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch b/debian/patches/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
 new file mode 100644
index 0000000..480eee0
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
@@ -0,0 +1,41 @@
+From 81a3042a12c7c06adc8e95264b6143b2eeb4953f Mon Sep 17 00:00:00 2001
+From: Pavel Hrdina <[e-mail]>
+Date: Tue, 25 Aug 2020 15:09:53 +0200
+Subject: [PATCH] storage_util: fix qemu-img sparse allocation
+
+Commit <c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9> introduced a support
+to fully allocate qcow2 images when <allocation> matches <capacity> but
+it doesn't work as expected.
+
+The issue is that info.size_arg is in KB but the info.allocation
+introduced by the mentioned commit is in B. This results in using
+"preallocation=falloc," in cases where "preallocation=metadata," should
+be used.
+
+Signed-off-by: Pavel Hrdina <[e-mail]>
+Reviewed-by: Michal Privoznik <[e-mail]>
+
+Origin: upstream, https://libvirt.org/git/?p=libvirt.git;a=commit;h=81a3042a12c7c06
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1847105
+Last-Update: 2020-09-01
+
+---
+ src/storage/storage_util.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
+index f7c09e3375..fcecedbc3a 100644
+--- a/src/storage/storage_util.c
++++ b/src/storage/storage_util.c
+@@ -1044,7 +1044,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
+         .type = NULL,
+         .inputType = NULL,
+         .path = vol->target.path,
+-        .allocation = vol->target.allocation,
++        .allocation = VIR_DIV_UP(vol->target.allocation, 1024),
+         .encryption = !!vol->target.encryption,
+         .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
+         .compat = vol->target.compat,
+-- 
+2.28.0
+