Re: [PATCH v14 10/13] qapi: block-stream: add "bottom" argument

[Max Reitz]

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:
> > > 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 
> > > > > backing
> > > > >     chain, 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" in
> > > > >     the 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-file
> > > >
> > > > s/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.
> > 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.
> >
> > (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