[Qemu-devel] [PATCH 10/18] block/mirror: Make source the file child

[Max Reitz]

Regarding the source BDS, the mirror BDS is arguably a filter node.
Therefore, the source BDS should be its "file" child.

Signed-off-by: Max Reitz <address@hidden>
---
 block/mirror.c             | 127 ++++++++++++++++++++++++++++++++++-----------
 block/qapi.c               |  25 ++++++---
 tests/qemu-iotests/141.out |   4 +-
 3 files changed, 119 insertions(+), 37 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9df4157511..05410c94ca 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -77,8 +77,16 @@ typedef struct MirrorBlockJob {
     int target_cluster_size;
     int max_iov;
     bool initial_zeroing_ongoing;
+
+    /* Signals that we are no longer accessing source and target and the mirror
+     * BDS should thus relinquish all permissions */
+    bool exiting;
 } MirrorBlockJob;
 
+typedef struct MirrorBDSOpaque {
+    MirrorBlockJob *job;
+} MirrorBDSOpaque;
+
 struct MirrorOp {
     MirrorBlockJob *s;
     QEMUIOVector qiov;
@@ -595,12 +603,15 @@ static void mirror_exit(BlockJob *job, void *opaque)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
     MirrorExitData *data = opaque;
+    MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
     AioContext *replace_aio_context = NULL;
     BlockDriverState *src = s->source->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
     BlockDriverState *mirror_top_bs = s->mirror_top_bs;
     Error *local_err = NULL;
 
+    s->exiting = true;
+
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
     /* Make sure that the source BDS doesn't go away before we called
@@ -622,7 +633,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
+    bdrv_child_try_set_perm(mirror_top_bs->file, 0, BLK_PERM_ALL,
                             &error_abort);
     if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
@@ -673,12 +684,11 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
     /* Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. Also give up permissions on mirror_top_bs->backing, which might
+     * valid. Also give up permissions on mirror_top_bs->file, which might
      * block the removal. */
     block_job_remove_all_bdrv(job);
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
-    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
+    bdrv_child_try_set_perm(mirror_top_bs->file, 0, BLK_PERM_ALL, 
&error_abort);
+    bdrv_replace_node(mirror_top_bs, mirror_top_bs->file->bs, &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
      * bdrv_replace_node() calls), so switch the BB back so the cleanup does
@@ -687,6 +697,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
     blk_set_perm(job->blk, 0, BLK_PERM_ALL, &error_abort);
     blk_insert_bs(job->blk, mirror_top_bs, &error_abort);
 
+    bs_opaque->job = NULL;
     block_job_completed(&s->common, data->ret);
 
     g_free(data);
@@ -1102,7 +1113,7 @@ static void mirror_drain(BlockJob *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
-    /* Need to keep a reference in case blk_drain triggers execution
+    /* Need to keep a reference in case bdrv_drain triggers execution
      * of mirror_complete...
      */
     if (s->target) {
@@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver = {
     .drain                  = mirror_drain,
 };
 
+static void source_child_inherit_fmt_options(int *child_flags,
+                                             QDict *child_options,
+                                             int parent_flags,
+                                             QDict *parent_options)
+{
+    child_backing.inherit_options(child_flags, child_options,
+                                  parent_flags, parent_options);
+}
+
+static char *source_child_get_parent_desc(BdrvChild *c)
+{
+    return child_backing.get_parent_desc(c);
+}
+
+static void source_child_cb_drained_begin(BdrvChild *c)
+{
+    BlockDriverState *bs = c->opaque;
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s && s->job) {
+        block_job_drained_begin(&s->job->common);
+    }
+    bdrv_drained_begin(bs);
+}
+
+static void source_child_cb_drained_end(BdrvChild *c)
+{
+    BlockDriverState *bs = c->opaque;
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s && s->job) {
+        block_job_drained_end(&s->job->common);
+    }
+    bdrv_drained_end(bs);
+}
+
+static BdrvChildRole source_child_role = {
+    .inherit_options    = source_child_inherit_fmt_options,
+    .get_parent_desc    = source_child_get_parent_desc,
+    .drained_begin      = source_child_cb_drained_begin,
+    .drained_end        = source_child_cb_drained_end,
+};
+
 static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
+    return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
 }
 
 static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+    return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
 }
 
 static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
 {
-    return bdrv_co_flush(bs->backing->bs);
+    return bdrv_co_flush(bs->file->bs);
 }
 
 static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
-    return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+    return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
 }
 
 static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
     int64_t offset, int bytes)
 {
-    return bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
+    return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }
 
 static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
 {
-    bdrv_refresh_filename(bs->backing->bs);
     pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
-            bs->backing->bs->filename);
+            bs->file->bs->filename);
 }
 
 static void bdrv_mirror_top_close(BlockDriverState *bs)
 {
+    bdrv_unref_child(bs, bs->file);
+    bs->file = NULL;
 }
 
 static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c,
