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-16 at 16:53, John Snow wrote:
>
>
> 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.

Recursively acquiring AIO contexts is just fine. :-)

Max

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