Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size

[Vladimir Sementsov-Ogievskiy]

On 08.03.24 18:51, Fiona Ebner wrote:
> Useful to make discard-source work in the context of backup fleecing
> when the fleecing image has a larger granularity than the backup
> target.
>
> Copy-before-write operations will use at least this granularity and in
> particular, discard requests to the source node will too. If the
> granularity is too small, they will just be aligned down in
> cbw_co_pdiscard_snapshot() and thus effectively ignored.
>
> The QAPI uses uint32 so the value will be non-negative, but still fit
> into a uint64_t.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   block/block-copy.c         | 17 +++++++++++++----
>   block/copy-before-write.c  |  3 ++-
>   include/block/block-copy.h |  1 +
>   qapi/block-core.json       |  8 +++++++-
>   4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..adb1cbb440 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -310,6 +310,7 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool 
> use_copy_range,
>   }
>   
>   static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> +                                                 int64_t min_cluster_size,

Maybe better use uint32_t here as well.

>                                                    Error **errp)
>   {
>       int ret;
> @@ -335,7 +336,7 @@ static int64_t 
> block_copy_calculate_cluster_size(BlockDriverState *target,
>                       "used. If the actual block size of the target exceeds "
>                       "this default, the backup may be unusable",
>                       BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> -        return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> +        return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
>       } else if (ret < 0 && !target_does_cow) {
>           error_setg_errno(errp, -ret,
>               "Couldn't determine the cluster size of the target image, "
> @@ -345,16 +346,18 @@ static int64_t 
> block_copy_calculate_cluster_size(BlockDriverState *target,
>           return ret;
>       } else if (ret < 0 && target_does_cow) {
>           /* Not fatal; just trudge on ahead. */
> -        return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> +        return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
>       }
>   
> -    return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> +    return MAX(min_cluster_size,
> +               MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
>   }
>   
>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>                                        BlockDriverState *copy_bitmap_bs,
>                                        const BdrvDirtyBitmap *bitmap,
>                                        bool discard_source,
> +                                     int64_t min_cluster_size,

and here why not uint32_t

>                                        Error **errp)
>   {
>       ERRP_GUARD();
> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>   
>       GLOBAL_STATE_CODE();
>   
> -    cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
> +    if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
> +        error_setg(errp, "min-cluster-size needs to be a power of 2");
> +        return NULL;
> +    }
> +
> +    cluster_size = block_copy_calculate_cluster_size(target->bs,
> +                                                     min_cluster_size, errp);
>       if (cluster_size < 0) {
>           return NULL;
>       }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index dac57481c5..f9896c6c1e 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -476,7 +476,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
> int flags,
>   
>       s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
>       s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> -                                  flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> +                                  flags & BDRV_O_CBW_DISCARD_SOURCE,
> +                                  opts->min_cluster_size, errp);

I assume it is guaranteed to be 0 when not specified by user.

>       if (!s->bcs) {
>           error_prepend(errp, "Cannot create block-copy-state: ");
>           return -EINVAL;
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..77857c6c68 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>                                        BlockDriverState *copy_bitmap_bs,
>                                        const BdrvDirtyBitmap *bitmap,
>                                        bool discard_source,
> +                                     int64_t min_cluster_size,
>                                        Error **errp);
>   
>   /* Function should be called prior any actual copy request */
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0a72c590a8..85c8f88f6e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4625,12 +4625,18 @@
>   #     @on-cbw-error parameter will decide how this failure is handled.
>   #     Default 0. (Since 7.1)
>   #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +#     operations.  Has to be a power of 2.  No effect if smaller than
> +#     the maximum of the target's cluster size and 64 KiB.  Default 0.
> +#     (Since 9.0)

will have to update to 9.1

> +#
>   # Since: 6.2
>   ##
>   { 'struct': 'BlockdevOptionsCbw',
>     'base': 'BlockdevOptionsGenericFormat',
>     'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> -            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
> +            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
> +            '*min-cluster-size': 'uint32' } }
>   
>   ##
>   # @BlockdevOptions:


Otherwise, looks good to me.

--
Best regards,
Vladimir