Re: [PATCH v14 05/13] qapi: create BlockdevOptionsCor structure for COR driver

[Max Reitz <mreitz@redhat.com>]

On 10.12.20 19:30, Vladimir Sementsov-Ogievskiy wrote:
> 10.12.2020 20:43, Max Reitz wrote:
>> I don’t like this patch’s subject very much, because I find the 
>> implementation of the @bottom option to be more noteworthy than the 
>> addition of the QAPI structure.
>>
>>
>> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>
>>> Create the BlockdevOptionsCor structure for COR driver specific options
>>> splitting it off form the BlockdevOptionsGenericFormat. The only option
>>> 'bottom' node in the structure denotes an image file that limits the
>>> COR operations in the backing chain.
>>> We are going to use the COR-filter for a block-stream job and will pass
>>> a bottom node name to the COR driver. The bottom node is the first
>>> non-filter overlay of the base. It was introduced because the base node
>>> itself may change due to possible concurrent jobs.
>>>
>>> Suggested-by: Max Reitz <mreitz@redhat.com>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>    [vsementsov: fix bdrv_is_allocated_above() usage]
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json | 21 +++++++++++++++-
>>>   block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++--
>>>   2 files changed, 75 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 8ef3df6767..04055ef50c 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3942,6 +3942,25 @@
>>>     'data': { 'throttle-group': 'str',
>>>               'file' : 'BlockdevRef'
>>>                } }
>>> +
>>> +##
>>> +# @BlockdevOptionsCor:
>>> +#
>>> +# Driver specific block device options for the copy-on-read driver.
>>> +#
>>> +# @bottom: the name of a non-filter node (allocation-bearing layer) 
>>> that limits
>>> +#          the COR operations in the backing chain (inclusive).
>>
>> This seems to me like something’s missing.  Perhaps technically there 
>> isn’t, but “limits the COR operations” begs the question (to me) 
>> “Limits them in what way?” (to which the answer is: No data below 
>> @bottom is copied).
>>
>> Could you make it more verbose?  Perhaps something like “The name of a 
>> non-filter node (allocation-bearing layer) that limits the COR 
>> operations in the backing chain (inclusive), so that no data below 
>> this node will be copied by this filter”?
> 
> Sounds good for me.
> 
>>
>>> +#          For the block-stream job, it will be the first non-filter 
>>> overlay of
>>> +#          the base node. We do not involve the base node into the COR
>>> +#          operations because the base may change due to a concurrent
>>> +#          block-commit job on the same backing chain.
>>
> 
> I now see that paragraph conflicts with further introduce of "bottom" 
> for stream job itself. I think it may be safely dropped. It's a wrong 
> place to describe how block-stream works.
> 
>> I think the default behavior should be mentioned here somewhere, i.e. 
>> that no limit is applied, so that data from all backing layers may be 
>> copied.
> 
> agree
> 
>>
>>> +#
>>> +# Since: 5.2
>>
>> *6.0
>>
>>> +##
>>> +{ 'struct': 'BlockdevOptionsCor',
>>> +  'base': 'BlockdevOptionsGenericFormat',
>>> +  'data': { '*bottom': 'str' } }
>>> +
>>>   ##
>>>   # @BlockdevOptions:
>>>   #
>>
>> [...]
>>
>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>> index 618c4c4f43..67f61983c0 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>
>> [...]
>>
>>> @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>>>               bs->file->bs->supported_zero_flags);
>>> +    if (bottom_node) {
>>> +        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
>>> +        if (!bottom_bs) {
>>> +            error_setg(errp, "Bottom node '%s' not found", 
>>> bottom_node);
>>> +            qdict_del(options, "bottom");
>>> +            return -EINVAL;
>>> +        }
>>
>> Should we verify that bottom_bs is not a filter, as required by the 
>> schema?
>>
> 
> yes, thanks for the catch!
> 
> 
> Hmm.. Interesting, we don't freeze the backing chain in cor filter open. 
> And I think we shouldn't. But then, bottom node may disappear. We should 
> handle it without a crash.
> 
> I suggest:
> 
> 1. document, that if bottom node disappear from the backing chain of the 
> filter, it continues to work like without any specified "bottom" node
> 
> 2. do bdrv_ref/bdrv_unref of bottom_bs, to not work with dead pointer
> 
> 3. check in cor_co_preadv_part() is bottom_bs is still in backing chain 
> or not

Hm, right.

Alternatively, we could also freeze the chain until the bottom node and 
then allow changing the @bottom node through reopen.  Then it would have 
to be manually unset before the bottom node is allowed to disappear from 
the chain.

Would freezing the chain pose a problem?

Max