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

From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, qemu-devel@nongnu.org,
	armbru@redhat.com, stefanha@redhat.com,
	andrey.shinkevich@virtuozzo.com, den@openvz.org,
	jsnow@redhat.com
Subject: Re: [PATCH v14 13/13] block: apply COR-filter to block-stream jobs
Date: Fri, 11 Dec 2020 18:21:09 +0100	[thread overview]
Message-ID: <6aa8bbb7-6745-eed3-ade6-3ef47d5ee08d@redhat.com> (raw)
In-Reply-To: <20201204220758.2879-14-vsementsov@virtuozzo.com>

On 04.12.20 23:07, 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             | 78 ++++++++++++++++++++++++++------------
>   tests/qemu-iotests/030     |  8 ++--
>   tests/qemu-iotests/141.out |  2 +-
>   tests/qemu-iotests/245     | 20 ++++++----
>   4 files changed, 72 insertions(+), 36 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index a7fd8945ad..b92f7de55b 100644
> --- a/block/stream.c
> +++ b/block/stream.c

[...]

> @@ -295,17 +287,49 @@ void stream_start(const char *job_id, BlockDriverState *bs,

[...]

> +    opts = qdict_new();
> +
> +    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);

Hm.  Should we set this option even if no base was specified?

On one hand, omitting this option would cor_co_preadv_part() a bit quicker.

On the other, what happens when you add a backing file below the bottom 
node during streaming (yes, a largely theoretical case)...  Now, all 
data from it is ignored.  That seemed a bit strange to me at first, but 
on second thought, it makes more sense.  Doing anything else would 
produce a garbage result basically, because stream_run() doesn’t take 
such a change into account.

So...  After all I think I agree with setting @bottom unconditionally.

And that’s the only comment I had. :)

Reviewed-by: Max Reitz <mreitz@redhat.com>