[Top][All Lists]

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

Re: [Qemu-devel] [PATCH v12 02/17] qmp: Add block-dirty-bitmap-add and b

From: John Snow
Subject: Re: [Qemu-devel] [PATCH v12 02/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
Date: Fri, 13 Feb 2015 17:39:12 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

Missed you by several seconds, I submitted a v13 fixup to cover Max's comments and had wrongly assumed I wouldn't be hearing anything else at 5PM on a Friday :)

On 02/13/2015 05:24 PM, Eric Blake wrote:
On 02/09/2015 06:35 PM, John Snow wrote:
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.

I haven't been following this series closely (I know I should be doing
that, though).  Is the bitmap associated with the BDS (a host resource,
independent of which device(s) are currently viewing that content) or
with the BlockBackend (only one bitmap namespace per device)?  I'm a bit
worried that we will WANT to have bitmaps associated with BDS (if we
don't already) because of image fleecing.  That is, if we start with:

base <- mid <- active

and request an image fleecing operation, we want:

base <- mid <- active
             \- overlay

Currently: I am intending to allow users to attach them to any old BDS, per-node. However, they are currently only /useful/ if you attach them to the root, since that's what Drive Backup is going to operate on.

where overlay serves the NBD that sees the point in time. If we then
allow a block-commit, then writes to 'mid' containing the content from
'active' will trigger another write to 'overlay' with the pre-modified
contents, so that the NBD fleecing operation doesn't see any changes.
If we then migrate, it means we need multiple bitmaps: the map for the
commit of active into mid (how much remains to be committed), and the
map for mid to overlay (how much of mid has been changed since the
point-in-time overlay was created).

By associating bitmaps with a device (a BB), rather than a BDS, you may
be artificially limiting which operations can be performed.  On the
other hand, if you associate with a BDS, and we improve things to allow
arbitrary refactoring relationships where a BDS can be in more than one
tree at once, it starts to be hard to prove that bitmap names won't be

Am I overthinking something here, or are we okay limiting bitmap names
to just the BB device, rather than a BDS?

Not my intention to create an artificial limitation, just an implicit one: There are currently no users of named BdrvDirtyBitmaps that act per-node.

You may review this series under this premise and yell at me if I have diverged from this assumption.

The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.

This patch also introduces the 'block_dirty_bitmap_lookup' helper,
which takes a device name and a dirty bitmap name and validates the
lookup, returning NULL and setting errp if there is a problem with
either field. This helper will be re-used in future patches in this

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}'

Signed-off-by: Fam Zheng <address@hidden>
Signed-off-by: John Snow <address@hidden>
  block.c               |  20 ++++++++++
  block/mirror.c        |  10 +----
  blockdev.c            | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
  include/block/block.h |   1 +
  qapi/block-core.json  |  55 +++++++++++++++++++++++++++
  qmp-commands.hx       |  51 +++++++++++++++++++++++++
  6 files changed, 228 insertions(+), 9 deletions(-)

+++ b/blockdev.c
@@ -1173,6 +1173,48 @@ out_aio_context:
      return NULL;

+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names. Returns NULL on error,
+ * including when the BDS and/or bitmap is not found.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+                                                  const char *name,
+                                                  BlockDriverState **pbs,
+                                                  Error **errp)
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+    if (!node) {
+        error_setg(errp, "Node cannot be NULL");
+        return NULL;
+    }

Node tends to be the term we use for BDS, rather than for device BB.

So far so good.

+    if (!name) {
+        error_setg(errp, "Bitmap name cannot be NULL");
+        return NULL;
+    }
+    bs = bdrv_lookup_bs(node, node, NULL);
+    if (!bs) {
+        error_setg(errp, "Node '%s' not found", node);
+        return NULL;
+    }
+    /* If caller provided a BDS*, provide the result of that lookup, too. */
+    if (pbs) {
+        *pbs = bs;
+    }
+    bitmap = bdrv_find_dirty_bitmap(bs, name);

and this seems to say that bitmap names are per-BDS, not per-device.

We're still on the same page.

+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap '%s' not found", name);
+        return NULL;
+    }
+    return bitmap;
  /* New and old BlockDriverState structs for atomic group operations */
+++ b/qapi/block-core.json
@@ -959,6 +959,61 @@
              '*on-target-error': 'BlockdevOnError' } }

+# @BlockDirtyBitmap
+# @node: name of device/node which the bitmap is tracking
+# @name: name of the dirty bitmap
+# Since 2.3
+{ 'type': 'BlockDirtyBitmap',
+  'data': { 'node': 'str', 'name': 'str' } }

This naming implies that bitmap is a per-BDS option, but that as a
convenience, we allow a device name (BB) as shorthand for the top-level
BDS associated with the BB.  I can live with that.

It sounds like I am not going to the mental facility after all. This is what I wanted.

+++ b/qmp-commands.hx
@@ -1244,6 +1244,57 @@ Example:

+        .name       = "block-dirty-bitmap-add",
+        .args_type  = "node:B,name:s,granularity:i?",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+    },
+    {
+        .name       = "block-dirty-bitmap-remove",
+        .args_type  = "node:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+    },

I don't know if it is worth interleaving these declarations...

I can demingle them if I must; I just carried forth the convention in the original patches I received and nobody has mentioned it yet.

+Since 2.3
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+- "node": device/node on which to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with (int, optional)
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0",
+                                                   "name": "bitmap0" } }
+<- { "return": {} }
+Since 2.3

...to align with their examples. I don't see much else grouping in this

OK, if a v14 is needed for other reasons I will decouple the examples in this file.

+Stop write tracking and remove the dirty bitmap that was created with
+- "node": device/node on which to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0",
+                                                      "name": "bitmap0" } }
+<- { "return": {} }
+    {
          .name       = "blockdev-snapshot-sync",
          .args_type  = 
          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,

The interface seems okay; I'm not sure if there are any tweaks we need
to the commit message or documentation.

reply via email to

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