Re: [Qemu-devel] [2.4 PATCH v3 04/19] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz <mreitz@redhat.com>]

On 2015-03-13 at 14:30, 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            | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |   1 +
>   qapi/block-core.json  |  55 +++++++++++++++++++++++++++
>   qmp-commands.hx       |  56 +++++++++++++++++++++++++++
>   6 files changed, 235 insertions(+), 9 deletions(-)
>

[snip]

> diff --git a/blockdev.c b/blockdev.c
> index b9c1c0c..b8455b9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1161,6 +1161,53 @@ out_aio_context:
>       return NULL;
>   }
>   
> +/**
> + * Return a dirty bitmap (if present), after validating
> + * the node reference and bitmap names. Returns NULL on error,
> + * including when the BDS and/or bitmap is not found.
> + */
> +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;
> +    }
> +
> +    /* If caller provided a BDS*, provide the result of that lookup, too. */
> +    if (pbs) {
> +        assert(paio);
> +        aio_context = bdrv_get_aio_context(bs);
> +        aio_context_acquire(aio_context);
> +        *pbs = bs;
> +        *paio = aio_context;

General question (because I'm too lazy to look up, or find out where to 
look it up in the first place): Do you maybe want to acquire the AIO 
context always before bdrv_find_dirty_bitmap(), even if paio == pbs == NULL?

> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap '%s' not found", name);

I'd propose a aio_context_release(aio_context); *paio = NULL; *pbs = 
NULL; here. Makes error handling easier.

> +        return NULL;
> +    }
> +
> +    return bitmap;
> +}
> +
>   /* New and old BlockDriverState structs for atomic group operations */
>   
>   typedef struct BlkTransactionState BlkTransactionState;
> @@ -1941,6 +1988,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) {

Like here, where aio_context is not released if !bitmap && bs.

Max