qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
Date: Wed, 26 Nov 2014 16:53:14 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

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 <address@hidden>

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 <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
  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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]