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: 5347 bytes --]

On 07.09.20 14:17, Vladimir Sementsov-Ogievskiy wrote:
> 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?

Yes.  I think the only problem with that would be that it’s unintuitive
in case the graph is modified while the job is running, but I can’t find
that worse than forbidding that case completely.

(And I think it would be easier to explain if we introduced @bottom-node.)

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

Hm, did we?  Off the top of my head, I can’t remember any.  Besides the
fact that it would require users to use a different parameter and us to
support two parameters unless we decide to deprecate @base.

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

I’m not sure whether I agree with skipping filters for the qcow2
metadata, just because then it’s different from the runtime state.  But
OTOH I would expect that any application that seriously cares about
filters would override the qcow2 metadata anyway, so I think I do agree
after all.

Yeah, I think skipping filters for the backing file name in the qcow2
header is right.

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

Who doesn’t...

Max


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