Hi Andrey,
On 1/26/21 3:19 PM, Max Reitz wrote:
> From: Andrey Shinkevich <andrey.shinkev...@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.shinkev...@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> Message-Id: <20201216061703.70908-14-vsement...@virtuozzo.com>
> Reviewed-by: Max Reitz <mre...@redhat.com>
> Signed-off-by: Max Reitz <mre...@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);
> }
...
