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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-26 at 16:39, John Snow wrote:
>
>
> 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.

See my reply to patch 8 for an explanation.

I still think we could have made nice use of error_propagate() for 
showing some kind of backtrace (with a certain configure option), but 
that idea was rejected...

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

Well, then you don't need to test the !device case in 
qmp_block_dirty_bitmap_add() either, I think.

Max

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