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@xxxxxxxxx>
> ---
> 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
