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
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
     BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
     Error *local_err = NULL;
     int ret = 0;
 
@@ -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);
diff --git a/blockdev.c b/blockdev.c
index c290cb1dca..b58f36fc31 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2510,7 +2510,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;
-    const char *base_name = NULL;
     int job_flags = JOB_DEFAULT;
 
     if (!has_on_error) {
@@ -2538,7 +2537,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
-        base_name = base;
     }
 
     if (has_base_node) {
@@ -2553,7 +2551,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
         bdrv_refresh_filename(base_bs);
-        base_name = base_bs->filename;
     }
 
     /* Check for op blockers in the whole chain between bs and base */
@@ -2573,9 +2570,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
-    /* backing_file string overrides base bs filename */
-    base_name = has_backing_file ? backing_file : base_name;
-
     if (has_auto_finalize && !auto_finalize) {
         job_flags |= JOB_MANUAL_FINALIZE;
     }
@@ -2583,7 +2577,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         job_flags |= JOB_MANUAL_DISMISS;
     }
 
-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file,
                  job_flags, has_speed ? speed : 0, on_error,
                  filter_node_name, &local_err);
     if (local_err) {
-- 
2.25.4

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

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

On 22.12.20 18:53, Vladimir Sementsov-Ogievskiy wrote:
> 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.

It was my understanding that this was an example use case; other users 
might want to use backing-file for something else entirely.  (I imagined 
using e.g. a completely different file in a different format.)

OTOH, considering that “something else entirely” simply doesn’t work 
(because the driver has to be something from the backing chain), my 
imagination was just too wild.  If anyone should ever want to follow up 
on it, I expect them to complain, and we’ll worry about it then.

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

If it does something else than what people want it to do, they’ll 
complain. :)

(It isn’t like this patch is breaking anything that would work right now.)

So I agree that we don’t need backing-fmt now.  (Or maybe ever.)

Max

Max