FYI: initially reported in https://github.com/virt-manager/virt-manager/issues/56 but then I realized that officially BZ is most likely the correct bug-tracker

This behaviour changed due to this commit:

commit c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
Author: Wim ten Have <wim.ten.have>
Date:   Mon Apr 9 20:14:35 2018 +0200

    storage: extend preallocation flags support for qemu-img
    
    This patch adds support to qcow2 formatted filesystem object storage by
    instructing qemu-img to build them with preallocation=falloc whenever the
    XML described storage <allocation> matches its <capacity>.  For all other
    cases the filesystem stored objects are built with preallocation=metadata.
    
    Signed-off-by: Wim ten Have <wim.ten.have>
    Signed-off-by: Michal Privoznik <mprivozn>


This is a semantic change, so should not have been allowed.

It has been 1 & 1/2 years already, but I'm wondering if we should none the less revert it and instead add some other way to declare which preallocation strategy to use.

Thanks Daniel,
yes that is the patch I was referring to.

If you really think reverting it in libvirt is the right path I'm fine following you on that and keep virt-manager untouched in that regard.
But as you say it is in the field for quite a while and other than this report I haven't seen any.
Therefore I wasn't going to suggest a revert, after that much time it almost is a semantic change "again".

Furthermore the reasoning to add it back in [1] was with virt-inst / virt -manager in mind and the exact definition in [2] IMHO is a bit weak for this particular case.
That was the reason I asked for guidance from virt-managers POV first.

If we end up reverting the change we might consider modifying the text in [2] to be more clear what is (expected) to happen if allocation==capacity.

[1]: https://www.redhat.com/archives/libvir-list/2018-April/msg00130.html
[2]: https://libvirt.org/formatstorage.html#StorageVol

Looking at the launchpad bug I see the original complaint was the disk creation was slow. This is somewhat surprising to me. falloc should be fast on any mainstream, modern filesystem as it merely has to write metadata in the FS to reserve the new blocks.  I don't see any report of what host filesystem was in use when the user saw > 1 minute disk allocation. It would be worth investigating that aspect more, because my expectation is that falloc should be negligible in time.

Hi Daniel,
yes falloc should be fast in many cases, but also isn't in others.

The reporter used this directly on a ZFS system.
I used to recreate the slowness on stacked containers which is EXT4 -> ZFS-image file -> LXD container -> libvirt.
So far it seems we can assume that ZFS is slow in this use case.

I agree that on ext4 and tmpfs this is <0.02 real time.
So to face the slowdown might have a ZFS component.

Yes, it seems fallocate on ZFS is problematic:

  https://github.com/zfsonlinux/zfs/issues/326

With that in mind, the right answer for ZFS is probably for virt-manager/virt-install to NOT ask for any preallocation at all in the ZFS case.

The fun of course is how does virt-manager know whether the image it is about to create is on ZFS or not.  This might point to an RFE for storage pools to report whether a given storage pool instance is capable of efficiently preallocating or not.

Yep Daniel, just found the same discussion a few minutes ago and updated the Ubuntu bug.

While correct, your suggestion of checking pool features for this seems very complex for the trivial issue that we see atm.

Maybe (only maybe) virt-manager would want to sparse-alloc always and could then change to not request allocation==capacity.
From most common virt-manager use cases I know of sparse would make more sense and be faster (at creation).
I'd expect non-sparse to be a special case that maybe gets a way to be specified if really needed.

And also on the libvirt side we still might consider reverting the commit that broke behavior which would render all the effort to flag those features at pools not needed (at least not for this).

Creating non-sparse images is a good thing as a default, because behaviour when sparse images fail on disk full is not transparent to users.

Ok, but maybe virt-manager should then get a switch to only create it as sparse if the user wants it to be fast.
Many people use virt-manager to quickly test things, so I'd assume often disks are created, only partially used for a very short time and then thrown away.

But I agree in general and the additional fact that the "pain" only comes in if this is also on ZFS makes it maybe too special to be of high importance.

I think the discussion above summarizes this for virt-manager well.
I'll lower the prio of the remaining task for virt-manager which is "add a UI switch to request creating images as sparse file"


@Daniel - for libvirt do you still consider reverting the changes around c9ec7088 as it was a semantic change?
And if so would you want an extra bug or ML post for it?
Or has your opinion changed due to the discussion we had here?

I'm honestly not sure about libvirt side. I think it is worth a mail on the list to discuss it more broadly.

FYI - Discussion for libvirt started in https://www.redhat.com/archives/libvir-list/2019-October/msg00467.html

