On 06/04/2016 08:46 PM, Martin Kletzander wrote:
> 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@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 11 ++++++
> docs/schemas/domaincommon.rng | 12 ++++++
> src/conf/domain_conf.c | 19 ++++++++-
> src/conf/domain_conf.h | 11 ++++++
> src/libvirt_private.syms | 2 +
> .../qemuxml2argv-disk-drive-detect-zeroes.xml | 45 ++++++++++++++++++++++
> .../qemuxml2xmlout-disk-drive-detect-zeroes.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 8 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeroes.xml
> create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-detect-zeroes.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index e737e39d384f..199900963938 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2574,6 +2574,17 @@
> <code>bus</code> and "pci" or "ccw" <code>address</code> types.
> <span class='since'>Since 1.2.8 (QEMU 2.1)</span>
> </li>
> + <li>
> + The optional <code>detect_zeroes</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. Detecting such writes can take some time, however it
> + can save space for spare images, the third value ("unmap") turns the
> + detection on and additionally 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.6</span>
> + </li>
Do we really want to get in to the two space debate? ~/~
I would move the description right after discard since they're related.
Also I was wondering what a spare image was until I reread it as "sparse"...
So I know I'm "late" with this thought, but since it's optional (e.g.
off is not supplied), there are really only two methods that would need
to be described. If the attribute is not there, then it's off! So....
The optional detect_zeroes attribute controls how to detect the writing
of zero filled blocks for sparse images. If the attribute is not
supplied, it can be considered as "off" and there will be no detection.
If the attribute is set to "on", each block will will be scanned and ...
If the attribute is set to "unmap" and the discard attribute is set to
"unmap", then ...
NB, enabling the detection is a compute intensive operation, but can
save file space.
FWIW: Looking at the qemu code there seems to be a requirement that
discard is also set to unmap
One other thought would be to provide "suggestions" for usage... I'm
honestly not clear what the advantage is beyond perhaps saving file
space or perhaps coupled with Michal's sparse streams work be able to
move images much faster (perhaps offsetting the performance hit of zero
block detection!)
> </ul>
> </dd>
> <dt><code>backenddomain</code></dt>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index d14c1c5ecffd..da8de14a8afd 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1618,6 +1618,9 @@
> <optional>
> <ref name="driverIOThread"/>
> </optional>
> + <optional>
> + <ref name="detect_zeroes"/>
> + </optional>
> <empty/>
> </element>
> </define>
> @@ -1702,6 +1705,15 @@
> <ref name="unsignedInt"/>
> </attribute>
> </define>
> + <define name="detect_zeroes">
> + <attribute name='detect_zeroes'>
> + <choice>
> + <value>off</value>
> + <value>on</value>
> + <value>unmap</value>
> + </choice>
> + </attribute>
> + </define>
> <define name="controller">
> <element name="controller">
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9f9fdf24190e..dbff1cfa0399 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -806,6 +806,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST,
> "unmap",
> "ignore")
>
> +VIR_ENUM_IMPL(virDomainDiskDetectZeroes, VIR_DOMAIN_DISK_DETECT_ZEROES_LAST,
> + "default",
> + "off",
see note below
> + "on",
> + "unmap")
> +
> VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
> "none",
> "yes",
> @@ -7203,6 +7209,14 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
> VIR_FREE(tmp);
> }
>
> + if ((tmp = virXMLPropString(cur, "detect_zeroes")) &&
> + (def->detect_zeroes = virDomainDiskDetectZeroesTypeFromString(tmp)) <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown driver detect_zeroes value '%s'"), tmp);
> + goto cleanup;
> + }
> + VIR_FREE(tmp);
> +
> ret = 0;
>
> cleanup:
> @@ -19368,6 +19382,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_zeroes = virDomainDiskDetectZeroesTypeToString(def->detect_zeroes);
>
> if (!type || !def->src->type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -19422,7 +19437,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_zeroes) {
> virBufferAddLit(buf, "<driver");
> virBufferEscapeString(buf, " name='%s'", def->src->driverName);
> if (def->src->format > 0)
> @@ -19446,6 +19461,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " discard='%s'", discard);
> if (def->iothread)
> virBufferAsprintf(buf, " iothread='%u'", def->iothread);
> + if (def->detect_zeroes)
> + virBufferAsprintf(buf, " detect_zeroes='%s'", detect_zeroes);
> virBufferAddLit(buf, "/>\n");
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b1953b31431a..f44daa0b6b2e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -527,6 +527,15 @@ typedef enum {
> VIR_DOMAIN_DISK_DISCARD_LAST
> } virDomainDiskDiscard;
>
> +typedef enum {
> + VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT = 0,
> + VIR_DOMAIN_DISK_DETECT_ZEROES_OFF,
I know this follows discard essentially, buy why even have a default?
If OFF were zero then it wouldn't be written and things would be "as is"
today. The qemu impl doesn't have a default value and it seems as if
OFF is the default.
Hope it's worth than 1 korun's worth of advice ;-) (it's how wikipedia
spells it - what do I know... I'm more familiar with cents)
Code wise, I think things are fine - it's the description of the
functionality that still needs some tweaks.
John
> + VIR_DOMAIN_DISK_DETECT_ZEROES_ON,
> + VIR_DOMAIN_DISK_DETECT_ZEROES_UNMAP,
> +
> + VIR_DOMAIN_DISK_DETECT_ZEROES_LAST
> +} virDomainDiskDetectZeroes;
> +
> typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
> struct _virDomainBlockIoTuneInfo {
> unsigned long long total_bytes_sec;
> @@ -605,6 +614,7 @@ struct _virDomainDiskDef {
> int sgio; /* enum virDomainDeviceSGIO */
> int discard; /* enum virDomainDiskDiscard */
> unsigned int iothread; /* unused = 0, > 0 specific thread # */
> + int detect_zeroes; /* enum virDomainDiskDetectZeroes */
> char *domain_name; /* backend domain name */
> };
>
> @@ -2910,6 +2920,7 @@ VIR_ENUM_DECL(virDomainDiskIo)
> VIR_ENUM_DECL(virDomainDeviceSGIO)
> VIR_ENUM_DECL(virDomainDiskTray)
> VIR_ENUM_DECL(virDomainDiskDiscard)
> +VIR_ENUM_DECL(virDomainDiskDetectZeroes)
> 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 333bf7c8b2db..b07e90bf9fec 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -266,6 +266,8 @@ virDomainDiskDefForeachPath;
> virDomainDiskDefFree;
> virDomainDiskDefNew;
> virDomainDiskDefSourceParse;
> +virDomainDiskDetectZeroesTypeFromString;
> +virDomainDiskDetectZeroesTypeToString;
> virDomainDiskDeviceTypeToString;
> virDomainDiskDiscardTypeToString;
> virDomainDiskErrorPolicyTypeFromString;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeroes.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeroes.xml
> new file mode 100644
> index 000000000000..8953f50f3f92
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeroes.xml
> @@ -0,0 +1,45 @@
> +<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_zeroes='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_zeroes='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'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-detect-zeroes.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-detect-zeroes.xml
> new file mode 120000
> index 000000000000..afa3199d4a7e
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-detect-zeroes.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/qemuxml2argv-disk-drive-detect-zeroes.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index e9d1f938360d..d8810f791f27 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -558,6 +558,7 @@ mymain(void)
> DO_TEST("disk-source-pool-mode");
>
> DO_TEST("disk-drive-discard");
> + DO_TEST("disk-drive-detect-zeroes");
>
> DO_TEST("virtio-rng-random");
> DO_TEST("virtio-rng-egd");
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list