On 2014-11-25 at 20:46, John Snow wrote:
From: Fam Zheng <f...@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 <f...@redhat.com>
Signed-off-by: John Snow <js...@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.
+ 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
