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

[Max Reitz]

On 15.09.20 15:31, Eric Blake wrote:
> 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.

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

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.

signature.asc
Description: OpenPGP digital signature