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

[Vladimir Sementsov-Ogievskiy]

14.01.2022 20:47, Hanna Reitz wrote:
> On 22.12.21 18:40, 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 | 30 +++++++++++++++++++++++++++++-
> >   2 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 1d3dd9cb48..6904daeacf 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4167,11 +4167,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.
> > +#
> >   # 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..4cd90d22df 100644
> > --- a/block/copy-before-write.c
> > +++ b/block/copy-before-write.c
> > @@ -149,6 +149,7 @@ 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 +164,33 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
> > int flags,
> >           return -EINVAL;
> >       }
> > +    if (qdict_haskey(options, "bitmap.node") ||
> > +        qdict_haskey(options, "bitmap.name"))
> > +    {
> > +        const char *bitmap_node, *bitmap_name;
> > +
> > +        if (!qdict_haskey(options, "bitmap.node")) {
> > +            error_setg(errp, "bitmap.node is not specified");
> > +            return -EINVAL;
> > +        }
> > +
> > +        if (!qdict_haskey(options, "bitmap.name")) {
> > +            error_setg(errp, "bitmap.name is not specified");
> > +            return -EINVAL;
> > +        }
> > +
> > +        bitmap_node = qdict_get_str(options, "bitmap.node");
> > +        bitmap_name = qdict_get_str(options, "bitmap.name");
> > +        qdict_del(options, "bitmap.node");
> > +        qdict_del(options, "bitmap.name");
>
> I’m not really a fan of this manual parsing, but I can see nothing technically 
> wrong with it.
>
> Still, what do you think of using an input visitor, like:
>
> QDict *bitmap_qdict;
>
> qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
> if (qdict_size(bitmap_qdict) > 0) {
>      BlockDirtyBitmap *bmp_param;
>      Visitor *v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
>      visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
>      visit_free(v);
>      qobject_unref(bitmap_qdict);
>
>      bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, ...);
>      qapi_free_BlockDirtyBitmap(bmp_param);
> }
>
> (+ error handling, which is why perhaps the first block should be put into a 
> separate function cbw_get_bitmap_param() to simplify error handling)

Will try. Hmm. At some point we should start to generate _marshal_ wrappers and 
 handle _open() realizations like we do we qmp commands..

> > +
> > +        bitmap = block_dirty_bitmap_lookup(bitmap_node, bitmap_name, NULL,
> > +                                           errp);
> > +        if (!bitmap) {
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> >       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 +198,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;


--
Best regards,
Vladimir