qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v12 06/17] block: Add bitmap successors


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v12 06/17] block: Add bitmap successors
Date: Wed, 11 Feb 2015 11:50:01 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

On 2015-02-09 at 20:35, John Snow wrote:
A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
be created just prior to a sensitive operation (e.g. Incremental Backup)
that can either succeed or fail, but during the course of which we still
want a bitmap tracking writes.

On creating a successor, we "freeze" the parent bitmap which prevents
its deletion, enabling, anonymization, or creating a bitmap with the
same name.

On success, the parent bitmap can "abdicate" responsibility to the
successor, which will inherit its name. The successor will have been
tracking writes during the course of the backup operation. The parent
will be safely deleted.

On failure, we can "reclaim" the successor from the parent, unifying
them such that the resulting bitmap describes all writes occurring since
the last successful backup, for instance. Reclamation will thaw the
parent, but not explicitly re-enable it.

BdrvDirtyBitmap operations that target a single bitmap are protected
by assertions that the bitmap is not frozen and/or disabled.

BdrvDirtyBitmap operations that target a group of bitmaps, such as
bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
conditional instead.

QMP transactions that enable/disable bitmaps have extra error checking
surrounding them that prevent modifying bitmaps that are frozen.

Signed-off-by: John Snow <address@hidden>
---
  block.c               | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
  blockdev.c            |  22 +++++++++++
  include/block/block.h |  10 +++++
  qapi/block-core.json  |   3 ++
  4 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 8d63375..8d84ace 100644
--- a/block.c
+++ b/block.c
@@ -51,8 +51,17 @@
  #include <windows.h>
  #endif
+/**
+ * A BdrvDirtyBitmap can be in three possible states:
+ * (1) successor is false and disabled is false: full r/w mode
+ * (2) successor is false and disabled is true: read only mode ("disabled")
+ * (3) successor is set: frozen mode.
+ *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
+ *     or enabled. A frozen bitmap can only abdicate() or reclaim().
+ */
  struct BdrvDirtyBitmap {
      HBitmap *bitmap;
+    BdrvDirtyBitmap *successor;
      char *name;
      bool disabled;
      QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5383,6 +5392,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
*bs, const char *name)
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {
+    assert(!bdrv_dirty_bitmap_frozen(bitmap));
      g_free(bitmap->name);
      bitmap->name = NULL;
  }
@@ -5418,9 +5428,98 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
      return bitmap;
  }
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->successor;
+}
+
  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
  {
-    return !bitmap->disabled;
+    return !(bitmap->disabled || bitmap->successor);
+}
+
+/**
+ * Create a successor bitmap destined to replace this bitmap after an 
operation.
+ * Requires that the bitmap is not frozen and has no successor.
+ */
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+                                       BdrvDirtyBitmap *bitmap, Error **errp)
+{
+    uint64_t granularity;
+    BdrvDirtyBitmap *child;
+
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        error_setg(errp, "Cannot create a successor for a bitmap that is "
+                   "currently frozen.");

Hm, I seem to remember Markus saying something about full stops at the end of error messages...

+        return -1;
+    }
+    assert(!bitmap->successor);
+
+    /* Create an anonymous successor */
+    granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+    if (!child) {
+        return -1;
+    }
+
+    /* Successor will be on or off based on our current state. */
+    child->disabled = bitmap->disabled;
+
+    /* Install the successor and freeze the parent */
+    bitmap->successor = child;
+    return 0;
+}
+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * Delete the old bitmap, and return a handle to the new bitmap.
+ */
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+                                            BdrvDirtyBitmap *bitmap,
+                                            Error **errp)
+{
+    char *name;
+    BdrvDirtyBitmap *successor = bitmap->successor;
+
+    if (successor == NULL) {
+        error_setg(errp, "Cannot relinquish control if "
+                   "there's no successor present.\n");

So while I'm unsure about full stops, I am pretty sure that we don't really want newlines.

With the full stops and newlines removed (in all error_setg() calls in this patch):

Reviewed-by: Max Reitz <address@hidden>



reply via email to

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