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

[Eric Blake <eblake@redhat.com>]

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