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

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

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