From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40720) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XteuC-00073y-6i for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:53:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xteu5-00077i-V1 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:53:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47229) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xteu5-00077a-Nn for qemu-devel@nongnu.org; Wed, 26 Nov 2014 10:53:17 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAQFrGLV030323 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 26 Nov 2014 10:53:17 -0500 Message-ID: <5475F76A.5030409@redhat.com> Date: Wed, 26 Nov 2014 16:53:14 +0100 From: Max Reitz 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> <5475F42A.5030900@redhat.com> In-Reply-To: <5475F42A.5030900@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: John Snow , qemu-devel@nongnu.org Cc: Fam Zheng 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 >>> >>> 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. 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.