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

[Vladimir Sementsov-Ogievskiy]

28.01.2021 21:38, Philippe Mathieu-Daudé wrote:
> 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?


Actually, not..

bdrv_insert_node() calls bdrv_open() which eats options even on failure.

I see, CID already marked false-positive?

> > +        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);
> >   }
> ...


--
Best regards,
Vladimir