Re: [PATCH v2] qemu-img: Support bitmap --merge into backing image

[Eric Blake]

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 <mreitz@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