Re: [PATCH v4 04/18] block/copy-before-write: add bitmap open parameter

[Hanna Reitz <hreitz@redhat.com>]

On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote:
> This brings "incremental" mode to copy-before-write filter: user can
> specify bitmap so that filter will copy only "dirty" areas.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json      | 10 +++++++-
>   block/copy-before-write.c | 51 ++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9a5a3641d0..3bab597506 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4171,11 +4171,19 @@
>   #
>   # @target: The target for copy-before-write operations.
>   #
> +# @bitmap: If specified, copy-before-write filter will do
> +#          copy-before-write operations only for dirty regions of the
> +#          bitmap. Bitmap size must be equal to length of file and
> +#          target child of the filter. Note also, that bitmap is used
> +#          only to initialize internal bitmap of the process, so further
> +#          modifications (or removing) of specified bitmap doesn't
> +#          influence the filter.

Sorry, missed this last time: There should be a “since: 7.0” here.

> +#
>   # Since: 6.2
>   ##
>   { 'struct': 'BlockdevOptionsCbw',
>     'base': 'BlockdevOptionsGenericFormat',
> -  'data': { 'target': 'BlockdevRef' } }
> +  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
>   
>   ##
>   # @BlockdevOptions:
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 799223e3fb..91a2288b66 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -34,6 +34,8 @@
>   
>   #include "block/copy-before-write.h"
>   
> +#include "qapi/qapi-visit-block-core.h"
> +
>   typedef struct BDRVCopyBeforeWriteState {
>       BlockCopyState *bcs;
>       BdrvChild *target;
> @@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
>       }
>   }
>   
> +static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
> +                                    Error **errp)
> +{
> +    QDict *bitmap_qdict = NULL;
> +    BlockDirtyBitmap *bmp_param = NULL;
> +    Visitor *v = NULL;
> +    bool ret = false;
> +
> +    *bitmap = NULL;
> +
> +    qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
> +    if (!qdict_size(bitmap_qdict)) {
> +        ret = true;
> +        goto out;
> +    }
> +
> +    v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
> +    if (!v) {
> +        goto out;
> +    }
> +
> +    visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
> +    if (!bmp_param) {
> +        goto out;
> +    }
> +
> +    *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
> +                                        errp);
> +    if (!*bitmap) {
> +        goto out;
> +    }
> +
> +    ret = true;
> +
> +out:
> +    qapi_free_BlockDirtyBitmap(bmp_param);
> +    visit_free(v);
> +    qobject_unref(bitmap_qdict);
> +
> +    return ret;
> +}
> +
>   static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>                       Error **errp)
>   {
>       BDRVCopyBeforeWriteState *s = bs->opaque;
> +    BdrvDirtyBitmap *bitmap = NULL;
>   
>       bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>                                  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> @@ -163,6 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>           return -EINVAL;
>       }
>   
> +    if (!cbw_parse_bitmap_option(options, &bitmap, errp)) {
> +        return -EINVAL;

Hm...  Just to get a second opinion on this: We don’t need to close 
s->target here, because the failure paths of bdrv_open_inherit() and 
bdrv_new_open_driver_opts() both call bdrv_unref(), which will call 
bdrv_close(), which will close all children including s->target, right?

> +    }
> +
>       bs->total_sectors = bs->file->bs->total_sectors;
>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>               (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
> @@ -170,7 +219,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, NULL, errp);
> +    s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp);
>       if (!s->bcs) {
>           error_prepend(errp, "Cannot create block-copy-state: ");
>           return -EINVAL;