On Wed 25 Jan 2017 03:03:36 PM CET, Max Reitz wrote: >>> (2) It is legal to have a greater refcount than the number of >>> internal snapshots plus one. qemu never produces such images, though >>> (or does it?). Could there be existing images where users will be >>> just annoyed by such warnings? Should we add a runtime option to >>> disable them? >> >> I don't think it's legal, or is there any reason why it would be? > > Yes, the spec doesn't disallow it, so it's legal. :-) > > I had thought about using it for the conversion of zero clusters to > data clusters in compat-level downgrading. You would only create a > couple of zero-filled data clusters and then just reference them very > often.
Ok, that's enough, then it's legal and it could be used at some point. > I think it could make sense to limit refcounts to snapshot count + 1 > in the spec -- the only issue I take with that is that I'm not sure > "better corruption detection" is a good reason to do so. I agree. > Also, while it makes sense to do the change to the spec *now*, it may > be difficult to revert in the future if needed. Let's say someone > comes up with a valid reason for wanting to use refcounts for > something else than internal snapshots. Then, we could simply allow > arbitrary refcounts again. But then-old qemus may then report such new > images as corrupted all the time. I also agree. > We could circumvent this by adding a compatible > has-arbitrary-refcounts flag at the same time. If that flag is set, > refcounts may be arbitrary; otherwise, they are limited by the > snapshot count. > > But again, that seems a bit much just so we can detect refcount > overwrites... Yes, it seems a bit too much :) >>> And of course another approach I already mentioned would be to scrap >>> the overlap checks altogether once we have image locking (and I >>> guess we can keep them around in their current form at least until >>> then). >> >> I think the overlap checks are fine, at least in my tests I only >> found problems with one of them, and only in some scenarios(*). So if >> we cannot optimize them easily I'd simply tell the user about the >> risks and suggest to disable them. Maybe the only thing that we need >> is simply good documentation. > > Well, overlap-check=none should be simple enough. I was thinking more of a short document detailing the overlap checks, what they exactly do and how to disable them. It could go in the wiki / blog post. To be honest I only learnt about them (and that they're enabled by default) when I was profiling the qcow2 driver. >> I think the most obvious candidate for optimization is >> refcount-block, and as I said it's the check what would create the >> bottleneck in most common scenarios. The optimization is simple: if >> the size of the qcow2 image is 7GB then you only need to check the >> first 4 entries in the refcount table. >> >> I can think of two problems with this, which is why I haven't sent a >> patch yet: >> >> (1) This needs the current size of the image every time we want to >> perform that check, and that means I/O. > > Could be cached (every time some refcount is updated, you update a > max_refcounted_cluster variable in the qcow2 state), but that means > code. That actually sounds simple enough. I can give it a try to see if it's worth doing, because I'd rather not do anything that needs too many changes. >> (*)I actually noticed (I'm talking about a qcow2 image stored in RAM >> now) that disabling the refcount-block check increases dramatically >> (+90%) the number of IOPS when using virtio-blk, but doesn't seem to >> have any effect (my tests even show a slightly negative effect!!) when >> using virtio-scsi. Does that make sense? Am I hitting a SCSI limit or >> what would be the reason for this? > > That's for someone else to answer. O:-) I'll see what I can find. Berto
