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

[Max Reitz]

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

> +#          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 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.

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

Max