Re: [PATCH v7 14/47] stream: Deal with filters

[Max Reitz]

On 19.08.20 17:16, Kevin Wolf wrote:
> Am 19.08.2020 um 16:47 hat Max Reitz geschrieben:
>> On 18.08.20 16:28, Kevin Wolf wrote:
>>> Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
>>>> Because of the (not so recent anymore) 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       | 63 ++++++++++++++++++++++++++++++++------------
>>>>  blockdev.c           |  4 ++-
>>>>  3 files changed, 53 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index b20332e592..df87855429 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2486,6 +2486,10 @@
>>>>  # On successful completion the image file is updated to drop the backing 
>>>> file
>>>>  # and the BLOCK_JOB_COMPLETED event is emitted.
>>>>  #
>>>> +# In case @device is a filter node, block-stream modifies the first 
>>>> non-filter
>>>> +# overlay node below it to point to base's backing node (or NULL if @base 
>>>> was
>>>> +# not specified) instead of modifying @device itself.
>>>
>>> Not to @base's backing node, but to @base itself (or actually, to
>>> above_base's backing node, which is initially @base, but may have
>>> changed when the job is completed).
>>
>> Oh, yes.
>>
>> (I thought I had noticed that already at some point and fixed it
>> locally...  But apparently not.)
>>
>>> Should we also document what using a filter node for @base means?
>>
>> Hm.  What does it mean?  I think the more interesting case is what it
>> means if above_base is a filter, right?
>>
>> Maybe we can put in somewhere in the “If a base file is specified then
>> sectors are not copied from that base file and its backing chain.”  But
>> the more I think about it, the less I know what we could add to it.
>> What happens if there are filters above @base is that their data isn’t
>> copied, because that’s exactly the data in @base.
> 
> The interesting part is probably the graph reconfiguration at the end of
> the job. Which is actually already documented:
> 
> # When streaming completes the image file will have the base
> # file as its backing file.
> 
> Of course, this is not entirely correct any more (because the base may
> have changed).
> 
> If @base is a filter, what backing file path do we write into the top
> layer? A json: filename including the filter?

Yes.

Or, actually.  Now that I read the code...  It takes @base’s filename
before the stream job and then uses that.  So if @base has changed
during the job, then it still uses the old filename.

But that’s not really due to this series.

> Is this worth mentioning
> or do you consider it obvious?

Hm.  I consider it obvious, yes.  @base becomes @top’s backing file (at
least without any graph changes while the job is running), so naturally
what’s written into the image header is @base’s filename – which is a
json:{} filename.

On second thought, @backing-file’s description mysteriously states that
“QEMU will automatically determine the backing file string to use”.
Which makes sense because it would clearly not make sense to describe
what’s actually happening, which is to use @base’s filename at job start
regardless of whether it’s still there at the end of the job.

So I suppose I have the choice of either documenting exactly what’s
happening, even though it doesn’t make much sense, or just not, keeping
it mysterious.

So all in all, I believe the biggest surprise about what’s written into
the top layer isn’t that it may be a json:{} filename, but the filename
of a node that maybe doesn’t even exist anymore?  (Oh, no, please don’t
tell me you can delete it and get an invalid pointer read...)

The more I think about it, the more I think there are problems beyond
the scope of this series here. :/

Max

signature.asc
Description: OpenPGP digital signature