@@ -1180,6 +1235,14 @@ static void bdrv_mirror_top_child_perm(BlockDriverState 
*bs, BdrvChild *c,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s->job && s->job->exiting) {
+        *nperm = 0;
+        *nshared = BLK_PERM_ALL;
+        return;
+    }
+
     /* Must be able to forward guest writes to the real image */
     *nperm = 0;
     if (perm & BLK_PERM_WRITE) {
@@ -1190,7 +1253,7 @@ static void bdrv_mirror_top_child_perm(BlockDriverState 
*bs, BdrvChild *c,
 }
 
 /* Dummy node that provides consistent read to its users without requiring it
- * from its backing file and that allows writes on the backing file chain. */
+ * from its source file and that allows writes on the source file. */
 static BlockDriver bdrv_mirror_top = {
     .format_name                = "mirror_top",
     .bdrv_co_preadv             = bdrv_mirror_top_preadv,
@@ -1198,7 +1261,7 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
-    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
+    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_file,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_close                 = bdrv_mirror_top_close,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
@@ -1221,6 +1284,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
                              Error **errp)
 {
     MirrorBlockJob *s;
+    MirrorBDSOpaque *bs_opaque;
     BlockDriverState *mirror_top_bs;
     bool target_graph_mod;
     bool target_is_backing;
@@ -1244,9 +1308,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
 
-    /* In the case of active commit, add dummy driver to provide consistent
-     * reads on the top, while disabling it in the intermediate nodes, and make
-     * the backing chain writable. */
+    /* Create mirror BDS */
     mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
                                          BDRV_O_RDWR, errp);
     if (mirror_top_bs == NULL) {
@@ -1256,14 +1318,19 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
         mirror_top_bs->implicit = true;
     }
     mirror_top_bs->total_sectors = bs->total_sectors;
+    bs_opaque = g_new0(MirrorBDSOpaque, 1);
+    mirror_top_bs->opaque = bs_opaque;
     bdrv_set_aio_context(mirror_top_bs, bdrv_get_aio_context(bs));
 
-    /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
-     * it alive until block_job_create() succeeds even if bs has no parent. */
-    bdrv_ref(mirror_top_bs);
-    bdrv_drained_begin(bs);
-    bdrv_append(mirror_top_bs, bs, &local_err);
-    bdrv_drained_end(bs);
+    /* Create reference for bdrv_attach_child() */
+    bdrv_ref(bs);
+    mirror_top_bs->file = bdrv_attach_child(mirror_top_bs, bs, "file",
+                                            &source_child_role, &local_err);
+    if (!local_err) {
+        bdrv_drained_begin(bs);
+        bdrv_replace_node(bs, mirror_top_bs, &local_err);
+        bdrv_drained_end(bs);
+    }
 
     if (local_err) {
         bdrv_unref(mirror_top_bs);
@@ -1280,6 +1347,8 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
     if (!s) {
         goto fail;
     }
+    bs_opaque->job = s;
+
     /* The block job now has a reference to this node */
     bdrv_unref(mirror_top_bs);
 
@@ -1329,7 +1398,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
         s->should_complete = true;
     }
 
-    s->source = mirror_top_bs->backing;
+    s->source = mirror_top_bs->file;
     s->mirror_top_bs = mirror_top_bs;
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
@@ -1373,12 +1442,12 @@ fail:
 
         g_free(s->replaces);
         blk_unref(s->target);
-        block_job_early_fail(&s->common);
+        bs_opaque->job = NULL;
+        block_job_unref(&s->common);
     }
 
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
-    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
+    bdrv_child_try_set_perm(mirror_top_bs->file, 0, BLK_PERM_ALL, 
&error_abort);
+    bdrv_replace_node(mirror_top_bs, mirror_top_bs->file->bs, &error_abort);
 
     bdrv_unref(mirror_top_bs);
 }
diff --git a/block/qapi.c b/block/qapi.c
index 7fa2437923..ee792d0cbc 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
         /* Skip automatically inserted nodes that the user isn't aware of for
          * query-block (blk != NULL), but not for query-named-block-nodes */
-        while (blk && bs0->drv && bs0->implicit) {
-            bs0 = backing_bs(bs0);
-            assert(bs0);
+        while (blk && bs0 && bs0->drv && bs0->implicit) {
+            if (bs0->backing) {
+                bs0 = backing_bs(bs0);
+            } else {
+                assert(bs0->file);
+                bs0 = bs0->file->bs;
+            }
         }
     }
 
@@ -337,7 +341,12 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
 
     /* Skip automatically inserted nodes that the user isn't aware of */
     while (bs && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
+        if (bs->backing) {
+            bs = backing_bs(bs);
+        } else {
+            assert(bs->file);
+            bs = bs->file->bs;
+        }
     }
 
     info->device = g_strdup(blk_name(blk));
@@ -466,8 +475,12 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState 
*bs,
      * a BlockBackend-level command. Stay at the exact node for a node-level
      * command. */
     while (blk_level && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
-        assert(bs);
+        if (bs->backing) {
+            bs = backing_bs(bs);
+        } else {
+            assert(bs->file);
+            bs = bs->file->bs;
+        }
     }
 
     if (bdrv_get_node_name(bs)[0]) {
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 82e763b68d..8c4dd6d531 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -20,7 +20,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.
 Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 
0, "type": "mirror"}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used 
as backing hd of 'NODE_NAME'"}}
+{"error": {"class": "GenericError", "desc": "Block device drv0 is in use"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, 
"speed": 0, "type": "mirror"}}
 {"return": {}}
@@ -30,7 +30,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 
0, "type": "commit"}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used 
as backing hd of 'NODE_NAME'"}}
+{"error": {"class": "GenericError", "desc": "Block device drv0 is in use"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, 
"speed": 0, "type": "commit"}}
 {"return": {}}
-- 
2.13.5