From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45055) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtbYu-0001SM-Mv for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:19:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtbYo-0005cG-Bn for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:19:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50594) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtbYo-0005c0-3X for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:19:06 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAQCJ4ZG020053 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 26 Nov 2014 07:19:05 -0500 Message-ID: <5475C535.4010705@redhat.com> Date: Wed, 26 Nov 2014 13:19:01 +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> In-Reply-To: <1416944800-17919-3-git-send-email-jsnow@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-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. > + 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? > + > + bs = bdrv_lookup_bs(device, NULL, &local_err); > + if (!bs) { > + error_propagate(errp, local_err); Same thing about error_propagate() here. > + 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