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

[Martin Kletzander]

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 at 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>
>> +          <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
there.

>> +            <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>
>>          </ul>
>>        </dd>
>>        <dt><code>backenddomain</code></dt>
>
>[...]
>
>> 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) {
>>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> -                               _("unknown driver format value '%s'"), tmp);
>> +                               _("unknown driver type value '%s'"), tmp);
>
>Unrelated change.
>

thanks, edited out.

>>                  goto cleanup;
>>              }
>>          }
>
>[...]
>
>Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160531/f66927f0/attachment-0001.sig>