Re: [PATCH v8 6/7] block-stream: freeze link to base node during stream job

[Vladimir Sementsov-Ogievskiy]

07.09.2020 14:44, Max Reitz wrote:
> On 04.09.20 15:48, Vladimir Sementsov-Ogievskiy wrote:
> > 04.09.2020 16:21, Max Reitz wrote:
> > > On 28.08.20 18:52, Andrey Shinkevich wrote:
> > > > To keep the base node unchanged during the block-stream operation,
> > > > freeze it as the other part of the backing chain with the intermediate
> > > > nodes related to the job.
> > > > This patch revers the change made with the commit c624b015bf as the
> > > > correct base file name and its format have to be written down to the
> > > > QCOW2 header on the disk when the backing file is being changed after
> > > > the stream job completes.
> > > > This reversion incurs changes in the tests 030, 245 and discards the
> > > > test 258 where concurrent stream/commit jobs are tested. When the link
> > > > to a base node is frozen, the concurrent job cannot change the common
> > > > backing chain.
> > > >
> > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > > ---
> > > >    block/stream.c             |  29 ++------
> > > >    tests/qemu-iotests/030     |  10 +--
> > > >    tests/qemu-iotests/245     |   2 +-
> > > >    tests/qemu-iotests/258     | 161
> > > > ---------------------------------------------
> > > >    tests/qemu-iotests/258.out |  33 ----------
> > > >    5 files changed, 14 insertions(+), 221 deletions(-)
> > > >    delete mode 100755 tests/qemu-iotests/258
> > > >    delete mode 100644 tests/qemu-iotests/258.out
> > >
> > > Does this need to be in this series?  (I’m not entirely sure, based on
> > > what I can see in patch 7.)
> > >
> > > When doing this, should we introduce a @bottom-node option alongside, so
> > > that we can make all the tests that are deleted here pass still, just
> > > with changes?
> >
> > That's a question to discuss, and I asked Andrey to make this patch in this
> > simple way to see, how much damage we have with this change.
> >
> > Honestly, I doubt that we need the new option. Previously, we decided that
> > we can make this change without a deprecation. If we still going to do it,
> > we shouldn't care about these use cases. So, if we freeze base again
> > wituhout
> > a depreaction and:
> >
> > 1. add bottom-node
> >
> >   - we keep the iotests
> >   - If (unlikely) someone will came and say: hey, you've broken my
> > working scenario, we will say "use bottom-node option, sorry"
> >   - Most likely we'll have unused option and corresponding unused logic,
> > making code more complex for nothing (and we'll have to say "sorry" anyway)
> >
> > 2. without option
> >
> >   - we loose the iotests. this looks scary, but it is honest: we drop
> > use-cases and corresponding iotests
> >   - If (unlikely) someone will came and say: hey, you've broken my
> > working scenario, he will have to wait for next release / package
> > version / or update the downstream himself
> >   - Most likely all will be OK.
>
> Well, yes, we’ll disrupt either way, but it is a difference whether we
> can tell people immediately that there’s an alternative now, or whether
> we’ll have to make them wait for the next release.
>
> Basically, the whole argument hinges on the question of whether anyone
> uses this right now or not, and we just don’t know.
>
> The question remains whether this patch is necessary for this series.

Otherwise iotests fail :)

> We also have the option of introducing @bottom-node, leaving @base’s
> behavior as-is

You mean not make it freeze base again, but just don't care?

> and explaining it as a legacy option from which
> @bottom-node is inferred.  Then specifying @base just becomes weird and
> problem-prone when the graph is reconfigured while the job is active,
> but you can get around that by simply using the non-legacy option.

Hmm. Last time, I thought that bottom-node was a bad idea, as we have a lot of 
problems with it, but you think it should be kept as preferred behavior? But 
this sounds as working idea.

Then, we'll probably want to set skip_filters(bottom->backing) as backing of top 
in qcow2 metadata, and direct bottom->backing as new backing of top in block node 
graph.

Anyway, I like the idea to deprecate filename-based interfaces wherever we can.

PS: Sorry for my decreased attention to the list for last weeks, I have to 
finish necessary work for Virtuozzo release.

> Max
>
> > Hmm. OK, and the hard-way:
> >
> > 3. Enable all the new logic (filter insertion, freezing base, etc.) only
> > when filter-node-name option specified. And immediately deprecate
> > not-specifying the option.
> >   [Note, that in way [3] we don't need bottom-node option]


--
Best regards,
Vladimir