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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

19.08.2020 15:39, Max Reitz wrote:
> On 10.08.20 13:04, Vladimir Sementsov-Ogievskiy wrote:
>> 10.08.2020 11:12, Max Reitz wrote:
>>> On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
> 
> [...]
> 
>>>> 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.
> 
> Yeah, but after c624b015bf, that’s just how it is.  I thought the best
> would be an approach where if there are no graph changes during the job,
> you would indeed end up with @base as the backing file afterwards.
> 
> [...]
> 
>>> 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.
> 
> Sounds very nice to me, but...
> 
>> 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..
> 
> ...yeah.  Hm.  What a pain, but right, we can just deprecate it.
> 
> Unfortunately, I don’t think there’s any way we could issue a warning
> (we’d want to deprecate using the @base node in something outside of the
> stream job, and we can’t really detect this case to issue a warning).
> So it would be a deprecation found only in the deprecation notes and the
> QAPI spec...

Hmm.. add "bool frozen_only_warn" field to BdrvChild and print warning
instead of fail where c->frozen cause failure?

I can make a patch, if you don't think that its too much.

> 
>> 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?
> 
> I don’t know, but I’m afraid I don’t think we can.

Actually, we already done so once:

Freezing of base was introduced in 4.0, when c624b015bf comes in 4.1
(there were no real user bugs or feature requests, it was just my [bad]
idea, related to the problem around our stream-filter and parallel
jobs in iotest 30).
So, at least for 4.0 we had frozen base.. Were there any bugs?
But now we live with not-frozen base for several releases..

> 
>> 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.
> 
> If that works...
> 
> 
> OK.  So what I think we can do is first just take this patch as part of
> this series.

Yes, let's start with it.

>  Then, we add @bottom-node separately and deprecate @base
> not being frozen.

I think we can just deprecate, and not add the bottom-node. If someone
will come during deprecation period and say that he have such use-case,
we'll add @bottom-node. Otherwise we'll just start to freeze base node
again.

> 
> If it’s feasible to not add a stream filter node until the deprecation
> period is over, then that should work.
> 
> Max
> 


-- 
Best regards,
Vladimir