From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtegl-0002cM-Vo for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:39:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xtegf-0002Ms-Nt for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:39:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtegf-0002Mm-FC for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:39:25 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAQFdNmO011260 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 26 Nov 2014 10:39:24 -0500 Message-ID: <5475F42A.5030900@redhat.com> Date: Wed, 26 Nov 2014 10:39:22 -0500 From: John Snow MIME-Version: 1.0 References: <1416944800-17919-1-git-send-email-jsnow@redhat.com> <1416944800-17919-3-git-send-email-jsnow@redhat.com> <5475C535.4010705@redhat.com> In-Reply-To: <5475C535.4010705@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Fam Zheng On 11/26/2014 07:19 AM, Max Reitz wrote: > On 2014-11-25 at 20:46, John Snow wrote: >> From: Fam Zheng >> >> 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 >> Signed-off-by: John Snow >> --- >> 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.