On 9/17/20 5:19 AM, Max Reitz wrote:
temporary over NBD, referring to a bitmap that lives only in Active is
less effort than having to copy a bitmap into temporary [1]. So the
testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and, when NOT using 'qemu-img
bitmap', that bitmaps are indeed visible through a backing chain.
Well. It is useful over NBD but I would doubt that it isn’t useful in
general. For example, the QMP commands that refer to bitmaps always do
so through a node-name + bitmap-name combination, and they require that
the given bitmap is exactly on the given node.
So I think this is a very much a case-by-case question. (And in
practice, NBD seems to be the outlier, not qemu-img bitmap.)
I'm happy to reword slightly to give that caveat.
The code looks good to me, but I wonder whether in the commit message it
should be noted that we don’t want to let bitmaps from deeper nodes
shine through by default everywhere, but just in specific cases where
that’s useful (i.e. only NBD so far AFAIA).
So is this a Reviewed-by? I'm happy to queue it through my bitmaps
tree, if so.
It wasn’t meant as an R-b, because my R-b depends on how the commit
message addresses the question of when exactly bitmaps from the backing
chain should be visible on the top image. Whether qemu-img bitmap is an
exception, or whether this is really a case-by-case question.
(I wanted to know whether you agree on including it. Normally, I’m
happy to give an R-b on the basis of “with that done”, but in this case
I wasn’t entirely sure whether my request was reasonable, but I also
felt that in case it was, it wasn’t optional, given that you do have an
entire paragraph in the commit message dedicated to why the backing
image’s bitmap is visible on an image exported over NBD.)
Here's my rewording:
However, note that on a case-by-case analysis, there _are_ times where
we treat it as a feature that we can access a bitmap from a backing
layer in association with an overlay BDS. A demonstration of this is
using NBD to expose both an overlay BDS (for constant contents) and a
bitmap (for learning which blocks are interesting) during an
incremental backup:
Base <- Active <- Temporary
\--block job ->/
where Temporary is being fed by a backup 'sync=none' job. When
exposing Temporary over NBD, referring to a bitmap that lives only in
Active is less effort than having to copy a bitmap into Temporary [1].
So the testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and that qemu-nbd is indeed able
to access a bitmap inherited from the backing chain since it is a
different use case than 'qemu-img bitmap'.
I have to say I would like to see how you do phrase it in the end, but
given that you do agree on including it, I can give a
Reviewed-by: Max Reitz <mre...@redhat.com>
Now.
Okay, I think I've met your request, so I'll go ahead and send the pull
request today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
