[libvirt] [PATCH v2 0/2] Add support for zero-write detection
QEMU supports detect-zeroes option since version 2.1, but we never added support for it in libvirt. If was requested by Vasiliy Tolstov in the list, so I just added it. There are two discussions to be had, optionally. One is to decide whether we should disable detect_zeros='unmap' if discard is not set to 'unmap', but this is getting very hypervisor-specific, so I just documented the behaviour. The other one is the naming. I described why I made the decision for "zeros" instead of "zeroes" the decision in the patch, but I have no problem changing it to what others like better. v2: - format detect_zeroes on the command line instead of detect_zeros v1: - https://www.redhat.com/archives/libvir-list/2015-December/msg00484.html Martin Kletzander (2): conf: Add support of zero-detection for disks qemu: Add support for zero-detection writes docs/formatdomain.html.in | 10 ++++++ docs/schemas/domaincommon.rng | 12 +++++++ src/conf/domain_conf.c | 23 +++++++++++++- src/conf/domain_conf.h | 11 +++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++ .../qemuxml2argv-disk-drive-detect-zeros.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 1 + 15 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml -- 2.6.4
This option allows or disallows detection of zero-writes if it is set to "on" or "off", respectively. It can be also set to "unmap" in which case it will try discarding that part of image based on the value of the "discard" option. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 10 ++++++ docs/schemas/domaincommon.rng | 12 +++++++ src/conf/domain_conf.c | 23 +++++++++++++- src/conf/domain_conf.h | 11 +++++++ src/libvirt_private.syms | 2 ++ .../qemuxml2argv-disk-drive-detect-zeros.xml | 37 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a8bd48e97129..bc925ea330c7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2498,6 +2498,16 @@ are numbered from 1 to the domain iothreads value. <span class='since'>Since 1.2.8 (QEMU only)</span> </li> + <li> + The optional <code>detect_zeros</code> attribute controls + whether to detect zero write requests. The value can be "off", + "on" or "unmap". First two values turn the detection off and + on, respectively, the third value turns the detection on and + tries to discard such areas from the image based on the value + of <code>discard</code> above (it will act as "on" if + <code>discard</code> is set to "ignore"). + <span class='since'>Since 1.3.1 (QEMU and KVM only)</span> + </li> </ul> </dd> <dt><code>backenddomain</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4804c692e210..de8d3614d24f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1576,6 +1576,9 @@ <optional> <ref name="driverIOThread"/> </optional> + <optional> + <ref name="detect_zeros"/> + </optional> <empty/> </element> </define> @@ -1659,6 +1662,15 @@ <ref name="unsignedInt"/> </attribute> </define> + <define name="detect_zeros"> + <attribute name='detect_zeros'> + <choice> + <value>off</value> + <value>on</value> + <value>unmap</value> + </choice> + </attribute> + </define> <define name="controller"> <element name="controller"> <attribute name="index"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5200c278914e..cb83266c92a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -792,6 +792,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore") +VIR_ENUM_IMPL(virDomainDiskDetectZeros, VIR_DOMAIN_DISK_DETECT_ZEROS_LAST, + "default", + "off", + "on", + "unmap") + VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "none", "yes", @@ -6532,6 +6538,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *mirrorFormat = NULL; char *mirrorType = NULL; char *domain_name = NULL; + char *detect_zeros = NULL; int expected_secret_usage = -1; int auth_secret_usage = -1; int ret = 0; @@ -6669,6 +6676,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, copy_on_read = virXMLPropString(cur, "copy_on_read"); discard = virXMLPropString(cur, "discard"); driverIOThread = virXMLPropString(cur, "iothread"); + detect_zeros = virXMLPropString(cur, "detect_zeros"); } else if (!def->mirror && xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { @@ -7271,6 +7279,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } + if (detect_zeros) { + if ((def->detect_zeros = + virDomainDiskDetectZerosTypeFromString(detect_zeros)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk detect_zeros mode '%s'"), + detect_zeros); + goto error; + } + } + if (driverIOThread) { if (virStrToLong_uip(driverIOThread, NULL, 10, &def->iothread) < 0 || def->iothread == 0) { @@ -18789,6 +18807,7 @@ virDomainDiskDefFormat(virBufferPtr buf, const char *copy_on_read = virTristateSwitchTypeToString(def->copy_on_read); const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio); const char *discard = virDomainDiskDiscardTypeToString(def->discard); + const char *detect_zeros = virDomainDiskDetectZerosTypeToString(def->detect_zeros); if (!type || !def->src->type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -18843,7 +18862,7 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src->driverName || def->src->format > 0 || def->cachemode || def->error_policy || def->rerror_policy || def->iomode || def->ioeventfd || def->event_idx || def->copy_on_read || - def->discard || def->iothread) { + def->discard || def->iothread || def->detect_zeros) { virBufferAddLit(buf, "<driver"); virBufferEscapeString(buf, " name='%s'", def->src->driverName); if (def->src->format > 0) @@ -18867,6 +18886,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " discard='%s'", discard); if (def->iothread) virBufferAsprintf(buf, " iothread='%u'", def->iothread); + if (def->detect_zeros) + virBufferAsprintf(buf, " detect_zeros='%s'", detect_zeros); virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cec681a788be..32b74ee60354 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -652,6 +652,15 @@ typedef enum { VIR_DOMAIN_DISK_DISCARD_LAST } virDomainDiskDiscard; +typedef enum { + VIR_DOMAIN_DISK_DETECT_ZEROS_DEFAULT = 0, + VIR_DOMAIN_DISK_DETECT_ZEROS_OFF, + VIR_DOMAIN_DISK_DETECT_ZEROS_ON, + VIR_DOMAIN_DISK_DETECT_ZEROS_UNMAP, + + VIR_DOMAIN_DISK_DETECT_ZEROS_LAST +} virDomainDiskDetectZeros; + typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo; struct _virDomainBlockIoTuneInfo { unsigned long long total_bytes_sec; @@ -730,6 +739,7 @@ struct _virDomainDiskDef { int sgio; /* enum virDomainDeviceSGIO */ int discard; /* enum virDomainDiskDiscard */ unsigned int iothread; /* unused = 0, > 0 specific thread # */ + int detect_zeros; /* enum virDomainDiskDetectZeros */ char *domain_name; /* backend domain name */ }; @@ -3000,6 +3010,7 @@ VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDeviceSGIO) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainDiskDiscard) +VIR_ENUM_DECL(virDomainDiskDetectZeros) VIR_ENUM_DECL(virDomainDiskMirrorState) VIR_ENUM_DECL(virDomainController) VIR_ENUM_DECL(virDomainControllerModelPCI) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55822aef9916..3b775cf10257 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -259,6 +259,8 @@ virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefNew; virDomainDiskDefSourceParse; +virDomainDiskDetectZerosTypeFromString; +virDomainDiskDetectZerosTypeToString; virDomainDiskDeviceTypeToString; virDomainDiskDiscardTypeToString; virDomainDiskErrorPolicyTypeFromString; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml new file mode 100644 index 000000000000..05494264d77a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml @@ -0,0 +1,37 @@ +<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-0.13'>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</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' discard='unmap' detect_zeros='unmap'/> + <source file='/var/lib/libvirt/images/f14.img'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver discard='ignore' detect_zeros='off'/> + <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> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index f967ceb94d0c..2b3db38b756f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -529,6 +529,7 @@ mymain(void) DO_TEST("disk-source-pool-mode"); DO_TEST_DIFFERENT("disk-drive-discard"); + DO_TEST("disk-drive-detect-zeros"); DO_TEST("virtio-rng-random"); DO_TEST("virtio-rng-egd"); -- 2.6.4
There is a slight problem here. The parameter in QEMU is called detect_zeroes, but we use "zeros" a tiny bit more in our code and documentation. I went with "zeros" to be consistent, but it might confuse some people. However, the other way around might be as confusing as this one, but we need to choose one. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 8 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6e5d203f0667..34a492df4538 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -308,6 +308,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet", /* 205 */ "virtio-input-host", + "drive-detect-zeros", ); @@ -2590,6 +2591,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, { "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT }, { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD }, + { "drive", "detect-zeroes", QEMU_CAPS_DRIVE_DETECT_ZEROS }, { "realtime", "mlock", QEMU_CAPS_MLOCK }, { "boot-opts", "strict", QEMU_CAPS_BOOT_STRICT }, { "boot-opts", "reboot-timeout", QEMU_CAPS_REBOOT_TIMEOUT }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 61d637997de1..992aea5c93f7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -335,6 +335,7 @@ typedef enum { /* 205 */ QEMU_CAPS_VIRTIO_TABLET, /* -device virtio-tablet-{device,pci} */ QEMU_CAPS_VIRTIO_INPUT_HOST, /* -device virtio-input-host-{device,pci} */ + QEMU_CAPS_DRIVE_DETECT_ZEROS, /* -drive detect_zeros= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d2f37e408bab..49b76adc416f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3969,6 +3969,17 @@ qemuBuildDriveStr(virConnectPtr conn, } } + if (disk->detect_zeros) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROS)) { + virBufferAsprintf(&opt, ",detect_zeros=%s", + virDomainDiskDetectZerosTypeToString(disk->detect_zeros)); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("detect_zeros is not supported by this QEMU binary")); + goto error; + } + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MONITOR_JSON)) { const char *wpolicy = NULL, *rpolicy = NULL; diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 1098dcf04044..15c6403a4a04 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -159,4 +159,5 @@ <flag name='rtl8139'/> <flag name='e1000'/> <flag name='virtio-net'/> + <flag name='drive-detect-zeros'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps index d67a48df1246..a3c4dc0d30b3 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps @@ -167,4 +167,5 @@ <flag name='virtio-mouse'/> <flag name='virtio-tablet'/> <flag name='virtio-input-host'/> + <flag name='drive-detect-zeros'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index f4f3673c50b9..1281d23a2536 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -168,4 +168,5 @@ <flag name='virtio-mouse'/> <flag name='virtio-tablet'/> <flag name='virtio-input-host'/> + <flag name='drive-detect-zeros'/> </qemuCaps> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args new file mode 100644 index 000000000000..5224a69798b2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name test \ +-S \ +-M pc-0.13 \ +-m 1024 \ +-smp 1 \ +-uuid 92d7a226-cfae-425b-a6d3-00bbf9ec5c9e \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot dc \ +-usb \ +-drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0,\ +format=qcow2,discard=unmap,detect_zeroes=unmap \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-drive file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,\ +id=drive-ide0-1-0,format=raw,discard=ignore,detect_zeroes=off \ +-device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 37f806edd3c0..e61553526ed2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -880,6 +880,10 @@ mymain(void) DO_TEST("disk-drive-discard", QEMU_CAPS_DRIVE_DISCARD, QEMU_CAPS_DEVICE); + DO_TEST("disk-drive-discard", + QEMU_CAPS_DRIVE_DISCARD, + QEMU_CAPS_DRIVE_DETECT_ZEROS, + QEMU_CAPS_DEVICE); DO_TEST("disk-snapshot", NONE); DO_TEST_FAILURE("disk-same-targets", QEMU_CAPS_DEVICE, QEMU_CAPS_SCSI_LSI, -- 2.6.4
On Mon, Dec 14, 2015 at 15:57:38 +0100, Martin Kletzander wrote:
There is a slight problem here. The parameter in QEMU is called detect_zeroes, but we use "zeros" a tiny bit more in our code and documentation. I went with "zeros" to be consistent, but it might confuse some people. However, the other way around might be as confusing as this one, but we need to choose one.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 8 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6e5d203f0667..34a492df4538 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -308,6 +308,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"virtio-tablet", /* 205 */ "virtio-input-host", + "drive-detect-zeros", );
@@ -2590,6 +2591,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, { "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT }, { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD }, + { "drive", "detect-zeroes", QEMU_CAPS_DRIVE_DETECT_ZEROS }, { "realtime", "mlock", QEMU_CAPS_MLOCK }, { "boot-opts", "strict", QEMU_CAPS_BOOT_STRICT }, { "boot-opts", "reboot-timeout", QEMU_CAPS_REBOOT_TIMEOUT },
A very noticable inconsistency in naming. Peter
On Mon, Dec 14, 2015 at 04:07:19PM +0100, Peter Krempa wrote:
On Mon, Dec 14, 2015 at 15:57:38 +0100, Martin Kletzander wrote:
There is a slight problem here. The parameter in QEMU is called detect_zeroes, but we use "zeros" a tiny bit more in our code and documentation. I went with "zeros" to be consistent, but it might confuse some people. However, the other way around might be as confusing as this one, but we need to choose one.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 8 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6e5d203f0667..34a492df4538 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -308,6 +308,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"virtio-tablet", /* 205 */ "virtio-input-host", + "drive-detect-zeros", );
@@ -2590,6 +2591,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, { "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT }, { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD }, + { "drive", "detect-zeroes", QEMU_CAPS_DRIVE_DETECT_ZEROS }, { "realtime", "mlock", QEMU_CAPS_MLOCK }, { "boot-opts", "strict", QEMU_CAPS_BOOT_STRICT }, { "boot-opts", "reboot-timeout", QEMU_CAPS_REBOOT_TIMEOUT },
A very noticable inconsistency in naming.
That is described in cover letter as well as in the commit message and should be discussed. As I said, I've found a reason for each spelling, so I'm OK with either of them.
Peter
On Tue, Dec 15, 2015 at 08:19:39 +0100, Martin Kletzander wrote:
On Mon, Dec 14, 2015 at 04:07:19PM +0100, Peter Krempa wrote:
On Mon, Dec 14, 2015 at 15:57:38 +0100, Martin Kletzander wrote:
There is a slight problem here. The parameter in QEMU is called detect_zeroes, but we use "zeros" a tiny bit more in our code and documentation. I went with "zeros" to be consistent, but it might confuse some people. However, the other way around might be as confusing as this one, but we need to choose one.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
...
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6e5d203f0667..34a492df4538 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -308,6 +308,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"virtio-tablet", /* 205 */ "virtio-input-host", + "drive-detect-zeros", );
@@ -2590,6 +2591,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, { "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT }, { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD }, + { "drive", "detect-zeroes", QEMU_CAPS_DRIVE_DETECT_ZEROS }, { "realtime", "mlock", QEMU_CAPS_MLOCK }, { "boot-opts", "strict", QEMU_CAPS_BOOT_STRICT }, { "boot-opts", "reboot-timeout", QEMU_CAPS_REBOOT_TIMEOUT },
A very noticable inconsistency in naming.
That is described in cover letter as well as in the commit message and should be discussed. As I said, I've found a reason for each spelling, so I'm OK with either of them.
At least the capability name should be consistent with the command name. Since the capabilities are qemu specific there is no reason to have the discrepancy here. I've googled some sources on the plural of 'zero' and looks like that indeed 'zeros' is more commonly used. At any rate. I don't really care which you actually choose for the XML name and variables, but the capability name should be spelled as qemu spells it. Peter
On 12/14/2015 09:57 AM, Martin Kletzander wrote:
QEMU supports detect-zeroes option since version 2.1, but we never added support for it in libvirt. If was requested by Vasiliy Tolstov in the list, so I just added it.
There are two discussions to be had, optionally. One is to decide whether we should disable detect_zeros='unmap' if discard is not set to 'unmap', but this is getting very hypervisor-specific, so I just documented the behaviour. The other one is the naming. I described why I made the decision for "zeros" instead of "zeroes" the decision in the patch, but I have no problem changing it to what others like better.
Unfortunate that qemu chose "zeroes" instead of "zeros" or instead of "detect_zero_writes" (or something even longer and more descriptive). Seems strange though to say "detect_zeros=unmap". Would perhaps something like : detect = zero_write [or just zero, zeros, or zeroes] detect = unmap ... or detect_zero = write detect_zero = unmap ... Be more generic? Also, I would think detect_zero[e]s = off is meaningless since it's an optional parameter. The way the code is written, passing detect_zeroes=off to me would be akin to not providing it at all. I think from my quick read - this would be similar to the "discard" option, w.r.t. values used/set. John
v2: - format detect_zeroes on the command line instead of detect_zeros
v1: - https://www.redhat.com/archives/libvir-list/2015-December/msg00484.html
Martin Kletzander (2): conf: Add support of zero-detection for disks qemu: Add support for zero-detection writes
docs/formatdomain.html.in | 10 ++++++ docs/schemas/domaincommon.rng | 12 +++++++ src/conf/domain_conf.c | 23 +++++++++++++- src/conf/domain_conf.h | 11 +++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++ .../qemuxml2argv-disk-drive-detect-zeros.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 1 + 15 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml
-- 2.6.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 15, 2015 at 07:27:46AM -0500, John Ferlan wrote:
On 12/14/2015 09:57 AM, Martin Kletzander wrote:
QEMU supports detect-zeroes option since version 2.1, but we never added support for it in libvirt. If was requested by Vasiliy Tolstov in the list, so I just added it.
There are two discussions to be had, optionally. One is to decide whether we should disable detect_zeros='unmap' if discard is not set to 'unmap', but this is getting very hypervisor-specific, so I just documented the behaviour. The other one is the naming. I described why I made the decision for "zeros" instead of "zeroes" the decision in the patch, but I have no problem changing it to what others like better.
Unfortunate that qemu chose "zeroes" instead of "zeros" or instead of "detect_zero_writes" (or something even longer and more descriptive).
Seems strange though to say "detect_zeros=unmap". Would perhaps something like :
detect = zero_write [or just zero, zeros, or zeroes] detect = unmap ...
or detect_zero = write detect_zero = unmap ...
Be more generic? Also, I would think detect_zero[e]s = off is meaningless since it's an optional parameter. The way the code is written, passing detect_zeroes=off to me would be akin to not providing it at all.
Yes, we could be more generic, I just couldn't think of any generalization of this feature. The "off" value is possible to use just in case the hypervisor default is something else ("on", for example), so that you can forcibly turn it off. Otherwise we would hae to guarantee that the default is off and I see that as a can of worms.
I think from my quick read - this would be similar to the "discard" option, w.r.t. values used/set.
John
v2: - format detect_zeroes on the command line instead of detect_zeros
v1: - https://www.redhat.com/archives/libvir-list/2015-December/msg00484.html
Martin Kletzander (2): conf: Add support of zero-detection for disks qemu: Add support for zero-detection writes
docs/formatdomain.html.in | 10 ++++++ docs/schemas/domaincommon.rng | 12 +++++++ src/conf/domain_conf.c | 23 +++++++++++++- src/conf/domain_conf.h | 11 +++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 11 +++++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + .../qemuxml2argv-disk-drive-detect-zeros.args | 27 ++++++++++++++++ .../qemuxml2argv-disk-drive-detect-zeros.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 1 + 15 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeros.xml
-- 2.6.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
John Ferlan -
Martin Kletzander -
Peter Krempa