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.shinkev...@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
signature.asc
Description: OpenPGP digital signature