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

Thu Jun 9 08:00:09 UTC 2016

On Wed, Jun 08, 2016 at 07:27:47PM -0400, John Ferlan wrote:
>
>
>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 at redhat.com>
>> ---
>>  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? ~/~
>

Never again =)  And since this is HTML, it will be rendered as a one
space, no matter how many whitespaces there are ;)

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

Funny typo, thanks for catching that.

>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....
>

I believe we discussed that already.  I don't know with whom that was or
in what version; maybe it was on another subject altogether.
Nevertheless we can't say that not supplying the attribute means "off"
and that's for one simple reason.  For QEMU that would work, but if
there is some other hypervisor (or maybe a new one in the future) that
does zero detection when we don't specify anything, then we're screwed.
Or when QEMU changes it's default behaviour (very unlikely, I know, but
better safe than sorry).

That's why not specifying anything means "let the hypervisor decide" and
we can support that until eternity and be sure we won't break anything
like that.  If we decide that not specifying anything means "off", then
we can always add that default value in the XML automatically.

>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.
>

I added this in the docs because I was unable to come up with such
nicely worded explanation ;)

>FWIW: Looking at the qemu code there seems to be a requirement that
>discard is also set to unmap
>

Differently than what I described in the documentation?  I mentioned it
there and we discussed whether we should follow that behaviour or not in
previous round of patches I believe.

>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!)
>

I'm thinking about doing some write-ups about suggestions and
explanation of scenarios that people ask about often.  But that's a
story for another day and it doesn't fit into documentation nor manuals
(maybe some example section in the manual but I don't know who'd be
looking for that there).

[...]

>> 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
>

[...]

>> 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.
>

Bear in mind that there are freaking weird hypervisors out there and the
fact that QEMU behaves "sanely" (in this particular scenario) doesn't
mean others will follow.  And libvirt being all about the abstraction
should make sure we won't have bunch of new XML elements just to say the
same thing (which happened way too many times already).

>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)
>

Wow, 1 crown is double the amount of 2 cents ;)

>Code wise, I think things are fine - it's the description of the
>functionality that still needs some tweaks.
>
>John
>
-------------- 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/20160609/ee8c4832/attachment-0001.sig>