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

Martin Kletzander <mkletzan@xxxxxxxxxx> · Thu, 9 Jun 2016 16:40:49 +0200

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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc

Description: Digital signature
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



