Re: [PATCH v7 07/33] block/block-copy: introduce block_copy_set_copy_opts()

[Hanna Reitz]

On 04.08.21 11:37, Vladimir Sementsov-Ogievskiy wrote:
> We'll need a possibility to set compress and use_copy_range options
> after initialization of the state. So make corresponding part of
> block_copy_state_new() separate and public.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block-copy.h |  2 ++
>   block/block-copy.c         | 66 +++++++++++++++++++++-----------------
>   2 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index 734389d32a..f0ba7bc828 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,8 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>                                        int64_t cluster_size, bool 
> use_copy_range,
>                                        bool compress, Error **errp);
>   
> +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
> +                              bool compress);
>   void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm);
>   
>   void block_copy_state_free(BlockCopyState *s);
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 58b4345a5a..84c29fb233 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -315,21 +315,11 @@ static uint32_t block_copy_max_transfer(BdrvChild 
> *source, BdrvChild *target)
>                                        target->bs->bl.max_transfer));
>   }
>   
> -BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> -                                     int64_t cluster_size, bool use_copy_range,
> -                                     bool compress, Error **errp)
> +/* Function should be called prior any actual copy request */

Given this is something the caller should know, I’d prefer putting this 
into the block-copy.h header.

However, I realize we have a wild mix of this in qemu and often do put 
such information into the C source file, so...

> +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
> +                              bool compress)
>   {
> -    BlockCopyState *s;
> -    BdrvDirtyBitmap *copy_bitmap;
>       bool is_fleecing;
> -
> -    copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL,
> -                                           errp);
> -    if (!copy_bitmap) {
> -        return NULL;
> -    }
> -    bdrv_disable_dirty_bitmap(copy_bitmap);
> -
>       /*
>        * If source is in backing chain of target assume that target is going to 
> be
>        * used for "image fleecing", i.e. it should represent a kind of snapshot 
> of
> @@ -344,24 +334,12 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>        * For more information see commit f8d59dfb40bb and test
>        * tests/qemu-iotests/222
>        */
> -    is_fleecing = bdrv_chain_contains(target->bs, source->bs);
> +    is_fleecing = bdrv_chain_contains(s->target->bs, s->source->bs);
>   
> -    s = g_new(BlockCopyState, 1);
> -    *s = (BlockCopyState) {
> -        .source = source,
> -        .target = target,
> -        .copy_bitmap = copy_bitmap,
> -        .cluster_size = cluster_size,
> -        .len = bdrv_dirty_bitmap_size(copy_bitmap),
> -        .write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) |
> -            (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
> -        .mem = shres_create(BLOCK_COPY_MAX_MEM),
> -        .max_transfer = QEMU_ALIGN_DOWN(
> -                                    block_copy_max_transfer(source, target),
> -                                    cluster_size),
> -    };
> +    s->write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) |
> +        (compress ? BDRV_REQ_WRITE_COMPRESSED : 0);

Shouldn’t we keep the is_fleecing check in block_copy_state_new()? We 
must perform it at least once, but there is no benefit in repeating it 
on every block_copy_set_copy_opts() call, right?

Hanna