Re: [PULL 14/53] block: apply COR-filter to block-stream jobs

[Philippe Mathieu-Daudé]

Hi Andrey,

On 1/26/21 3:19 PM, Max Reitz 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>
> Message-Id: <20201216061703.70908-14-vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.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
...
> @@ -228,7 +211,9 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>      bool bs_read_only;
>      int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>      BlockDriverState *base_overlay;
> +    BlockDriverState *cor_filter_bs = NULL;
>      BlockDriverState *above_base;
> +    QDict *opts;
>  
>      assert(!(base && bottom));
>      assert(!(backing_file_str && bottom));
> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>          }
>      }
>  
> -    if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
> -        return;
> -    }
> -
>      /* 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) {
> +            return;
>          }
>      }
>  
> -    /* Prevent concurrent jobs trying to modify the graph structure here, we
> -     * already have our own plans. Also don't allow resize as the image size 
> is
> -     * queried only at the job start and then cached. */
> -    s = block_job_create(job_id, &stream_job_driver, NULL, bs,
> -                         basic_flags | BLK_PERM_GRAPH_MOD,
> +    opts = qdict_new();

Coverity reported (CID 1445793) that this resource could be leaked...

> +
> +    qdict_put_str(opts, "driver", "copy-on-read");
> +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> +    /* Pass the base_overlay node name as 'bottom' to COR driver */
> +    qdict_put_str(opts, "bottom", base_overlay->node_name);
> +    if (filter_node_name) {
> +        qdict_put_str(opts, "node-name", filter_node_name);
> +    }
> +
> +    cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
> +    if (!cor_filter_bs) {

... probably here.

Should we call g_free(opts) here?

> +        goto fail;
> +    }
> +
> +    if (!filter_node_name) {
> +        cor_filter_bs->implicit = true;
> +    }
> +
> +    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
> +                         BLK_PERM_CONSISTENT_READ,
>                           basic_flags | BLK_PERM_WRITE,
>                           speed, creation_flags, NULL, NULL, errp);
>      if (!s) {
>          goto fail;
>      }
>  
> +    /*
> +     * Prevent concurrent jobs trying to modify the graph structure here, we
> +     * already have our own plans. Also don't allow resize as the image size 
> is
> +     * queried only at the job start and then cached.
> +     */
> +    if (block_job_add_bdrv(&s->common, "active node", bs, 0,
> +                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
> +        goto fail;
> +    }
> +
>      /* Block all intermediate nodes between bs and base, because they will
>       * disappear from the chain after this operation. The streaming job reads
>       * every block only once, assuming that it doesn't change, so forbid 
> writes
> @@ -310,9 +327,9 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>      s->base_overlay = base_overlay;
>      s->above_base = above_base;
>      s->backing_file_str = g_strdup(backing_file_str);
> +    s->cor_filter_bs = cor_filter_bs;
>      s->target_bs = bs;
>      s->bs_read_only = bs_read_only;
> -    s->chain_frozen = true;
>  
>      s->on_error = on_error;
>      trace_stream_start(bs, base, s);
> @@ -320,8 +337,10 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>      return;
>  
>  fail:
> +    if (cor_filter_bs) {
> +        bdrv_cor_filter_drop(cor_filter_bs);
> +    }
>      if (bs_read_only) {
>          bdrv_reopen_set_read_only(bs, true, NULL);
>      }
> -    bdrv_unfreeze_backing_chain(bs, above_base);
>  }
...