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

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 >> -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list