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

[John Snow <jsnow@redhat.com>]

On 11/26/2014 07:19 AM, Max Reitz wrote:
> 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.

I found other cases where we do this in the code, I actually thought it 
was a "style thing," which is why I did it so often.

I'll happily cut it out.

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

Trusting lookup to error out.

>> +
>> +    bs = bdrv_lookup_bs(device, NULL, &local_err);
>> +    if (!bs) {
>> +        error_propagate(errp, local_err);
>
> Same thing about error_propagate() here.

Same.

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

No, just another bonehead moment.

Thanks for the reviews.