Re: [Qemu-devel] [PATCH v4 04/20] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz <mreitz@redhat.com>]

On 2015-03-20 at 15:16, John Snow wrote:
> The new command pair is added to manage a 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 granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.
>
> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> which takes a device name and a dirty bitmap name and validates the
> lookup, returning NULL and setting errp if there is a problem with
> either field. This helper will be re-used in future patches in this
> series.
>
> 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}'
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c               |  20 +++++++++
>   block/mirror.c        |  10 +----
>   blockdev.c            | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |   1 +
>   qapi/block-core.json  |  55 ++++++++++++++++++++++++
>   qmp-commands.hx       |  56 ++++++++++++++++++++++++
>   6 files changed, 250 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index a05971a..c5d68ff 100644
> --- a/block.c
> +++ b/block.c
> @@ -5497,6 +5497,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>       }
>   }
>   
> +/**
> + * Chooses a default granularity based on the existing cluster size,
> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there
> + * is no cluster size information available.
> + */
> +uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> +{
> +    BlockDriverInfo bdi;
> +    uint32_t granularity;
> +
> +    if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
> +        granularity = MAX(4096, bdi.cluster_size);
> +        granularity = MIN(65536, granularity);
> +    } else {
> +        granularity = 65536;
> +    }
> +
> +    return granularity;
> +}
> +
>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>   {
> diff --git a/block/mirror.c b/block/mirror.c
> index f8aac33..1cb700e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -668,15 +668,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_get_default_bitmap_granularity(target);
>       }
>   
>       assert ((granularity & (granularity - 1)) == 0);
> diff --git a/blockdev.c b/blockdev.c
> index fbb3a79..7d71b81 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1164,6 +1164,68 @@ out_aio_context:
>       return NULL;
>   }
>   
> +/**
> + * block_dirty_bitmap_lookup:
> + * Return a dirty bitmap (if present), after validating
> + * the node reference and bitmap names.
> + *
> + * @node: The name of the BDS node to search for bitmaps
> + * @name: The name of the bitmap to search for
> + * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
> + * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
> + * @errp: Output pointer for error information. Can be NULL.
> + *
> + * @return: A bitmap object on success, or NULL on failure.
> + */
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> +                                                  const char *name,
> +                                                  BlockDriverState **pbs,
> +                                                  AioContext **paio,
> +                                                  Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +    AioContext *aio_context;
> +
> +    if (!node) {
> +        error_setg(errp, "Node cannot be NULL");
> +        return NULL;
> +    }
> +    if (!name) {
> +        error_setg(errp, "Bitmap name cannot be NULL");
> +        return NULL;
> +    }
> +    bs = bdrv_lookup_bs(node, node, NULL);
> +    if (!bs) {
> +        error_setg(errp, "Node '%s' not found", node);
> +        return NULL;
> +    }
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap '%s' not found", name);
> +        goto fail;
> +    }
> +
> +    if (pbs) {
> +        *pbs = bs;
> +    }
> +    if (paio) {
> +        *paio = aio_context;
> +    } else {
> +        aio_context_release(aio_context);
> +    }
> +
> +    return bitmap;
> +
> + fail:
> +    aio_context_release(aio_context);
> +    return NULL;
> +}
> +
>   /* New and old BlockDriverState structs for atomic group operations */
>   
>   typedef struct BlkTransactionState BlkTransactionState;
> @@ -1953,6 +2015,61 @@ 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 *node, const char *name,
> +                                bool has_granularity, uint32_t granularity,
> +                                Error **errp)
> +{
> +    AioContext *aio_context;
> +    BlockDriverState *bs;
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +
> +    bs = bdrv_lookup_bs(node, node, errp);
> +    if (!bs) {
> +        return;
> +    }
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
> +    if (has_granularity) {
> +        if (granularity < 512 || !is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and at least 512");
> +            goto out;
> +        }
> +    } else {
> +        /* Default to cluster size, if available: */
> +        granularity = bdrv_get_default_bitmap_granularity(bs);
> +    }
> +
> +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +
> + out:
> +    aio_context_release(aio_context);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> +                                   Error **errp)
> +{
> +    AioContext *aio_context;
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp);
> +    if (!bitmap || !bs) {

Is there actually a way for bs to be NULL if bitmap is non-NULL? In 
fact, bs is never set to NULL by block_dirty_bitmap_lookup(), so it's 
either uninitialized or valid. If you have to respin for some reason or 
another, I think it would be nice to drop the !bs part.

Because bitmap != NULL always means bs is valid, though, this is not 
wrong, and thus with or without it:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +
> +    aio_context_release(aio_context);
> +}
> +
>   int hmp_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 5235086..0f014a3 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -458,6 +458,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);
> +uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>   int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
>   void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                              int64_t cur_sector, int nr_sectors);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 42c9d75..faadd10 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -960,6 +960,61 @@
>               '*on-target-error': 'BlockdevOnError' } }
>   
>   ##
> +# @BlockDirtyBitmap
> +#
> +# @node: name of device/node which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.4
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'node': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @node: name of device/node 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.4
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> +
> +##
> +# @block-dirty-bitmap-add
> +#
> +# Create a dirty bitmap with a name on the node
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device or node, DeviceNotFound
> +#          If @name is already taken, GenericError with an explanation
> +#
> +# Since 2.4
> +##
> +{ 'command': 'block-dirty-bitmap-add',
> +  'data': 'BlockDirtyBitmapAdd' }
> +
> +##
> +# @block-dirty-bitmap-remove
> +#
> +# Remove a dirty bitmap on the node
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device or node, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation
> +#
> +# Since 2.4
> +##
> +{ 'command': 'block-dirty-bitmap-remove',
> +  'data': 'BlockDirtyBitmap' }
> +
> +##
>   # @block_set_io_throttle:
>   #
>   # Change I/O throttle limits for a block drive.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7f68760..e9f2eb1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1270,6 +1270,62 @@ Example:
>   EQMP
>   
>       {
> +        .name       = "block-dirty-bitmap-add",
> +        .args_type  = "node:B,name:s,granularity:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
> +    },
> +
> +SQMP
> +
> +block-dirty-bitmap-add
> +----------------------
> +Since 2.4
> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "node": device/node on which to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with (int, optional)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "block-dirty-bitmap-remove",
> +        .args_type  = "node:B,name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
> +    },
> +
> +SQMP
> +
> +block-dirty-bitmap-remove
> +-------------------------
> +Since 2.4
> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-dirty-bitmap-add.
> +
> +Arguments:
> +
> +- "node": device/node on which to remove dirty bitmap (json-string)
> +- "name": name of the dirty bitmap to remove (json-string)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0",
> +                                                      "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>           .name       = "blockdev-snapshot-sync",
>           .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
>           .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,