[Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup

[John Snow]

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap
according to both the individual job's success and the success of all
jobs in the transaction group.

As a consequence of moving the bitmap successor cleanup actions behind
the frozen bitmap decref operation, the previous operations are made
static, their errp parameter deleted, and some user-facing errors are
replaced by assertions.

"Also," The code for cleaning up a bitmap is also moved from backup_run to
backup_complete.

Signed-off-by: John Snow <address@hidden>
---
 block.c               | 78 +++++++++++++++++++++++++++++++++++++++------------
 block/backup.c        | 20 +++++--------
 include/block/block.h | 10 +++----
 3 files changed, 71 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index a481654..1eb81ac 100644
--- a/block.c
+++ b/block.c
@@ -51,6 +51,12 @@
 #include <windows.h>
 #endif
 
+typedef enum BitmapSuccessorAction {
+    SUCCESSOR_ACTION_UNDEFINED = 0,
+    SUCCESSOR_ACTION_ABDICATE,
+    SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;
+
 /**
  * A BdrvDirtyBitmap can be in three possible states:
  * (1) successor is NULL and disabled is false: full r/w mode
@@ -65,6 +71,8 @@ struct BdrvDirtyBitmap {
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
+    int successor_refcount;     /* Number of active handles to the successor */
+    BitmapSuccessorAction act;  /* Action to take on successor upon release */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -3156,6 +3164,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
*bs,
 
     /* Install the successor and freeze the parent */
     bitmap->successor = child;
+    bitmap->successor_refcount = 1;
     return 0;
 }
 
@@ -3163,19 +3172,13 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
*bs,
  * 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)
+static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *bitmap)
 {
     char *name;
     BdrvDirtyBitmap *successor = bitmap->successor;
 
-    if (successor == NULL) {
-        error_setg(errp, "Cannot relinquish control if "
-                   "there's no successor present");
-        return NULL;
-    }
-
+    assert(successor);
     name = bitmap->name;
     bitmap->name = NULL;
     successor->name = name;
@@ -3190,19 +3193,13 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
  * 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)
+static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+                                                  BdrvDirtyBitmap *parent)
 {
     BdrvDirtyBitmap *successor = parent->successor;
 
-    if (!successor) {
-        error_setg(errp, "Cannot reclaim a successor when none is present");
-        return NULL;
-    }
-
+    assert(successor);
     if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
-        error_setg(errp, "Merging of parent and successor bitmap failed");
         return NULL;
     }
     bdrv_release_dirty_bitmap(bs, successor);
@@ -3211,6 +3208,51 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return parent;
 }
 
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *parent)
+{
+    assert(!parent->successor_refcount);
+    assert(parent->successor);
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent);
+    case SUCCESSOR_ACTION_UNDEFINED:
+    default:
+        g_assert_not_reached();
+    }
+}
+
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    if (ret) {
+        parent->act = SUCCESSOR_ACTION_RECLAIM;
+    } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) {
+        parent->act = SUCCESSOR_ACTION_ABDICATE;
+    }
+
+    parent->successor_refcount--;
+    if (parent->successor_refcount == 0) {
+        return bdrv_free_bitmap_successor(bs, parent);
+    }
+    return parent;
+}
+
+void bdrv_frozen_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
 /**
  * Truncates _all_ bitmaps attached to a BDS.
  */
diff --git a/block/backup.c b/block/backup.c
index d3f648d..4ac0be8 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque)
 
     bdrv_unref(s->target);
 
+    if (s->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        bm = bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, data->ret);
+        assert(bm);
+    }
+
     block_job_completed(job, data->ret);
     g_free(data);
 }
@@ -428,18 +434,6 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
 
-    if (job->sync_bitmap) {
-        BdrvDirtyBitmap *bm;
-        if (ret < 0) {
-            /* Merge the successor back into the parent, delete nothing. */
-            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        } else {
-            /* Everything is fine, delete this bitmap and install the backup. 
*/
-            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        }
-    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
@@ -543,6 +537,6 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 
  error:
     if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        bdrv_frozen_bitmap_decref(bs, sync_bitmap, -ECANCELED);
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 3d62d3e..b7892d2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -459,12 +459,10 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
-                                            Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *bitmap,
-                                           Error **errp);
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret);
+void bdrv_frozen_bitmap_incref(BdrvDirtyBitmap *parent);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
-- 
2.1.0