Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz <mreitz@redhat.com>]

On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
>
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'

Thanks, this helps. :-)

> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c               | 19 ++++++++++++++++
>   block/mirror.c        | 10 +-------
>   blockdev.c            | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  1 +
>   qapi/block-core.json  | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx       | 49 +++++++++++++++++++++++++++++++++++++++
>   6 files changed, 191 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index f94b753..a940345 100644
> --- a/block.c
> +++ b/block.c
> @@ -5385,6 +5385,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>       }
>   }
>   
> +#define BDB_MIN_DEF_GRANULARITY 4096
> +#define BDB_MAX_DEF_GRANULARITY 65536
> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY

You mean this is the default for the default? ;-)

> +
> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)

You may want to make this a uint64_t so it's clear that this function 
does not return errors.

> +{
> +    BlockDriverInfo bdi;
> +    int64_t granularity;
> +
> +    if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
> +        granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
> +        granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
> +    } else {
> +        granularity = BDB_DEFAULT_GRANULARITY;
> +    }
> +
> +    return granularity;
> +}
> +
>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>   {
> diff --git a/block/mirror.c b/block/mirror.c
> index 858e4ff..3633632 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -664,15 +664,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>       MirrorBlockJob *s;
>   
>       if (granularity == 0) {
> -        /* Choose the default granularity based on the target file's cluster
> -         * size, clamped between 4k and 64k.  */
> -        BlockDriverInfo bdi;
> -        if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
> -            granularity = MAX(4096, bdi.cluster_size);
> -            granularity = MIN(65536, granularity);
> -        } else {
> -            granularity = 65536;
> -        }
> +        granularity = bdrv_dbm_calc_def_granularity(target);

Maybe you should note this replacement in the commit message.

>       }
>   
>       assert ((granularity & (granularity - 1)) == 0);
> diff --git a/blockdev.c b/blockdev.c
> index 57910b8..e2fe687 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1810,6 +1810,69 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>       aio_context_release(aio_context);
>   }
>   
> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
> +                                bool has_granularity, int64_t granularity,
> +                                Error **errp)
> +{
> +    BlockDriverState *bs;
> +    Error *local_err = NULL;
> +
> +    if (!device) {
> +        error_setg(errp, "Device to add dirty bitmap to must not be null");
> +        return;
> +    }

I don't know if checking for that case makes sense, but of course it 
won't hurt. But...[1]

> +
> +    bs = bdrv_lookup_bs(device, NULL, &local_err);

Fair enough, I'd still like blk_by_name() and blk_bs() more 
(bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as 
bdrv_find() did), but this is at least not a completely trivial wrapper.

> +    if (!bs) {
> +        error_propagate(errp, local_err);

Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for 
a local Error object and error_propagate(). But I'm fine with it either way.

> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    if (has_granularity) {
> +        if (granularity < 512 || !is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and at least 512");
> +            return;
> +        }
> +    } else {
> +        /* Default to cluster size, if available: */
> +        granularity = bdrv_dbm_calc_def_granularity(bs);
> +    }
> +
> +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +    Error *local_err = NULL;

[1] why aren't you minding !device here?

> +
> +    bs = bdrv_lookup_bs(device, NULL, &local_err);
> +    if (!bs) {
> +        error_propagate(errp, local_err);

Same thing about error_propagate() here.

> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +}
> +
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
>       const char *id = qdict_get_str(qdict, "id");
> diff --git a/include/block/block.h b/include/block/block.h
> index 977f7b5..feb84e2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -437,6 +437,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
>   int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
>   void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
>   void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 569c9f5..53daf49 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -864,6 +864,64 @@
>               '*on-target-error': 'BlockdevOnError' } }
>   
>   ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               block-dirty-bitmap-add
> +#
> +# Since 2.3
> +##
> +#{ 'type': 'BlockDirtyBitmapAdd',
> +#  'base': 'BlockDirtyBitmap',
> +#  'data': { '*granularity': 'int' } }

This part of the comment doesn't seem right...

If you left it on purpose, you should add a comment like "XXX: Should 
use this representation after the code generator has been fixed to make 
it work".

Max