Re: [PATCH 1/2] Add discard_no_unref option for qcow2 images

On Tue, Jun 06, 2023 at 11:54:40 +0200, Jean-Louis Dupond wrote: > Qemu 8.1.0 will add discard_no_unref option for qcow2 images. > When this option is enabled (default=false), then it will no longer > unreference clusters when guest does a discard, but it will just free > the blocks (useful for incremental backups for example) and pass the > discard to the lower layer. > > This was implemented to avoid fragmentation within the qcow2 image. > > Signed-off-by: Jean-Louis Dupond <jean-louis(a)dupond.be&gt; > --- > docs/formatdomain.rst | 6 +++ > src/conf/domain_conf.c | 8 +++ > src/conf/domain_conf.h | 1 + > src/conf/domain_validate.c | 15 ++++++ > src/conf/schemas/domaincommon.rng | 8 +++ > src/conf/storage_source_conf.c | 1 + > src/conf/storage_source_conf.h | 1 + > src/qemu/qemu_block.c | 11 ++--- > src/qemu/qemu_domain.c | 1 + > src/qemu/qemu_driver.c | 4 +- > src/vz/vz_utils.c | 6 +++ > tests/qemusecuritytest.c | 1 + > .../disk-discard_no_unref.x86_64-latest.args | 39 +++++++++++++++ > .../disk-discard_no_unref.xml | 39 +++++++++++++++ > tests/qemuxml2argvtest.c | 1 + > .../disk-discard_no_unref.x86_64-latest.xml | 49 +++++++++++++++++++ > tests/qemuxml2xmltest.c | 1 + > 17 files changed, 185 insertions(+), 7 deletions(-) > create mode 100644 tests/qemuxml2argvdata/disk-discard_no_unref.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/disk-discard_no_unref.xml > create mode 100644 tests/qemuxml2xmloutdata/disk-discard_no_unref.x86_64-latest.xml > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index c3526439bf..6e89f8e01f 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -3284,6 +3284,12 @@ paravirtualized driver is specified via the ``disk`` element. > format driver of the ``qemu`` hypervisor can be controlled via the > ``max_size`` subelement (see example below). > > + The optional ``discard_no_unref`` attribute can be set to control the way > + the ``qemu`` hypervisor handles guest discard commands inside the qcow2 > + image. When enabled, a discard from within the guest will not cause the > + qcow2 image to remove the cluster references, but will still send the > + discard command to its lower layer. :since:`Since 9.5.0` I'm not quite sure that I understand from this description what it actually does. I understand that if a discard request from the guest gets to qemu, the QCOW2 image keeps the reference, but the backing image unreferences it? What would this be useful for?

> + > In the majority of cases the default configuration used by the hypervisor > is sufficient so modifying this setting should not be necessary. For > specifics on how the metadata cache of ``qcow2`` in ``qemu`` behaves refer [...] > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c > index 80d6a2ffd9..34508b1474 100644 > --- a/src/conf/domain_validate.c > +++ b/src/conf/domain_validate.c > @@ -379,6 +379,12 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk) > return -1; > } > > + if (disk->discard_no_unref) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("discard_no_unref is not supported with vhostuser disk")); > + return -1; > + } [1] > + > /* Unsupported driver elements */ > > if (disk->src->metadataCacheMaxSize > 0) { > @@ -921,6 +927,15 @@ virDomainDiskDefValidate(const virDomainDef *def, > return -1; > } > > + if (disk->discard_no_unref == VIR_TRISTATE_SWITCH_ON) { > + if (disk->src->readonly) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("discard_no_unref is not compatible with read-only disk '%1$s'"), > + disk->dst); > + return -1; > + } > + } > + Validation that the flag is used with qcow2 is missing. If you add that [1] becomes pointless as vhost-user doesn't work with 'qcow2'

> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng > index c1725bb511..0b49fbcfeb 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -2514,6 +2514,9 @@ > <optional> > <ref name="detect_zeroes"/> > </optional> > + <optional> > + <ref name="discard_no_unref"/> > + </optional> You can declare the attribute directly here, having a definition for it doesn't make too much sense.

> diff --git a/tests/qemuxml2argvdata/disk-discard_no_unref.xml b/tests/qemuxml2argvdata/disk-discard_no_unref.xml > new file mode 100644 > index 0000000000..34d61b9879 > --- /dev/null > +++ b/tests/qemuxml2argvdata/disk-discard_no_unref.xml > @@ -0,0 +1,39 @@ > +<domain type='qemu'> > + <name>test</name> > + <uuid>92d7a226-cfae-425b-a6d3-00bbf9ec5c9e</uuid> > + <memory unit='KiB'>1048576</memory> > + <currentMemory unit='KiB'>1048576</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='x86_64' machine='pc'>hvm</type> > + <boot dev='cdrom'/> > + <boot dev='hd'/> > + <bootmenu enable='yes'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>restart</on_crash> > + <devices> > + <emulator>/usr/bin/qemu-system-x86_64</emulator> > + <!-- For this disk, intentionally stress parser resilience to > + atypical ordering --> > + <disk device='disk'> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > + <source file='/var/lib/libvirt/images/f14.img'/> > + <target dev='vda' bus='virtio'/> > + <driver discard='unmap' discard_no_unref='on' name='qemu' type='qcow2'/> > + </disk> > + <disk type='file' device='cdrom'> > + <driver discard='ignore'/> > + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> > + <target dev='hdc' bus='ide'/> > + <readonly/> > + <address type='drive' controller='0' bus='1' target='0' unit='0'/> > + </disk> Please drop this irrelevant disk