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

[Vladimir Sementsov-Ogievskiy]

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