Re: [Qemu-devel] [PATCH v11 08/13] block: Add bitmap successors

[Fam Zheng]

On Fri, 01/16 13:22, John Snow wrote:
> 
> 
> On 01/13/2015 04:24 AM, Fam Zheng wrote:
> >On Mon, 01/12 11:31, 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.
> >
> >If we compare this with image snapshot and overlay, it fits the current 
> >backing
> >chain model very well.  Given that we're on the way to persistent dirty 
> >bitmap,
> >which will probably be read/written with general block.c code, I'm wondering 
> >if
> >it will be any better to reuse the block layer backing file code to handle
> >"successor" logic?
> >
> >Also with the frozen feature built in dirty bitmaps, is it okay to drop
> >"enabled" state?
> >
> >I think there are three states for a bitmap:
> >
> >1) Active, no successor (similar to an read-write top image)
> >2) Frozen, no successor (similar to an read-only top image)
> >3) Frozen, with successor (similar to an read-only backing file, with an
> >    overlap)
> >
> >Admittedly, more code is needed than this patch, in order to glue hbitmap and
> >block layer together, but it would probably make live migration of dirty 
> >bitmap
> >very easy, but I'm not sure without a closer look.
> >
> >>
> >>Signed-off-by: John Snow <address@hidden>
> >>---
> >>  block.c               | 123 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  blockdev.c            |  14 ++++++
> >>  include/block/block.h |  10 ++++
> >>  3 files changed, 144 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index bd4b449..3f33b9d 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -53,10 +53,12 @@
> >>
> >>  struct BdrvDirtyBitmap {
> >>      HBitmap *bitmap;
> >>+    BdrvDirtyBitmap *successor;
> >>      int64_t size;
> >>      int64_t granularity;
> >>      char *name;
> >>      bool enabled;
> >>+    bool frozen;
> >>      QLIST_ENTRY(BdrvDirtyBitmap) list;
> >>  };
> >>
> >>@@ -5342,6 +5344,7 @@ BdrvDirtyBitmap 
> >>*bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> >>
> >>  void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
> >> *bitmap)
> >>  {
> >>+    assert(!bitmap->frozen);
> >>      g_free(bitmap->name);
> >>      bitmap->name = NULL;
> >>  }
> >>@@ -5379,9 +5382,114 @@ BdrvDirtyBitmap 
> >>*bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >>      return bitmap;
> >>  }
> >>
> >>+/**
> >>+  * A frozen bitmap cannot be renamed, deleted, cleared,
> >>+  * or set. A frozen bitmap can only abdicate, reclaim,
> >>+  * or thaw.
> >>+  */
> >>+static void bdrv_freeze_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> >>+{
> >>+    bitmap->frozen = true;
> >>+}
> >>+
> >>+static void bdrv_thaw_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> >>+{
> >>+    bitmap->frozen = false;
> >>+}
> >>+
> >>+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> >>+{
> >>+    return bitmap->frozen;
> >>+}
> >>+
> >>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> >>  {
> >>-    return bitmap->enabled;
> >>+    return bitmap->enabled && !bitmap->frozen;
> >
> >An indication that "enabled" could be replaced by "frozen". Otherwise it 
> >looks
> >confusing to me.
> >
> >>+}
> >>+
> >>+/**
> >>+ * 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;
> >>+
> >>+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> >>+        error_setg(errp,
> >>+                   "Cannot create a successor for a bitmap currently 
> >>in-use.");
> >>+        return -1;
> >>+    } else if (bitmap->successor) {
> >>+        error_setg(errp, "Cannot create a successor for a bitmap that "
> >>+                   "already has one.");
> >>+        return -1;
> >>+    }
> >>+
> >>+    /* Create an anonymous successor */
> >>+    granularity = bdrv_dirty_bitmap_granularity(bs, bitmap);
> >>+    bitmap->successor = bdrv_create_dirty_bitmap(bs, granularity, NULL, 
> >>errp);
> >>+    if (!bitmap->successor) {
> >>+        return -1;
> >>+    }
> >>+
> >>+    /* Successor will be on or off based on our current state.
> >>+     * Parent will be disabled and frozen. */
> >>+    bitmap->successor->enabled = bitmap->enabled;
> >>+    bdrv_disable_dirty_bitmap(bitmap);
> >>+    bdrv_freeze_dirty_bitmap(bitmap);
> >>+    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");
> >>+        return NULL;
> >>+    }
> >>+
> >>+    name = bitmap->name;
> >>+    bitmap->name = NULL;
> >>+    successor->name = name;
> >>+
> >>+    bdrv_thaw_dirty_bitmap(bitmap);
> >>+    bdrv_release_dirty_bitmap(bs, bitmap);
> >>+
> >>+    return successor;
> >>+}
> >>+
> >>+/**
> >>+ * In cases of failure where we can no longer safely delete the parent,
> >>+ * We may wish to re-join the parent and child/successor.
> >>+ * The merged parent will be un-frozen, but not explicitly re-enabled.
> >>+ */
> >>+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> >>+                                           BdrvDirtyBitmap *parent,
> >>+                                           Error **errp)
> >>+{
> >>+    BdrvDirtyBitmap *successor = parent->successor;
> >>+    if (!successor) {
> >>+        error_setg(errp, "Cannot reclaim a successor when none is 
> >>present.\n");
> >>+        return NULL;
> >>+    }
> >>+
> >>+    hbitmap_merge(parent->bitmap, successor->bitmap);
> >>+    bdrv_release_dirty_bitmap(bs, successor);
> >>+
> >>+    bdrv_thaw_dirty_bitmap(parent);
> >>+    parent->successor = NULL;
> >>+
> >>+    return parent;
> >>  }
> >>
> >>  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
> >> *bitmap)
> >>@@ -5389,6 +5497,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
> >>BdrvDirtyBitmap *bitmap)
> >>      BdrvDirtyBitmap *bm, *next;
> >>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> >>          if (bm == bitmap) {
> >>+            assert(!bm->frozen);
> >>              QLIST_REMOVE(bitmap, list);
> >>              hbitmap_free(bitmap->bitmap);
> >>              g_free(bitmap->name);
> >>@@ -5405,6 +5514,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap 
> >>*bitmap)
> >>
> >>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> >>  {
> >>+    assert(!bitmap->frozen);
> >>      bitmap->enabled = true;
> >>  }
> >>
> >>@@ -5483,7 +5593,9 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, 
> >>BdrvDirtyBitmap *bitmap,
> >>  void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
> >> *bitmap,
> >>                               int64_t cur_sector, int nr_sectors)
> >>  {
> >>-    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> >>+    if (!bitmap->frozen) {
> >
> >Probably just assert it's not frozen?
> >
> >>+        hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> >>+    }
> >>  }
> >>
> >>  /**
> >>@@ -5492,7 +5604,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
> >>BdrvDirtyBitmap *bitmap,
> >>   */
> >>  void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
> >> *bitmap)
> >>  {
> >>-    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> >>+    if (!bitmap->frozen) {
> >>+        hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> >
> >The same: probably just assert it's not frozen?
> >
> 
> Looking at this again, I see what I was trying to accomplish. Similar to how
> (!enabled) will silently ignore writes, I just wanted frozen bitmaps to
> silently ignore attempts to clear or reset bits, not assert or error out.
> 
> Just in case, in some future state, there is a more generic "set/clear bits"
> mechanism that may blindly try to clear bits to all attached bitmaps of a
> particular BDS, I didn't want to throw an error in that case -- just ignore
> it.
> 
> It would be a logical error to try to clear a bitmap we KNOW is frozen, but
> there may be cases where clears and resets may be done with less
> discrimination.
> 
> I think I will leave this as-is, but I can write some extra commentary for
> it. Is that OK?
> 
> --JS

OK.

I thought "better be safe than be sorry" when suggesting the stricter and more
explicit calling contract, but since this is not wrong, it's your decision.

Fam