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

[John Snow <jsnow@redhat.com>]

On 03/16/2015 04:44 PM, Max Reitz wrote:
> 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?
>

Somewhat a leftover from an earlier revision when not every caller 
actually cared to receive the BDS for a bitmap lookup -- There is the 
assumption that maybe certain callers don't care, already know the BDS, etc.

In these cases maybe the lock isn't important because they already have 
a lock from acquiring the BDS.

Impossible to say, anyway, since nobody uses the function as such right 
now, so it might be just as good to remove the optional-ness of these 
parameters "for now."

>> +    }
>> +
>> +    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
>