[Qemu-devel] [PATCH 05/11] block: add delayed bitmap successor cleanup

[John Snow <jsnow@redhat.com>]

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.

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

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block.c               | 79 +++++++++++++++++++++++++++++++++++++++++++++++----
 block/backup.c        | 23 ++++++---------
 include/block/block.h | 11 ++++---
 3 files changed, 87 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 5eaa874..a0036af 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 false 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;
 };
 
@@ -5508,6 +5516,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;
 }
 
@@ -5515,9 +5524,9 @@ 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,
+                                                   Error **errp)
 {
     char *name;
     BdrvDirtyBitmap *successor = bitmap->successor;
@@ -5542,9 +5551,9 @@ 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,
+                                                  Error **errp)
 {
     BdrvDirtyBitmap *successor = parent->successor;
 
@@ -5563,6 +5572,64 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return parent;
 }
 
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *parent,
+                                                   Error **errp)
+{
+    if (parent->successor_refcount) {
+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the refcount is non-zero.", parent->name);
+        return NULL;
+    }
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_UNDEFINED:
+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the successor action has not yet been set.",
+                   parent->name);
+        return NULL;
+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent, errp);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent, errp);
+    default:
+        error_setg(errp,
+                   "Unrecognized successor action (%d), "
+                   "cannot free successor for bitmap '%s'",
+                   parent->act, parent->name);
+        return NULL;
+    }
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs,
+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp)
+{
+    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, errp);
+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
 static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
 {
     /* Should only be frozen during a block backup job, which should have
diff --git a/block/backup.c b/block/backup.c
index 41bd9af..4332df4 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_dirty_bitmap_decref(job->bs, s->sync_bitmap, data->ret, NULL);
+        assert(bm);
+    }
+
     block_job_completed(job, data->ret);
     g_free(data);
 }
@@ -419,19 +425,6 @@ leave:
     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);
-            bdrv_enable_dirty_bitmap(job->sync_bitmap);
-        } 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);
@@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
 
  error:
     if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        BdrvDirtyBitmap *ret;
+        ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL);
+        assert(ret);
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 3a85690..d7859a7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(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_dirty_bitmap_decref(BlockDriverState *bs,
+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
-- 
1.9.3