You may just help the user making an informed decision with all the pros and cons explained in the user interface. Such behavior, common to other virtualization solutions, was beautifully fulfilled before being removed with this commit: https://github.com/virt-manager/virt-manager/commit/15a6a7e2105440df528f75c4df4d2471df28bd1e#diff-03b5bf10a38d88f4a09a14e211c23e05. Every time I create a new virtual machine I need to use the command line to generate a dynamically allocated storage, which is not good.

*** Bug 1789853 has been marked as a duplicate of this bug. ***

I think this was fixed in libvirt upstream, please reopen and correct me if I'm wrong

commit 81a3042a12c7c06adc8e95264b6143b2eeb4953f
Author: Pavel Hrdina <phrdina>
Date:   Tue Aug 25 15:09:53 2020 +0200

    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 <phrdina>
    Reviewed-by: Michal Privoznik <mprivozn>

Hi Cole,
thanks for the hint on that fix.
I have applied and tried it but it still uses falloc for the allocation calls from virt-manager.

I have traced a -O0 build in libvirt and see this:

Thread 6 "rpc-worker" hit Breakpoint 4, storageBackendCreateQemuImgOpts (encinfo=0x0, opts=0x7fd1791102a0, info=0x7fd179110380) at ../../../src/storage/storage_util.c:712

(gdb) p *info
$3 = {format = 14, type = 0x7fd17ef920f0 "qcow2", inputType = 0x0, path = 0x7fd164018440 "/var/lib/libvirt/images/ubuntu20.04-l.qcow2", size_arg = 2097152, allocation = 2097152, 
  encryption = false, preallocate = true, compat = 0x7fd16400deb0 "1.1", features = 0x7fd16400e170, nocow = false, backingPath = 0x0, backingFormat = 0, inputPath = 0x0, 
  inputFormatStr = 0x0, inputFormat = 0, secretAlias = 0x0}

And in particular:
(gdb) p info->preallocate
$4 = true
(gdb) p info->size_arg
$5 = 2097152
(gdb) p info->allocation
$6 = 2097152

Due to that the following
712	    if (info->preallocate) {
713	        if (info->size_arg > info->allocation)
714	            virBufferAddLit(&buf, "preallocation=metadata,");
715	        else
716	            virBufferAddLit(&buf, "preallocation=falloc,");

Still decides for falloc as the values are now (due to the fix) equal but not greater than.

Per the description of the initial patch: "build them with preallocation=falloc whenever the XML described storage <allocation> matches its <capacity>" I wonder if we should change the libvirt code to something like:

 if (info.preallocate) {
     if (info.size_arg == info.allocation)
         virBufferAddLit(&buf, "preallocation=falloc,");
     else
         virBufferAddLit(&buf, "preallocation=metadata,");
 }

That code would then work and read exactly as the description did.

P.S. reopening the case

Hi,
as outlined above the behavior is still wrong.
The patch suggested in [1] will fix the behavior of libvirt to what it was meant to be when preallocation=falloc was added.

But now virt-manager comes into play, the XML sent by virt-manager is exactly matching the definition of the use case to use falloc.
So virt-manager triggered storage request will continue to use falloc despite libvirt being "fixed to how it was intended".

The question now is should we consider changing the XML virt-manager submits so that we are back at the much faster preallocation=metadata mode?

[1]: https://www.redhat.com/archives/libvir-list/2020-September/msg00105.html

If you have followed the mailing list discussion, this might be:
- stay as-is in virt-manager
- improve libvirt to explicitly select the prealloc mode (optional)
- zfs got a fix to no more surface such an unexpected waiting time (but in to be released 2.0)

The discussion isn't over but I wanted to summarize here as well for everyone just reading this bug now or later.

Okay now that I actually look into this and wrap my head around it, virt-manager should change.
See this previously mentioned change: https://github.com/virt-manager/virt-manager/commit/15a6a7e2105440df528f75c4df4d2471df28bd1e

The idea behind virt-manager's default was if user selected 'raw' for disk images, assume the user wants
to maximize performance, so fully allocate the disk.

qcow2 didn't support anything except sparse, so the sparse=True vs sparse=False made no difference.
So we always set sparse=False

Then qcow2 grows non-sparse support, and virt-manager is suddenly defaulting to it. That's a pretty big semantic
change. Even if falloc is fast enough in most cases, it is still claiming all the storage up front. Which
is not the historically intended behavior.

So I pushed a change to virt-manager to default to only use non-sparse for raw format by default.
That should side step this issue

I'll be pushing a new release with the fix sometime this coming week

commit ba08f84b3408744e9aa9763d100e8aa217c1f5ff
Author: Cole Robinson <crobinso>
Date:   Sat Sep 19 18:06:45 2020 -0400

    addstorage: Return to using qcow2 sparse by default