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

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, qemu-devel@nongnu.org,
	armbru@redhat.com, stefanha@redhat.com,
	andrey.shinkevich@virtuozzo.com, den@openvz.org,
	jsnow@redhat.com
Subject: Re: [PATCH v14 10/13] qapi: block-stream: add "bottom" argument
Date: Fri, 11 Dec 2020 19:50:47 +0300	[thread overview]
Message-ID: <9e131589-84b5-761b-3e26-c4e8eb32ec09@virtuozzo.com> (raw)
In-Reply-To: <79993af7-1993-5af1-d343-04f208b03d73@redhat.com>

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.

> 
> [...]
> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 70900f4f77..e0e19db88b 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
> 
> [...]
> 
>> @@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>>           bdrv_refresh_filename(base_bs);
>>       }
>> -    /* Check for op blockers in the whole chain between bs and base */
>> -    for (iter = bs; iter && iter != base_bs;
>> +    if (has_bottom) {
>> +        bottom_bs = bdrv_lookup_bs(NULL, bottom, errp);
>> +        if (!bottom_bs) {
>> +            goto out;
>> +        }
>> +        if (!bottom_bs->drv) {
>> +            error_setg(errp, "Node '%s' is not open", bottom);
>> +            goto out;
>> +        }
>> +        if (bottom_bs->drv->is_filter) {
>> +            error_setg(errp, "Node '%s' is filter, use non-filter node"
>> +                       "as 'bottom'", bottom);
> 
> Missing a space between “node” and “as”.  (Also, probably two articles, i.e. “Node '%s' is a filter, use a non-filter node...”.)
> 
> The rest looks good to me, but I’m withholding my R-b because I haven’t understood why using @bottom precludes giving @backing-file.
> 
> Max
> 

-- 
Best regards,
Vladimir