Re: [PATCH v15 09/13] stream: rework backing-file changing

[Vladimir Sementsov-Ogievskiy]

22.12.2020 18:59, Max Reitz wrote:
> On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
> > From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> >
> > Stream in stream_prepare calls bdrv_change_backing_file() to change
> > backing-file in the metadata of bs.
> >
> > It may use either backing-file parameter given by user or just take
> > filename of base on job start.
> >
> > Backing file format is determined by base on job finish.
> >
> > There are some problems with this design, we solve only two by this
> > patch:
> >
> > 1. Consider scenario with backing-file unset. Current concept of stream
> > supports changing of the base during the job (we don't freeze link to
> > the base). So, we should not save base filename at job start,
> >
> >    - let's determine name of the base on job finish.
> >
> > 2. Using direct base to determine filename and format is not very good:
> > base node may be a filter, so its filename may be JSON, and format_name
> > is not good for storing into qcow2 metadata as backing file format.
> >
> >    - let's use unfiltered_base
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >    [vsementsov: change commit subject, change logic in stream_prepare]
> > ---
> >   block/stream.c | 9 +++++----
> >   blockdev.c     | 8 +-------
> >   2 files changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index 6e281c71ac..6a525a5edf 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
>
> [...]
>
> > @@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
> >       if (bdrv_cow_child(unfiltered_bs)) {
> >           const char *base_id = NULL, *base_fmt = NULL;
> > -        if (base) {
> > -            base_id = s->backing_file_str;
> > -            if (base->drv) {
> > -                base_fmt = base->drv->format_name;
> > +        if (unfiltered_base) {
> > +            base_id = s->backing_file_str ?: unfiltered_base->filename;
> > +            if (unfiltered_base->drv) {
> > +                base_fmt = unfiltered_base->drv->format_name;
> >               }
> >           }
> >           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>
> I think I preferred the v14 behavior of not setting a backing file format if 
> backing_file_str is nowhere to be found in the current backing chain.  (I just 
> noticed, I had a typo in my reply to v14, though; the “continuing on with 
> setting a backing_fmt” should have read “continuing on *without* setting a 
> backing_fmt”...)
>
> Anyway, this is still an improvement on the pre-patch behavior, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> (And as we discussed, the best would be for the user to specify a backing 
> format through a yet-to-be-added option.)

We discussed that the original aim of backing_file_str arg is something like 
fd-passing, when qemu doesn't know real name. In that way what was done in v14 
is a degradation: we'll never find such name in a backing chain. And acutally, 
using format of backing file is a correct thing.

So, as I understand now:

We set backing file to the node which is the new backing-bs (maybe, skipping 
some filters). Nobody should set backing in qcow2 metadata to something 
absolutely different. So, using format_name of backing bs (skipping filters) is 
a correct thing.

We want to support cases when qemu doens't know real file-names. So, trying to 
check filename, or search it in a backing chain is wrong idea..

Hmm, or when we search backing name, we really track what was written in 
backing_file field of some qcow2 image in a chain, so it should be something 
correct? Hmm, then may be v14 was OK.. But in this case again, user should not 
pass backing_file, but we can just use backing_file field of cow-parent of the 
base at job start.. Oh. Anyway, we can adjust it later in a separate series.

--
Best regards,
Vladimir