[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/2] conf: Add support of zero-detection for disks

On Fri, Apr 29, 2016 at 08:52:50AM +0200, Peter Krempa wrote:
On Thu, Apr 28, 2016 at 16:56:55 +0200, 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.

Neither this ...

Signed-off-by: Martin Kletzander <mkletzan redhat com>
 docs/formatdomain.html.in                          | 10 +++++
 docs/schemas/domaincommon.rng                      | 12 ++++++
 src/conf/domain_conf.c                             | 21 +++++++++-
 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(+), 2 deletions(-)
 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 d2db53b41823..3d09f511a8c4 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2564,6 +2564,16 @@
             are numbered from 1 to the domain iothreads value.
             <span class='since'>Since 1.2.8 (QEMU only)</span>
+          <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, the third value turns the detection on and

... nor this does document the "on" option in a way that would describe
what it actually does.

Without the documentation it's not really clear what to expect in qemu
or what to map this to in other hypervisors.

Makes sense, I fixed that for next version.

+            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").

This is behavior of qemu so it should be marked as so.

I felt like it made sense to mimic that behaviour as it is also pretty
abstract and other drivers could make sense of all the values.  If we
say that this is specific to QEMU, then we are not creating any
abstraction above that.  If you like, though, I can add that information

+            <span class='since'>Since 1.3.1 (QEMU and KVM only)</span>

Sorry for delaying the review. The version is unfortunately incorrect.

Also why "KVM only"? Does qemu reject it in TCG mode? Shouldn't we then
forbid trying to use it in such case (comment for next patch).

It doesn't say "KVM only", but "QEMU and KVM only" because it is
supported by qemu driver with domain type='qemu' as well as type='kvm'.
I thought that's what all the other occurrences of "QEMU and KVM" in
this file mean.  Anyway, I edited that part out.

Additionally it's not clear what happens if the storage technology does
not support unmaping of the sectors.

With the documentation fixed, I believe it already makes sense, let's
see what you'll say for v2 (I'll send that after the release).

+          </li>


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8bed670f243..1a336d08cda5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c


@@ -7062,7 +7068,7 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
         } else {
             if ((def->src->format = virStorageFileFormatTypeFromString(tmp)) <= 0) {
-                               _("unknown driver format value '%s'"), tmp);
+                               _("unknown driver type value '%s'"), tmp);

Unrelated change.

thanks, edited out.

                 goto cleanup;



Attachment: signature.asc
Description: Digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]