6:33 a.m.
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.
> 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)
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"?
> 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=465bee1da82e43f18d10c43cc7...
[...]
>> 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!
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
>