[Qemu-devel] [PATCH 08/16] block: Manage backing file references in bdrv_set_backing_hd()

[Kevin Wolf]

This simplifies the code somewhat, especially when dropping whole
backing file subchains.

The exception is the mirroring code that does adventurous things with
bdrv_swap() and in order to keep it working, I had to duplicate most of
bdrv_set_backing_hd() locally. We'll get rid again of this ugliness
shortly.

Signed-off-by: Kevin Wolf <address@hidden>
---
 block.c               | 35 ++++++++++++++---------------------
 block/mirror.c        | 17 ++++++++++++-----
 block/stream.c        | 21 ---------------------
 block/vvfat.c         |  6 +++++-
 include/block/block.h |  1 +
 5 files changed, 32 insertions(+), 48 deletions(-)

diff --git a/block.c b/block.c
index fb94567..743f82e 100644
--- a/block.c
+++ b/block.c
@@ -1094,7 +1094,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState 
*parent_bs,
     return child;
 }
 
-static void bdrv_detach_child(BdrvChild *child)
+void bdrv_detach_child(BdrvChild *child)
 {
     QLIST_REMOVE(child, next);
     g_free(child);
@@ -1112,13 +1112,20 @@ void bdrv_unref_child(BlockDriverState *parent, 
BdrvChild *child)
     bdrv_unref(child_bs);
 }
 
+/*
+ * Sets the backing file link of a BDS. A new reference is created; callers
+ * which don't need their own reference any more must call bdrv_unref().
+ */
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
+    if (backing_hd) {
+        bdrv_ref(backing_hd);
+    }
 
     if (bs->backing) {
         assert(bs->backing_blocker);
         bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker);
-        bdrv_detach_child(bs->backing);
+        bdrv_unref_child(bs, bs->backing);
     } else if (backing_hd) {
         error_setg(&bs->backing_blocker,
                    "node is used as backing hd of '%s'",
@@ -1214,7 +1221,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
         goto free_exit;
     }
 
+    /* Hook up the backing file link; drop our reference, bs owns the
+     * backing_hd reference now */
     bdrv_set_backing_hd(bs, backing_hd);
+    bdrv_unref(backing_hd);
 
 free_exit:
     g_free(backing_filename);
@@ -1884,11 +1894,7 @@ void bdrv_close(BlockDriverState *bs)
 
         bs->drv->bdrv_close(bs);
 
-        if (bs->backing) {
-            BlockDriverState *backing_hd = bs->backing->bs;
-            bdrv_set_backing_hd(bs, NULL);
-            bdrv_unref(backing_hd);
-        }
+        bdrv_set_backing_hd(bs, NULL);
 
         if (bs->file != NULL) {
             bdrv_unref(bs->file->bs);
@@ -2427,12 +2433,9 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
BlockDriverState *top,
     BlockDriverState *intermediate;
     BlockDriverState *base_bs = NULL;
     BlockDriverState *new_top_bs = NULL;
-    BlkIntermediateStates *intermediate_state, *next;
+    BlkIntermediateStates *intermediate_state;
     int ret = -EIO;
 
-    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
-    QSIMPLEQ_INIT(&states_to_delete);
-
     if (!top->drv || !base->drv) {
         goto exit;
     }
@@ -2459,7 +2462,6 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
BlockDriverState *top,
     while (intermediate) {
         intermediate_state = g_new0(BlkIntermediateStates, 1);
         intermediate_state->bs = intermediate;
-        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
 
         if (backing_bs(intermediate) == base) {
             base_bs = backing_bs(intermediate);
@@ -2482,17 +2484,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
BlockDriverState *top,
     }
     bdrv_set_backing_hd(new_top_bs, base_bs);
 
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        /* so that bdrv_close() does not recursively close the chain */
-        bdrv_set_backing_hd(intermediate_state->bs, NULL);
-        bdrv_unref(intermediate_state->bs);
-    }
     ret = 0;
-
 exit:
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        g_free(intermediate_state);
-    }
     return ret;
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 259e11a..571da27 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -370,11 +370,18 @@ static void mirror_exit(BlockJob *job, void *opaque)
         bdrv_swap(s->target, to_replace);
         if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
             /* drop the bs loop chain formed by the swap: break the loop then
-             * trigger the unref from the top one */
-            BlockDriverState *p = s->base->backing
-                                ? s->base->backing->bs : NULL;
-            bdrv_set_backing_hd(s->base, NULL);
-            bdrv_unref(p);
+             * trigger the unref */
+            /* FIXME This duplicates bdrv_set_backing_hd(), except for the
+             * actual detach/unref so that the loop can be broken. When
+             * bdrv_swap() gets replaced, this will become sane again. */
+            BlockDriverState *backing = s->base->backing->bs;
+            assert(s->base->backing_blocker);
+            bdrv_op_unblock_all(s->base->backing->bs, 
s->base->backing_blocker);
+            error_free(s->base->backing_blocker);
+            s->base->backing_blocker = NULL;
+            bdrv_detach_child(s->base->backing);
+            s->base->backing = NULL;
+            bdrv_unref(backing);
         }
     }
     if (s->to_replace) {
diff --git a/block/stream.c b/block/stream.c
index ece7b58..c20aedb 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -55,28 +55,7 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
 static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
                                 const char *base_id)
 {
-    BlockDriverState *intermediate;
-    intermediate = top->backing ? top->backing->bs : NULL;
-
-    /* Must assign before bdrv_delete() to prevent traversing dangling pointer
-     * while we delete backing image instances.
-     */
     bdrv_set_backing_hd(top, base);
-
-    while (intermediate) {
-        BlockDriverState *unused;
-
-        /* reached base */
-        if (intermediate == base) {
-            break;
-        }
-
-        unused = intermediate;
-        intermediate = intermediate->backing ? intermediate->backing->bs : 
NULL;
-        bdrv_set_backing_hd(unused, NULL);
-        bdrv_unref(unused);
-    }
-
     bdrv_refresh_limits(top, NULL);
 }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index 7c4b0f5..b41055a 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2923,6 +2923,7 @@ static BlockDriver vvfat_write_target = {
 static int enable_write_target(BDRVVVFATState *s, Error **errp)
 {
     BlockDriver *bdrv_qcow = NULL;
+    BlockDriverState *backing;
     QemuOpts *opts = NULL;
     int ret;
     int size = sector2cluster(s, s->sector_count);
@@ -2971,7 +2972,10 @@ static int enable_write_target(BDRVVVFATState *s, Error 
**errp)
     unlink(s->qcow_filename);
 #endif
 
-    bdrv_set_backing_hd(s->bs, bdrv_new());
+    backing = bdrv_new();
+    bdrv_set_backing_hd(s->bs, backing);
+    bdrv_unref(backing);
+
     s->bs->backing->bs->drv = &vvfat_write_target;
     s->bs->backing->bs->opaque = g_new(void *, 1);
     *(void**)s->bs->backing->bs->opaque = s;
diff --git a/include/block/block.h b/include/block/block.h
index 130477c..6dc6766 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -510,6 +510,7 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+void bdrv_detach_child(BdrvChild *child);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
-- 
1.8.3.1