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

[Eric Blake <eblake@redhat.com>]

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