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