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

Thu Jun 9 14:40:49 UTC 2016

On Thu, Jun 09, 2016 at 07:33:30AM -0400, John Ferlan wrote:
>
>
>On 06/09/2016 04:00 AM, Martin Kletzander wrote:
>[...]
>>
>>> 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.
>>
>
>Reading the description of the parameters is what led me down the path
>more than anything else. Going back to the v1 discussion pointer just
>now proves your point - short term memory gets shorter every day.
>

Especially since it's six months since I sent the first series to the
list, right? =D

>>> 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 ;)
>>
>
>And looking at the qemu docs didn't help much either did it! It seems
>this is one of those attribute/features that would have a more specific
>use case. Although given the rest of your response - perhaps "assuming"
>what a hypervisor would do with such a detect_zeroes option could be
>dangerous. I left the ellipses because it wasn't clear to me.
>
>>> 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.
>>
>
>Suffice to say the qemu code wasn't clear enough for my quick read.
>There's some combination where if unmap is set, but the block flags
>doesn't have unmap, then qemu returns an error "setting detect-zeroes to
>unmap is not allowed without setting discard operation to unmap". (in
>blockdev.c)
>

And sure enough, you are right.  I updated my qemu and found out that
since the first posting some things have changed.  It looks like it's
hard keeping out-of-tree patches in sync across bunch of releases =)

>What would happen if "discard" is not set or is set to "ignore" and
>detect_zeroes to "unmap". I assume you've tried the various
>combinations. Does it make sense to "fail" XML config check if we
>determine that "detect_zeroes=unmap", but that "discard=none" or
>"discard=ignore"?  It seems perhaps as generic as we try to make these
>options, it's still qemu specific (and even more so if we document it as
>a qemu/kvm option was was suggested in some previous review). That is
>would some other hypervisor call it "unmap"?
>

I looked at the code, and at qemu and so on and I've found out that this
needs few more changes.  So I fixed that and also incorporated one small
piece of logic in there which does not assume any hypervisor defaults.

>
>>> 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).
>>
>
>Fair enough - it wasn't a requirement. As I'm reading things I'm
>wondering what value does this provide. The comment about "detecting
>such writes can take some time" may lead one to believe it isn't good;
>however, depending on your operation e.g. creation of a large, empty
>disk where one could drastically speed up creation as shown in the
>commit message:
>
>http://git.qemu.org/?p=qemu.git;a=commitdiff;h=465bee1da82e43f18d10c43cc7566d0284ad13a9
>
>
>>
>> [...]
>>
>>>> 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).
>>
>
>OK - fair enough... Like 2 cents, it wasn't worth much!
>
>Keep the default, fix up the docs, rebase, and hope that it stays fresh
>in my mind when it's reposted!
>

Have a look at the next version and see.  But don't worry, it's not like
something tragic will happen if it slips one more release.  Or five =D

I'll try to post is ASAP.

>John
>>> 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
>>>
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/6939ad27/attachment-0001.sig>