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 Pending
git-ubuntu developers Pending
Review via email:
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:

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
diff --git a/debian/changelog b/debian/changelog
index 90e66db..cdcbd55 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
1libvirt (6.6.0-1ubuntu3) groovy; urgency=medium
3 * fix slow disk allocation (LP: #1847105)
4 - d/p/ubuntu/lp-1847105-storage_util-fix-qemu-img-sparse-allocation.patch
5 fix allocation size
6 - d/p/u/lp-1847105-storage-only-fallocate-when-allocation-matches.patch:
7 fix comparison
9 -- Christian Ehrhardt <> Tue, 01 Sep 2020 09:36:24 +0200
1libvirt (6.6.0-1ubuntu2) groovy; urgency=medium11libvirt (6.6.0-1ubuntu2) groovy; urgency=medium
3 * d/p/u/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch: avoid clashes13 * 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
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
45new file mode 10064447new 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 @@
1From da88ddbe16e4eb02734bd8ced3b27b3a11538445 Mon Sep 17 00:00:00 2001
2From: Christian Ehrhardt <>
3Date: Wed, 2 Sep 2020 15:00:45 +0200
4Subject: [PATCH] storage: only fallocate when allocation matches capacity
6In c9ec7088 "storage: extend preallocation flags support for qemu-img"
7the option to fallocate was added and meant to be active when (quote):
8"the XML described storage <allocation> matches its <capacity>"
10Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation"
11the compared allocation size was an order of magnitude too small, but still
12it does use fallocate too often unless capacity>allocation.
14This change fixes the comparison to match the intended description
15of the feature.
17Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
21Signed-off-by: Christian Ehrhardt <>
25Last-Update: 2020-09-03
28 src/storage/storage_util.c | 6 +++---
29 1 file changed, 3 insertions(+), 3 deletions(-)
31diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
32index cf82ea0a87..85bed76863 100644
33--- a/src/storage/storage_util.c
34+++ b/src/storage/storage_util.c
35@@ -710,10 +710,10 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo,
36 virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias);
38 if (info->preallocate) {
39- if (info->size_arg > info->allocation)
40- virBufferAddLit(&buf, "preallocation=metadata,");
41- else
42+ if (info->size_arg == info->allocation)
43 virBufferAddLit(&buf, "preallocation=falloc,");
44+ else
45+ virBufferAddLit(&buf, "preallocation=metadata,");
46 }
48 if (info->nocow)
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
0new file mode 10064452new 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 @@
1From 81a3042a12c7c06adc8e95264b6143b2eeb4953f Mon Sep 17 00:00:00 2001
2From: Pavel Hrdina <>
3Date: Tue, 25 Aug 2020 15:09:53 +0200
4Subject: [PATCH] storage_util: fix qemu-img sparse allocation
6Commit <c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9> introduced a support
7to fully allocate qcow2 images when <allocation> matches <capacity> but
8it doesn't work as expected.
10The issue is that info.size_arg is in KB but the info.allocation
11introduced by the mentioned commit is in B. This results in using
12"preallocation=falloc," in cases where "preallocation=metadata," should
13be used.
15Signed-off-by: Pavel Hrdina <>
16Reviewed-by: Michal Privoznik <>
18Origin: upstream,;a=commit;h=81a3042a12c7c06
20Last-Update: 2020-09-01
23 src/storage/storage_util.c | 2 +-
24 1 file changed, 1 insertion(+), 1 deletion(-)
26diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
27index f7c09e3375..fcecedbc3a 100644
28--- a/src/storage/storage_util.c
29+++ b/src/storage/storage_util.c
30@@ -1044,7 +1044,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
31 .type = NULL,
32 .inputType = NULL,
33 .path = vol->target.path,
34- .allocation = vol->target.allocation,
35+ .allocation = VIR_DIV_UP(vol->target.allocation, 1024),
36 .encryption = !!vol->target.encryption,
37 .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
38 .compat = vol->target.compat,


People subscribed via source and target branches