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

Proposed by Christian Ehrhardt 
Status: Rejected
Rejected by: Christian Ehrhardt 
Proposed branch: ~paelzer/ubuntu/+source/libvirt:lp-1847105-allocation-falloc-instead-metadata-GROOVY
Merge into: ubuntu/+source/libvirt:ubuntu/groovy-devel
Diff against target: 132 lines (+104/-0)
4 files modified
debian/changelog (+10/-0)
debian/patches/series (+2/-0)
debian/patches/ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch (+51/-0)
debian/patches/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch (+41/-0)
Reviewer Review Type Date Requested Status
Canonical Server packageset reviewers Pending
Canonical Server Team Pending
Ubuntu Server Dev import team Pending
Review via email: mp+390220@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Revision history for this message
Richard Laager (rlaager) wrote :

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.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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 

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

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

5f881dd... by Christian Ehrhardt 

* 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>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 90e66db..cdcbd55 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,13 @@
6+libvirt (6.6.0-1ubuntu3) groovy; urgency=medium
7+
8+ * fix slow disk allocation (LP: #1847105)
9+ - d/p/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
10+ fix allocation size
11+ - d/p/u/lp-1847105-storage-only-fallocate-when-allocation-matches.patch:
12+ fix comparison
13+
14+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Tue, 01 Sep 2020 09:36:24 +0200
15+
16 libvirt (6.6.0-1ubuntu2) groovy; urgency=medium
17
18 * d/p/u/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch: avoid clashes
19diff --git a/debian/patches/series b/debian/patches/series
20index e8a58b2..11f8777 100644
21--- a/debian/patches/series
22+++ b/debian/patches/series
23@@ -42,3 +42,5 @@ ubuntu-aa/apparmor-profiles-are-meant-to-allow-adding-permanen.patch
24 ubuntu-aa/apparmor-allow-unmounting-.dev-entries.patch
25 ubuntu/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch
26 ubuntu-aa/lp-1892736-apparmor-allow-libvirtd-to-call-virtiofsd.patch
27+ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
28+ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch
29diff --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
30new file mode 100644
31index 0000000..024947e
32--- /dev/null
33+++ b/debian/patches/ubuntu/lp-1847105-storage-only-fallocate-when-allocation-matches.patch
34@@ -0,0 +1,51 @@
35+From da88ddbe16e4eb02734bd8ced3b27b3a11538445 Mon Sep 17 00:00:00 2001
36+From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
37+Date: Wed, 2 Sep 2020 15:00:45 +0200
38+Subject: [PATCH] storage: only fallocate when allocation matches capacity
39+
40+In c9ec7088 "storage: extend preallocation flags support for qemu-img"
41+the option to fallocate was added and meant to be active when (quote):
42+"the XML described storage <allocation> matches its <capacity>"
43+
44+Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation"
45+the compared allocation size was an order of magnitude too small, but still
46+it does use fallocate too often unless capacity>allocation.
47+
48+This change fixes the comparison to match the intended description
49+of the feature.
50+
51+Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
52+Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
53+Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105
54+
55+Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
56+
57+Forwarded: https://www.redhat.com/archives/libvir-list/2020-September/msg00105.html
58+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1847105
59+Last-Update: 2020-09-03
60+
61+---
62+ src/storage/storage_util.c | 6 +++---
63+ 1 file changed, 3 insertions(+), 3 deletions(-)
64+
65+diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
66+index cf82ea0a87..85bed76863 100644
67+--- a/src/storage/storage_util.c
68++++ b/src/storage/storage_util.c
69+@@ -710,10 +710,10 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo,
70+ virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias);
71+
72+ if (info->preallocate) {
73+- if (info->size_arg > info->allocation)
74+- virBufferAddLit(&buf, "preallocation=metadata,");
75+- else
76++ if (info->size_arg == info->allocation)
77+ virBufferAddLit(&buf, "preallocation=falloc,");
78++ else
79++ virBufferAddLit(&buf, "preallocation=metadata,");
80+ }
81+
82+ if (info->nocow)
83+--
84+2.28.0
85+
86diff --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
87new file mode 100644
88index 0000000..480eee0
89--- /dev/null
90+++ b/debian/patches/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
91@@ -0,0 +1,41 @@
92+From 81a3042a12c7c06adc8e95264b6143b2eeb4953f Mon Sep 17 00:00:00 2001
93+From: Pavel Hrdina <phrdina@redhat.com>
94+Date: Tue, 25 Aug 2020 15:09:53 +0200
95+Subject: [PATCH] storage_util: fix qemu-img sparse allocation
96+
97+Commit <c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9> introduced a support
98+to fully allocate qcow2 images when <allocation> matches <capacity> but
99+it doesn't work as expected.
100+
101+The issue is that info.size_arg is in KB but the info.allocation
102+introduced by the mentioned commit is in B. This results in using
103+"preallocation=falloc," in cases where "preallocation=metadata," should
104+be used.
105+
106+Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
107+Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
108+
109+Origin: upstream, https://libvirt.org/git/?p=libvirt.git;a=commit;h=81a3042a12c7c06
110+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1847105
111+Last-Update: 2020-09-01
112+
113+---
114+ src/storage/storage_util.c | 2 +-
115+ 1 file changed, 1 insertion(+), 1 deletion(-)
116+
117+diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
118+index f7c09e3375..fcecedbc3a 100644
119+--- a/src/storage/storage_util.c
120++++ b/src/storage/storage_util.c
121+@@ -1044,7 +1044,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
122+ .type = NULL,
123+ .inputType = NULL,
124+ .path = vol->target.path,
125+- .allocation = vol->target.allocation,
126++ .allocation = VIR_DIV_UP(vol->target.allocation, 1024),
127+ .encryption = !!vol->target.encryption,
128+ .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
129+ .compat = vol->target.compat,
130+--
131+2.28.0
132+

Subscribers

People subscribed via source and target branches