Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs

[Max Reitz <mreitz@redhat.com>]

On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> This patch completes the series with the COR-filter applied to
> block-stream operations.
> 
> Adding the filter makes it possible in future implement discarding
> copied regions in backing files during the block-stream job, to reduce
> the disk overuse (we need control on permissions).
> 
> Also, the filter now is smart enough to do copy-on-read with specified
> base, so we have benefit on guest reads even when doing block-stream of
> the part of the backing chain.
> 
> Several iotests are slightly modified due to filter insertion.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/stream.c             | 105 ++++++++++++++++++++++---------------
>   tests/qemu-iotests/030     |   8 +--
>   tests/qemu-iotests/141.out |   2 +-
>   tests/qemu-iotests/245     |  20 ++++---
>   4 files changed, 80 insertions(+), 55 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 626dfa2b22..1fa742b0db 100644
> --- a/block/stream.c
> +++ b/block/stream.c

[...]

> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,

[...]

>       /* Make sure that the image is opened in read-write mode */
>       bs_read_only = bdrv_is_read_only(bs);
>       if (bs_read_only) {
> -        if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
> -            bs_read_only = false;
> -            goto fail;
> +        int ret;
> +        /* Hold the chain during reopen */
> +        if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
> +            return;
> +        }
> +
> +        ret = bdrv_reopen_set_read_only(bs, false, errp);
> +
> +        /* failure, or cor-filter will hold the chain */
> +        bdrv_unfreeze_backing_chain(bs, above_base);
> +
> +        if (ret < 0) {

Shouldn’t we keep the “bs_read_only = false;” here?

(All the rest of this patch looks good.)

Max

> +            return;
>           }
>       }