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

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>


On 10.08.2020 14:04, Vladimir Sementsov-Ogievskiy wrote:
> 10.08.2020 11:12, Max Reitz wrote:
>> 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)?
>
> I think, we can address it later, but it would be good to fit into one 
> release cycle with these series, to not make incompatible behavior 
> changes later.
>
>>
>>> 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?
>
> Right. In my picture, "-(backing)->" means backing link. Hmm, most 
> probably stream-filter is COR, which actually have file child. It 
> doesn't matter here.
>
>>
>>> 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.
>
> Hmm. I'm sure we already tried/discussed this.. But I don't remember 
> the results. Seems, such logic should work as well. Andrey don't you 
> remember? I think, the only difficulty is to update iotests 30 (in our 
> stream-filter series) to use node-names, not filename.. Can we go this 
> way?
>
>>
>> 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.
>
> Agree. I think stream-filter should be explicit, like recently 
> introduced backup-top.
>
>>
>> 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.
>
> But this will never work with either way: base node may disappear 
> during stream. Even with you way, they only stable thing is 
> "above-base", which backing child may be completely another node at 
> stream finish.
>
>>
>> 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?
>
> I don't, but I like the idea :)
>
>>
>> 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.
>
> And again, this is impossible even with your way. I have an idea:
>
> What about making the whole thing explicit?
>
> We add an optional parameter to stream-job: bottom-node, which is 
> mutally exclusive with specifying base.
>
> Then, if user specified base node, we freeze base as well, so it can't 
> disappear. User will not be able to start parallel stream with this 
> base node as top (because new stream can not insert a filter into 
> frozen chain), but for sure it's rare case, used only in iotest 30 
> :)). Benefit: user have guarantee of what would be final backing node.
>
> Otherwise, if user specified bottom-node, we use the way of this 
> patch. So user can run parallel streams (iotest 30 will have to use 
> bottom-node argument). No guarantee of final base-node, it would be 
> backing of bottom-node at job finish.
>
> But, this is incompatible change, and we probably should wait for 3 
> releases for deprecation of old behavior..
>
> Anyway, I feel now, that you convinced me. I'm not sure that we will 
> not have to change it make filter work, but not reason to change 
> something now. Andrey, could you try to rebase your series on top of 
> this and fix iotest 30 by just specifying  exact node-names in it?..
>
>
> Hmmm. My thought goes further. Seems, that in this way, introducing 
> explicit filter would be incompatible change anyway: it will break 
> scenario with parallel stream jobs, when user specifies filenames, not 
> node names (user will have to specify filter-node name as base for 
> another stream job, as you said). So, it's incompatible anyway.
>
> What do you think of it? Could we break this scenario in one release 
> without deprecation and don't care? Than I think my idea about base vs 
> bottom-node arguments for stream job may be applied. Or what to do?
>
> If we can't break this scenario without a deprecation, we'll have to 
> implement "implicit" filter, like for mirror, when filter-node-name is 
> not specified. And for this implicit filter we'll need additional 
> logic (closer to what I've proposed in a previous mail). Or, try to 
> keep stream without a filter (not insert it at all and behave the old 
> way), when filter-node-name is not specified. Than new features based 
> on filter will be available only when filter-node-name is specified, 
> but this is OK. The latter seems better for me.
>
>>
>>> ====
>>>
>>> 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.)
>
> We can break existing user scenarios.. Seems unlikely. I don't know.
>
>>
>>> 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.
>
> Yes, absolutely agree. If we do it now, would it be incompatible 
> change or not?
>
>>
>> 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.
>
> I also try to think about future introduction of stream filter. It 
> shouldn't be a breaking change as well. But now I think it can be done 
> the hard-way if needed: just work without a filter if filter-node-name 
> is not given. but probably I care to much. Finally we can just drop 
> the test-case from 030 or insert additional intermediate node into it..
>
>>  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.
>
> Then, we should freeze base node again, so just revert c624b015bf14fe01
>
> I go and see it cover-letter of the series introduced c624b015bf14fe01:
>
>   This series introduces a bottom intermediate node that eliminates the
>   dependency on the base that may change while stream job is running.
>   It happens when stream/commit parallel jobs are running on the same
>   backing chain. The base node of the stream job may be a top node of
>   the parallel commit job and can change before the stream job is
>   completed. We avoid that dependency by introducing the bottom node.
>
> Hmm. Bad that we didn't add an iotest, but the series solved existing 
> problem:
> parallel stream and commit, as commit already has filter. Commit has 
> filter
> with "backing" child, so, I assume that the case worked prior to 
> introducing
> frozen chains, broken then by frozen chains and then fixed by 
> c624b015bf14fe01.
> Still I don't know are there some real users of such parallel jobs..
>
>>  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.
>>
>
> OK, at this point, I think, I'm OK with your patch in context of these 
> series.
>
> But I fill that something more should be done. Could we just revert 
> c624b015bf14fe01 as "non corresponding to specification"?
>
>