Re: [PATCH v3 03/19] block/block-copy: block_copy_state_new(): add bitmap parameter

[Hanna Reitz]

On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote:
> This will be used in the following commit to bring "incremental" mode
> to copy-before-write filter.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block-copy.h |  2 +-
>   block/block-copy.c         | 14 ++++++++++++--
>   block/copy-before-write.c  |  2 +-
>   3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index 99370fa38b..8da4cec1b6 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -25,7 +25,7 @@ typedef struct BlockCopyState BlockCopyState;
>   typedef struct BlockCopyCallState BlockCopyCallState;
>   
>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> -                                     Error **errp);
> +                                     BdrvDirtyBitmap *bitmap, Error **errp);
>   
>   /* Function should be called prior any actual copy request */
>   void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
> diff --git a/block/block-copy.c b/block/block-copy.c
> index abda7a80bd..f6345e3a4c 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -384,8 +384,9 @@ static int64_t 
> block_copy_calculate_cluster_size(BlockDriverState *target,
>   }
>   
>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> -                                     Error **errp)
> +                                     BdrvDirtyBitmap *bitmap, Error **errp)

Could be `const` to signal that we won’t be using this bitmap for the 
BCS, but given our inconsistent usage of `const`, it isn’t anything 
that’d be important.

>   {
> +    ERRP_GUARD();
>       BlockCopyState *s;
>       int64_t cluster_size;
>       BdrvDirtyBitmap *copy_bitmap;
> @@ -402,7 +403,16 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>           return NULL;
>       }
>       bdrv_disable_dirty_bitmap(copy_bitmap);
> -    bdrv_set_dirty_bitmap(copy_bitmap, 0, bdrv_dirty_bitmap_size(copy_bitmap));
> +    if (bitmap) {
> +        if (!bdrv_merge_dirty_bitmap(copy_bitmap, bitmap, NULL, errp)) {
> +            error_prepend(errp, "Failed to merge bitmap '%s' to internal "
> +                          "copy-bitmap: ", bdrv_dirty_bitmap_name(bitmap));
> +            return NULL;

What might be Should we free `copy_bitmap` here?

(Apart from this, looks good to me!)

> +        }
> +    } else {
> +        bdrv_set_dirty_bitmap(copy_bitmap, 0,
> +                              bdrv_dirty_bitmap_size(copy_bitmap));
> +    }
>   
>       /*
>        * If source is in backing chain of target assume that target is going to 
> be
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 5bdaf0a9d9..799223e3fb 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -170,7 +170,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
> int flags,
>               ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>                bs->file->bs->supported_zero_flags);
>   
> -    s->bcs = block_copy_state_new(bs->file, s->target, errp);
> +    s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp);
>       if (!s->bcs) {
>           error_prepend(errp, "Cannot create block-copy-state: ");
>           return -EINVAL;