|
From: | Max Reitz |
Subject: | Re: [PATCH v14 10/13] qapi: block-stream: add "bottom" argument |
Date: | Fri, 11 Dec 2020 18:52:45 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 |
On 11.12.20 18:42, Vladimir Sementsov-Ogievskiy wrote:
11.12.2020 20:24, Max Reitz wrote:On 11.12.20 17:50, Vladimir Sementsov-Ogievskiy wrote:Before I can agree on removing backing-file (or deprecating it), I need to know what it’s actually used for. I actually don’t, though. The only reason I could imagine was because the user wanted to write some string into there that is different from base.filename.11.12.2020 19:05, Max Reitz wrote:On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:The code already don't freeze base node and we try to make it prepared for the situation when base node is changed during the operation. In other words, block-stream doesn't own base node. Let's introduce a new interface which should replace the current one, which will in better relations with the code. Specifying bottom node instead of base, and requiring it to be non-filter gives us the following benefits: - drop difference between above_base and base_overlay, which will be renamed to just bottom, when old interface dropped- clean way to work with parallel streams/commits on the same backingchain, which otherwise become a problem when we introduce a filter for stream job- cleaner interface. Nobody will surprised the fact that base node may disappear during block-stream, when there is no word about "base" inthe interface. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 8 +++-- include/block/block_int.h | 1 + block/monitor/block-hmp-cmds.c | 3 +- block/stream.c | 50 +++++++++++++++++++---------blockdev.c | 61 ++++++++++++++++++++++++++++------5 files changed, 94 insertions(+), 29 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 04055ef50c..5d6681a35d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2522,6 +2522,10 @@ # @base-node: the node name of the backing file. # It cannot be set if @base is also set. (Since 2.8) # +# @bottom: the last node in the chain that should be streamed into+# top. It cannot be set any of @base, @base-node or @backing-files/set any/set if any/But what’s the problem with backing-file? The fact that specifying backing-file means that stream will look for that filename in the backing chain when the job is done (so if you use @bottom, we generally don’t want to rely on the presence of any nodes below it)?I just wanted to deprecate 'backing-file' together with base and base-node as a next step. If user wants to set backing file unrelated to current backing-chain, is it correct at all? It's a direct violation of what's going on, and I doubt that other parts of Qemu working with backing-file are prepared for such situation. User can do it by hand later.. Anyway, we'll have three releases deprecation period for people to come and cry that this is a really needed option, so we can support it later on demand.(If so, I would have thought that we actually want the user to specify backing-file so we don’t have to look down below @bottom to look for a filename. Perhaps a @backing-fmt parameter would help.)If we decide that 'backing-file' is really needed, than yes we should require backing-fmt to be specified together with backing-file when using new "bottom" interface.(The original commit 13d8cc515df does mention cases like FD passing, where qemu has no idea what an appropriate filename would be (it can only see /dev/fd/*). From that, it does appear to me that it’ll be needed even with @bottom.)I should have checked it myself.. That's one more reason for my "RFC: don't store backing filename in qcow2 image"..OK, do you think we can require backing-fmt to be specified if backing-file and bottom are specified?
Sure.
Or allow omitting it and deprecate this thing? We actually already have deprecation message in bdrv_change_backing_file(), and how we are trying to workaround it in block-stream will not work with file descriptors anyway (hmm, and old code works, so, actually 09 is a regression?)
I think requiring backing-fmt for bottom + backing-file would be the most simple and clean way, hopefully saving us some headaches.
Max
[Prev in Thread] | Current Thread | [Next in Thread] |