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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3844 bytes --]

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.
We also have the option of introducing @bottom-node, leaving @base’s
behavior as-is 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.

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]
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]