Re: [PATCH v6 28/42] stream: Deal with filters

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3390 bytes --]

On 16.09.19 11:52, Max Reitz wrote:
> On 13.09.19 16:16, Kevin Wolf wrote:
>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>>> Because of the recent changes that make the stream job independent of
>>> the base node and instead track the node above it, we have to split that
>>> "bottom" node into two cases: The bottom COW node, and the node directly
>>> above the base node (which may be an R/W filter or the bottom COW node).
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  qapi/block-core.json |  4 ++++
>>>  block/stream.c       | 52 ++++++++++++++++++++++++++++----------------
>>>  blockdev.c           |  2 +-
>>>  3 files changed, 38 insertions(+), 20 deletions(-)


[...]

>>> +    if (bdrv_freeze_chain(bs, above_base, errp) < 0) {
>>>          return;
>>>      }
>>
>> Hm... This feels odd. There are two places where stopping to freeze the
>> chain would make obvious sense: At base, like we originally did; or at
>> base_overlay, like we (intend to) do since commit c624b015, because we
>> say that we don't actually mind if the user replaces the base image. I
>> don't see how stopping at the first filter above base makes sense.
>>
>> So should this use bottom_cow_node/base_overlay instead of above_base?
> 
> I suppose I thought “Better be safe than sorry”.
> 
>> You couldn't use StreamBlockJob.above_base any more then because it
>> could change, but you also don't really need it anywhere. It's only used
>> for unfreezing (which would change) and for finding the base (you can
>> still find bdrv_backing_chain_next(s->base_overlay)). I guess this would
>> even be a code simplification.
> 
> Great, I’ll see to it.

On second thought (yes, I know, it’s been a couple of months...) I’m not
so sure.

If @base is a filter, then bdrv_backing_chain_next(s->base_overlay) will
not return it.  So then the filter will be dropped, but that probably
isn’t what the user intended.

(In fact, the block-stream doc says “When streaming completes the image
file will have the base file as its backing file.”)

So now this gets hairy.  We do want exactly @base as the backing file
unless the user changed the graph.  But how can we detect that and if it
hasn’t changed find @base again?

What this patch did in this version worked because the graph was frozen
until @above_base.

Alternatively, we could store a pointer to @base directly (or its node
nmae) and then see whether there is some node between s->base_overlay
and bdrv_backing_chain_next(s->base_overlay) that matches that at the
end of streaming.

Well, actually, a pointer won’t work because of course maybe that node
was deleted and the area is now used for an unrelated node that the user
doesn’t want as the new backing file.

The node name could actually work, though.  I mean, if there is some
node in the immediate backing filter chain of base_overlay with that
node name after streaming, it does make sense to assume that the user
wants this to be the backing file; regardless of whether that’s exactly
the same node as it was at the start of streaming.

But we now also have to think about what to do when there is no node
with such a node name.  Should we keep all filters below base_overlay?
Or should we drop all of them?  I actually think keeping them is the
safer choice...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]