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

[Eric Blake]

On 9/15/20 3:57 AM, Max Reitz wrote:
> On 14.09.20 21:10, Eric Blake wrote:
> > If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
> > bitmap from top into base, qemu-img was failing with:
> >
> > qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared 
> > "write" lock
> > Is another process using the image [base.qcow2]?
> >
> > The easiest fix is to not open the entire backing chain of either
> > image (source or destination); after all, the point of 'qemu-img
> > bitmap' is solely to manipulate bitmaps directly within a single qcow2
> > image, and this is made more precise if we don't pay attention to
> > other images in the chain that may happen to have a bitmap by the same
> > name.
> >
> > However, note that during normal usage, it is a feature that qemu will
> > allow a bitmap from a backing image to be exposed by an overlay BDS;
> > doing so makes it easier to perform incremental backup, where we have:
> >
> > Base <- Active <- temporrary
>
> *temporary
>
> (Also it’s a bit strange that “Base” and “Active” are capitalized, but
> “temporary” isn’t)
>
> >            \--block job ->/
> >
> > with temporary being fed by a block-copy 'sync' job; when exposing
>
> s/block-copy 'sync'/backup 'sync=none'/?

Will fix both.

> > 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.

> > [1] Full disclosure: prior to the recent commit 374eedd1c4 and
> > friends, we were NOT able to see bitmaps through filters, which meant
> > that we actually did not have nice clean semantics for uniformly being
> > able to pick up bitmaps from anywhere in the backing chain (seen as a
> > change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when
> > block-copy swapped from a one-off to a filter).  Which means libvirt
> > was already coded to copy bitmaps around for the sake of older qemu,
> > even though modern qemu no longer needs it.  Oh well.
> >
> > Fixes: http://bugzilla.redhat.com/1877209
> > Reported-by: Eyal Shenitzky <eshenitz@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >
> > In v2:
> > - also use BDRV_O_NO_BACKING on source [Max]
> > - improved commit message [Max]
> >
> >   qemu-img.c                 | 11 ++++++--
> >   tests/qemu-iotests/291     | 12 ++++++++
> >   tests/qemu-iotests/291.out | 56 ++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 76 insertions(+), 3 deletions(-)
>
> 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.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org