8:57 a.m.
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
8:57 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: Add support of zero-detection for disks
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(a)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
8:57 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Add support for zero-detection writes
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(a)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
9:07 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Add support for zero-detection writes
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(a)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
1:19 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Add support for zero-detection writes
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(a)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
1:31 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Add support for zero-detection writes
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(a)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
6:27 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:22 a.m.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
3265
days inactive
3266
days old
7 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Peter Krempa