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

[Max Reitz]

On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 17:59, Max Reitz wrote:
>> On 10.07.20 19:41, Andrey Shinkevich wrote:
>>> On 10.07.2020 18:24, Max Reitz wrote:
>>>> On 09.07.20 16:52, Andrey Shinkevich wrote:
>>>>> On 25.06.2020 18:21, Max Reitz wrote:
>>>>>> 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.
>>>>>> +#
>>>>>>     # @job-id: identifier for the newly-created block job. If
>>>>>>     #          omitted, the device name will be used. (Since 2.7)
>>>>>>     #
>>>>>> diff --git a/block/stream.c b/block/stream.c
>>>>>> index aa2e7af98e..b9c1141656 100644
>>>>>> --- a/block/stream.c
>>>>>> +++ b/block/stream.c
>>>>>> @@ -31,7 +31,8 @@ enum {
>>>>>>       typedef struct StreamBlockJob {
>>>>>>         BlockJob common;
>>>>>> -    BlockDriverState *bottom;
>>>>>> +    BlockDriverState *base_overlay; /* COW overlay (stream from
>>>>>> this) */
>>>>>> +    BlockDriverState *above_base;   /* Node directly above the
>>>>>> base */
>>>>> Keeping the base_overlay is enough to complete the stream job.
>>>> Depends on the definition.  If we decide it isn’t enough, then it isn’t
>>>> enough.
>>>>
>>>>> The above_base may disappear during the job and we can't rely on it.
>>>> In this version of this series, it may not, because the chain is
>>>> frozen.
>>>>    So the above_base cannot disappear.
>>>
>>> Once we insert a filter above the top bs of the stream job, the parallel
>>> jobs in
>>>
>>> the iotests #030 will fail with 'frozen link error'. It is because of
>>> the
>>>
>>> independent parallel stream or commit jobs that insert/remove their
>>> filters
>>>
>>> asynchroniously.
>>
>> I’m not sure whether that’s a problem with this series specifically.
>>
>>>> We can discuss whether we should allow it to disappear, but I think
>>>> not.
>>>>
>>>> The problem is, we need something to set as the backing file after
>>>> streaming.  How do we figure out what that should be?  My proposal
>>>> is we
>>>> keep above_base and use its immediate child.
>>>
>>> We can do the same with the base_overlay.
>>>
>>> If the backing node turns out to be a filter, the proper backing
>>> child will
>>>
>>> be set after the filter is removed. So, we shouldn't care.
>>
>> And what if the user manually added some filter above the base (i.e.
>> below base_overlay) that they want to keep after the job?
> 
> 
> It's automatically kept, if we use base_overlay->backing->bs as final
> backing node.
> 
> You mean, that they want it to be dropped?

Er, yes.  Point is, the graph structure below with @base at the root may
be different than the one right below @base_overlay.

> so, assuming the following:
> 
> top -(backing)-> manually-inserted-filter -(file)-> base
> 
> and user do stream with base=base, and expects filter to be removed by
> stream job?
> 
> Hmm, yes, such use-case is broken with our proposed way...
> 
> ====
> 
> Let me now clarify the problem we'll have with your way.
> 
> When stream don't have any filter, we can easily imagine two parallel
> stream jobs:
> 
> top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
> 
> stream1: top=top, base=mid2
> stream2: top=mid2, base=NULL
> 
> final picture is obvious:
> 
> top (merged with mid1) -(backing)-> mid2 (merged with base)

Yes, and I don’t think this currently working case is broken by this series.

> But we want stream job has own filter, like mirror.

Which it does not have yet, right?  Which is why I was saying that I
don’t think this is a problem with this series.  We could try to address
it later.

Or do you think we can’t address it later because right now all filter
cases are broken anyway so now would be the time to make a breaking
change (which the suggestion to not use @base as the final backing node is)?

> So the picture becomes more complex.
> 
> Assume stream2 starts first.
> 
> top -(backing)-> mid1 -(backing)-> stream2-filter -(backing)-> mid2
> -(backing)-> base

stream2-filter would be on top of mid2, right?

> Now, when we run stream1, with your solution, stream1 will freeze
> stream2-filter
> (wrong thing, stream2 will fail to remove it if it finished first), and
> stream1 will
> remove stream2-filter on finish (which is wrong as well, stream2 is not
> prepared to
> removing of its filter)..

Note that the user first needs to pass “mid2” as the base to the stream
job stream1.  Why don’t they just pass “stream2-filter”?  In my model,
the user should specify exactly which node they want not to be touched
by this stream job, and so that would be stream2-filter, not mid1.

I feel like the answer to this question has to do with implicit nodes.
AFAIU you wanted to remove them, so I don’t think we’d want to
special-case them here.

If you think that we can’t expect users to pass “stream2-filter” because
currently it should work with “mid2”, then that’s a case of implicit
nodes and it means we should ascend from @base up to the first
non-implicit node to get the @above_base we want.

> But, with our proposed way (freeze only chain up to base_overlay
> inclusively, and use backing(base_overlay) as final backing), all will
> work as expected, and two parallel jobs will work..

I don’t think it will work as expected because users can no longer
specify which node should be the base node after streaming.  And the
QAPI schema says that base-node is to become the backing file of the top
node after streaming.

I suppose you’re arguing that streaming through filters basically just
doesn’t work at all right now, so we’re free to do whatever?

Well, that still leaves the problem that users should be able to specify
which node is to become the base after streaming, and that that node
maybe shouldn’t be restricted to immediate children of COW images.

> ====
> 
> So, these are two mutually exclusive cases.. I vote for freezing up to
> base_overlay, and use backing(base_overlay) as final backing, because:
> 
> 1. I can't imaging other way to fix the case with parallel streams with
> filters (it's not a problem of current master, but we have pending
> series which will introduce stream job filter, and the problem will
> appear and even break iotest 30)

Besides the question of whether the top job could just use the bottom
job’s filter node as the base, there’s also the alternative of admitting
defeat and declaring that you just cannot use a single node in two
streams, because we didn’t find a way to make it work after all.

You could still create a temporary overlay in between that’s never used
and then drop it with a trivial stream afterwards.

(But that just in case specifying the bottom job’s stream node somehow
wouldn’t work.)

> 2. I don't think that removing filters above base node by stream job is
> so important case to break parallel stream jobs in future:
> 
>  - Stream job is not intended to remove filters, but to stream data.
> Filters between base_overlay and base don't contain any data and
> unrelated to stream process

Well, it is intended to remove nodes.  You can only remove data-bearing
nodes by moving data around.  I suspect if there was a way to get the
to-be-removed nodes removed without having to move their data around,
that would be popular.

>  - I think, that filters are "more related" to their children than to
> their parents. So, removing filters related to base node, when we just
> remove all data-containing nodes between top and base (and are not going
> to remove base node) is at least questionable.

Yes.

Although it could be argued that it is a handy way to remove filters, in
a backing chain at least.  (Thanks to bdrv_find_overlay(), @base and
@top still need to refer to different levels of the backing chain, but
if we lifted that restriction, I suppose it could work for any filter
chain.)

*shrug*

> On the contrary, removing
> all intermediate data containing nodes _together_ with their filters is
> absolutely correct thing to do.

I don’t think so, actually.  Like, you have a throttle node somewhere in
the chain, shouldn’t you maybe want to move it down below the chain?  Or
a COR node, shouldn’t that go above the chain after streaming?

I’m not making an argument here, I just don’t quite understand why you’d
bring up what happens with intermediate filters here.  The only reason
to drop them is because that’s what I expect users to expect of the
stream job.

> Next, with your way, what about filters, inserted above base during
> stream job? They will be between above_base and base, and will not be
> removed. So with your way, filters above base, existing before job start
> will be frozen during the job and removed after it, but filters appended
> above base during the job will be untouched. With our way, just all base
> node related filters are untouched by the job. It seems simpler
> definition for me and simpler to document.

Hm.  The documentation seems the same to me.  Either it’s “The backing
node (at the end of the job) of @base’s parent node (when starting the
job)” or “The backing node (at the end of the job) of the next
non-filter node above @base (when starting the job)”.

The problem you describe (that @above_base at the end of the job isn’t
necessarily above @base anymore) also exists with your suggestion,
namely that you can add overlays above @base after the job has started,
so @base_overlay at the end of the job isn’t necessarily the first
non-filter node above @base anymore.


OK, so after all this text, maybe some more original problem searching.
 I think it the root of the problem is that the stream job takes a @base
parameter, but as of c624b015bf14fe01, it doesn’t really matter anymore.
 Maybe c624b015bf14fe01 should have introduced a new parameter for users
to specify the bottom node instead of @base.

Well, that would have made everything a parameter mess, but it would
have saved us the trouble now.

In any case, the problem we have now is that we want a way to
automagically find out which node the bottom node should be, because the
user can’t specify it.  So the documentation is always going to be
written as “The backing node (at the end of the job) of $bottom”, where
“$bottom” is what we’re interested in figuring out.

I thought it would be best if we stick as close as possible to the
spirit of the current documentation, which basically requires @base-node
to be the backing node of the top after streaming.  (If you do graph
modifications during the job, that’s on you, because since
c624b015bf14fe01 we can’t keep the base frozen.)

Your suggestion to do basically what you consider to be right, which
comes at the caveat of being untruthful to the current documentation
even if there are no graph modifications during the job.  Luckily, the
stream job right now doesn’t work in the cases we’re looking at, so it
wouldn’t be a breaking change.  The problem I have with it is that
you’re assuming what is right and what isn’t (i.e. “Who would want to
remove filter nodes directly above @base”) without giving the user a
chance to specify.


I think it would be nice if we could have something that remains
truthful to the current documentation.  If just ignoring implicit
filters above @base would work, then I’d find that nice.  If it doesn’t,
I suppose there’s indeed little we can do but to indeed forego
@above_base and just use @base_overlay for all cases.

Max

signature.asc
Description: OpenPGP digital signature