[Qemu-devel] [PATCH v3 0/5] block: Fixes for concurrent block jobs

[Qemu-devel] [PATCH v3 0/5] block: Fixes for concurrent block jobs

[Max Reitz]

I think the patches speak for themselves now.

(The title of this series alludes to what the iotest added in the final
patch tests.)


v3:
- Rebased on master
- Added two tests to test-bdrv-drain [Kevin]
- Removed new iotest from auto [Thomas]


git-backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[----] [--] 'block: Keep subtree drained in drop_intermediate'
002/5:[0004] [FC] 'block: Reduce (un)drains when replacing a child'
003/5:[down] 'tests: Test polling in bdrv_drop_intermediate()'
004/5:[down] 'tests: Test mid-drain bdrv_replace_child_noperm()'
005/5:[0002] [FC] 'iotests: Add test for concurrent stream/commit'


Max Reitz (5):
  block: Keep subtree drained in drop_intermediate
  block: Reduce (un)drains when replacing a child
  tests: Test polling in bdrv_drop_intermediate()
  tests: Test mid-drain bdrv_replace_child_noperm()
  iotests: Add test for concurrent stream/commit

 block.c                    |  51 ++--
 tests/test-bdrv-drain.c    | 475 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/258     | 163 +++++++++++++
 tests/qemu-iotests/258.out |  33 +++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 707 insertions(+), 16 deletions(-)
 create mode 100755 tests/qemu-iotests/258
 create mode 100644 tests/qemu-iotests/258.out

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/5] block: Keep subtree drained in drop_intermediate

[Max Reitz]

bdrv_drop_intermediate() calls BdrvChildRole.update_filename().  That
may poll, thus changing the graph, which potentially breaks the
QLIST_FOREACH_SAFE() loop.

Just keep the whole subtree drained.  This is probably the right thing
to do anyway (dropping nodes while the subtree is not drained seems
wrong).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 9c94f7f28a..818885d467 100644
--- a/block.c
+++ b/block.c
@@ -4499,6 +4499,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
     int ret = -EIO;
 
     bdrv_ref(top);
+    bdrv_subtree_drained_begin(top);
 
     if (!top->drv || !base->drv) {
         goto exit;
@@ -4570,6 +4571,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
 
     ret = 0;
 exit:
+    bdrv_subtree_drained_end(top);
     bdrv_unref(top);
     return ret;
 }
-- 
2.21.0

[Qemu-devel] [PATCH v3 2/5] block: Reduce (un)drains when replacing a child

[Max Reitz]

Currently, bdrv_replace_child_noperm() undrains the parent until it is
completely undrained, then re-drains it after attaching the new child
node.

This is a problem with bdrv_drop_intermediate(): We want to keep the
whole subtree drained, including parents, while the operation is
under way.  bdrv_replace_child_noperm() breaks this by allowing every
parent to become unquiesced briefly, and then redraining it.

In fact, there is no reason why the parent should become unquiesced and
be allowed to submit requests to the new child node if that new node is
supposed to be kept drained.  So if anything, we have to drain the
parent before detaching the old child node.  Conversely, we have to
undrain it only after attaching the new child node.

Thus, change the whole drain algorithm here: Calculate the number of
times we have to drain/undrain the parent before replacing the child
node then drain it (if necessary), replace the child node, and then
undrain it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 49 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 818885d467..153e2a8805 100644
--- a/block.c
+++ b/block.c
@@ -2238,13 +2238,27 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs)
 {
     BlockDriverState *old_bs = child->bs;
-    int i;
+    int new_bs_quiesce_counter;
+    int drain_saldo;
 
     assert(!child->frozen);
 
     if (old_bs && new_bs) {
         assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
     }
+
+    new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0);
+    drain_saldo = new_bs_quiesce_counter - child->parent_quiesce_counter;
+
+    /*
+     * If the new child node is drained but the old one was not, flush
+     * all outstanding requests to the old child node.
+     */
+    while (drain_saldo > 0 && child->role->drained_begin) {
+        bdrv_parent_drained_begin_single(child, true);
+        drain_saldo--;
+    }
+
     if (old_bs) {
         /* Detach first so that the recursive drain sections coming from @child
          * are already gone and we only end the drain sections that came from
@@ -2252,28 +2266,22 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
         if (child->role->detach) {
             child->role->detach(child);
         }
-        while (child->parent_quiesce_counter) {
-            bdrv_parent_drained_end_single(child);
-        }
         QLIST_REMOVE(child, next_parent);
-    } else {
-        assert(child->parent_quiesce_counter == 0);
     }
 
     child->bs = new_bs;
 
     if (new_bs) {
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
-        if (new_bs->quiesce_counter) {
-            int num = new_bs->quiesce_counter;
-            if (child->role->parent_is_bds) {
-                num -= bdrv_drain_all_count;
-            }
-            assert(num >= 0);
-            for (i = 0; i < num; i++) {
-                bdrv_parent_drained_begin_single(child, true);
-            }
-        }
+
+        /*
+         * Detaching the old node may have led to the new node's
+         * quiesce_counter having been decreased.  Not a problem, we
+         * just need to recognize this here and then invoke
+         * drained_end appropriately more often.
+         */
+        assert(new_bs->quiesce_counter <= new_bs_quiesce_counter);
+        drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter;
 
         /* Attach only after starting new drained sections, so that recursive
          * drain sections coming from @child don't get an extra .drained_begin
@@ -2282,6 +2290,15 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
             child->role->attach(child);
         }
     }
+
+    /*
+     * If the old child node was drained but the new one is not, allow
+     * requests to come in only after the new node has been attached.
+     */
+    while (drain_saldo < 0 && child->role->drained_end) {
+        bdrv_parent_drained_end_single(child);
+        drain_saldo++;
+    }
 }
 
 /*
-- 
2.21.0

[Qemu-devel] [PATCH v3 3/5] tests: Test polling in bdrv_drop_intermediate()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-bdrv-drain.c | 167 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 03fa1142a1..1600d41e9a 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -100,6 +100,13 @@ static void bdrv_test_child_perm(BlockDriverState *bs, BdrvChild *c,
                               nperm, nshared);
 }
 
+static int bdrv_test_change_backing_file(BlockDriverState *bs,
+                                         const char *backing_file,
+                                         const char *backing_fmt)
+{
+    return 0;
+}
+
 static BlockDriver bdrv_test = {
     .format_name            = "test",
     .instance_size          = sizeof(BDRVTestState),
@@ -111,6 +118,8 @@ static BlockDriver bdrv_test = {
     .bdrv_co_drain_end      = bdrv_test_co_drain_end,
 
     .bdrv_child_perm        = bdrv_test_child_perm,
+
+    .bdrv_change_backing_file = bdrv_test_change_backing_file,
 };
 
 static void aio_ret_cb(void *opaque, int ret)
@@ -1671,6 +1680,161 @@ static void test_blockjob_commit_by_drained_end(void)
     bdrv_unref(bs_child);
 }
 
+
+typedef struct TestSimpleBlockJob {
+    BlockJob common;
+    bool should_complete;
+    bool *did_complete;
+} TestSimpleBlockJob;
+
+static int coroutine_fn test_simple_job_run(Job *job, Error **errp)
+{
+    TestSimpleBlockJob *s = container_of(job, TestSimpleBlockJob, common.job);
+
+    while (!s->should_complete) {
+        job_sleep_ns(job, 0);
+    }
+
+    return 0;
+}
+
+static void test_simple_job_clean(Job *job)
+{
+    TestSimpleBlockJob *s = container_of(job, TestSimpleBlockJob, common.job);
+    *s->did_complete = true;
+}
+
+static const BlockJobDriver test_simple_job_driver = {
+    .job_driver = {
+        .instance_size  = sizeof(TestSimpleBlockJob),
+        .free           = block_job_free,
+        .user_resume    = block_job_user_resume,
+        .drain          = block_job_drain,
+        .run            = test_simple_job_run,
+        .clean          = test_simple_job_clean,
+    },
+};
+
+static int drop_intermediate_poll_update_filename(BdrvChild *child,
+                                                  BlockDriverState *new_base,
+                                                  const char *filename,
+                                                  Error **errp)
+{
+    /*
+     * We are free to poll here, which may change the block graph, if
+     * it is not drained.
+     */
+
+    /* If the job is not drained: Complete it, schedule job_exit() */
+    aio_poll(qemu_get_current_aio_context(), false);
+    /* If the job is not drained: Run job_exit(), finish the job */
+    aio_poll(qemu_get_current_aio_context(), false);
+
+    return 0;
+}
+
+/**
+ * Test a poll in the midst of bdrv_drop_intermediate().
+ *
+ * bdrv_drop_intermediate() calls BdrvChildRole.update_filename(),
+ * which can yield or poll.  This may lead to graph changes, unless
+ * the whole subtree in question is drained.
+ *
+ * We test this on the following graph:
+ *
+ *                    Job
+ *
+ *                     |
+ *                  job-node
+ *                     |
+ *                     v
+ *
+ *                  job-node
+ *
+ *                     |
+ *                  backing
+ *                     |
+ *                     v
+ *
+ * node-2 --chain--> node-1 --chain--> node-0
+ *
+ * We drop node-1 with bdrv_drop_intermediate(top=node-1, base=node-0).
+ *
+ * This first updates node-2's backing filename by invoking
+ * drop_intermediate_poll_update_filename(), which polls twice.  This
+ * causes the job to finish, which in turns causes the job-node to be
+ * deleted.
+ *
+ * bdrv_drop_intermediate() uses a QLIST_FOREACH_SAFE() loop, so it
+ * already has a pointer to the BdrvChild edge between job-node and
+ * node-1.  When it tries to handle that edge, we probably get a
+ * segmentation fault because the object no longer exists.
+ *
+ *
+ * The solution is for bdrv_drop_intermediate() to drain top's
+ * subtree.  This prevents graph changes from happening just because
+ * BdrvChildRole.update_filename() yields or polls.  Thus, the block
+ * job is paused during that drained section and must finish before or
+ * after.
+ *
+ * (In addition, bdrv_replace_child() must keep the job paused.)
+ */
+static void test_drop_intermediate_poll(void)
+{
+    static BdrvChildRole chain_child_role;
+    BlockDriverState *chain[3];
+    TestSimpleBlockJob *job;
+    BlockDriverState *job_node;
+    bool job_has_completed = false;
+    int i;
+    int ret;
+
+    chain_child_role = child_backing;
+    chain_child_role.update_filename = drop_intermediate_poll_update_filename;
+
+    for (i = 0; i < 3; i++) {
+        char name[32];
+        snprintf(name, 32, "node-%i", i);
+
+        chain[i] = bdrv_new_open_driver(&bdrv_test, name, 0, &error_abort);
+    }
+
+    job_node = bdrv_new_open_driver(&bdrv_test, "job-node", BDRV_O_RDWR,
+                                    &error_abort);
+    bdrv_set_backing_hd(job_node, chain[1], &error_abort);
+
+    /*
+     * Establish the chain last, so the chain links are the first
+     * elements in the BDS.parents lists
+     */
+    for (i = 0; i < 3; i++) {
+        if (i) {
+            /* Takes the reference to chain[i - 1] */
+            chain[i]->backing = bdrv_attach_child(chain[i], chain[i - 1],
+                                                  "chain", &chain_child_role,
+                                                  &error_abort);
+        }
+    }
+
+    job = block_job_create("job", &test_simple_job_driver, NULL, job_node,
+                           0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort);
+
+    /* The job has a reference now */
+    bdrv_unref(job_node);
+
+    job->did_complete = &job_has_completed;
+
+    job_start(&job->common.job);
+    job->should_complete = true;
+
+    g_assert(!job_has_completed);
+    ret = bdrv_drop_intermediate(chain[1], chain[0], NULL);
+    g_assert(ret == 0);
+    g_assert(job_has_completed);
+
+    bdrv_unref(chain[2]);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -1757,6 +1921,9 @@ int main(int argc, char **argv)
     g_test_add_func("/bdrv-drain/blockjob/commit_by_drained_end",
                     test_blockjob_commit_by_drained_end);
 
+    g_test_add_func("/bdrv-drain/bdrv_drop_intermediate/poll",
+                    test_drop_intermediate_poll);
+
     ret = g_test_run();
     qemu_event_destroy(&done_event);
     return ret;
-- 
2.21.0

[Qemu-devel] [PATCH v3 4/5] tests: Test mid-drain bdrv_replace_child_noperm()

[Max Reitz]

Add a test for what happens when you call bdrv_replace_child_noperm()
for various drain situations ({old,new} child {drained,not drained}).

Most importantly, if both the old and the new child are drained, the
parent must not be undrained at any point.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-bdrv-drain.c | 308 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 308 insertions(+)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 1600d41e9a..9dffd86c47 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1835,6 +1835,311 @@ static void test_drop_intermediate_poll(void)
     bdrv_unref(chain[2]);
 }
 
+
+typedef struct BDRVReplaceTestState {
+    bool was_drained;
+    bool was_undrained;
+    bool has_read;
+
+    int drain_count;
+
+    bool yield_before_read;
+    Coroutine *io_co;
+    Coroutine *drain_co;
+} BDRVReplaceTestState;
+
+static void bdrv_replace_test_close(BlockDriverState *bs)
+{
+}
+
+/**
+ * If @bs has a backing file:
+ *   Yield if .yield_before_read is true (and wait for drain_begin to
+ *   wake us up).
+ *   Forward the read to bs->backing.  Set .has_read to true.
+ *   If drain_begin has woken us, wake it in turn.
+ *
+ * Otherwise:
+ *   Set .has_read to true and return success.
+ */
+static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,
+                                                    uint64_t offset,
+                                                    uint64_t bytes,
+                                                    QEMUIOVector *qiov,
+                                                    int flags)
+{
+    BDRVReplaceTestState *s = bs->opaque;
+
+    if (bs->backing) {
+        int ret;
+
+        g_assert(!s->drain_count);
+
+        s->io_co = qemu_coroutine_self();
+        if (s->yield_before_read) {
+            s->yield_before_read = false;
+            qemu_coroutine_yield();
+        }
+        s->io_co = NULL;
+
+        ret = bdrv_preadv(bs->backing, offset, qiov);
+        s->has_read = true;
+
+        /* Wake up drain_co if it runs */
+        if (s->drain_co) {
+            aio_co_wake(s->drain_co);
+        }
+
+        return ret;
+    }
+
+    s->has_read = true;
+    return 0;
+}
+
+/**
+ * If .drain_count is 0, wake up .io_co if there is one; and set
+ * .was_drained.
+ * Increment .drain_count.
+ */
+static void coroutine_fn bdrv_replace_test_co_drain_begin(BlockDriverState *bs)
+{
+    BDRVReplaceTestState *s = bs->opaque;
+
+    if (!s->drain_count) {
+        /* Keep waking io_co up until it is done */
+        s->drain_co = qemu_coroutine_self();
+        while (s->io_co) {
+            aio_co_wake(s->io_co);
+            s->io_co = NULL;
+            qemu_coroutine_yield();
+        }
+        s->drain_co = NULL;
+
+        s->was_drained = true;
+    }
+    s->drain_count++;
+}
+
+/**
+ * Reduce .drain_count, set .was_undrained once it reaches 0.
+ * If .drain_count reaches 0 and the node has a backing file, issue a
+ * read request.
+ */
+static void coroutine_fn bdrv_replace_test_co_drain_end(BlockDriverState *bs)
+{
+    BDRVReplaceTestState *s = bs->opaque;
+
+    g_assert(s->drain_count > 0);
+    if (!--s->drain_count) {
+        int ret;
+
+        s->was_undrained = true;
+
+        if (bs->backing) {
+            char data;
+            QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1);
+
+            /* Queue a read request post-drain */
+            ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0);
+            g_assert(ret >= 0);
+        }
+    }
+}
+
+static BlockDriver bdrv_replace_test = {
+    .format_name            = "replace_test",
+    .instance_size          = sizeof(BDRVReplaceTestState),
+
+    .bdrv_close             = bdrv_replace_test_close,
+    .bdrv_co_preadv         = bdrv_replace_test_co_preadv,
+
+    .bdrv_co_drain_begin    = bdrv_replace_test_co_drain_begin,
+    .bdrv_co_drain_end      = bdrv_replace_test_co_drain_end,
+
+    .bdrv_child_perm        = bdrv_format_default_perms,
+};
+
+static void coroutine_fn test_replace_child_mid_drain_read_co(void *opaque)
+{
+    int ret;
+    char data;
+
+    ret = blk_co_pread(opaque, 0, 1, &data, 0);
+    g_assert(ret >= 0);
+}
+
+/**
+ * We test two things:
+ * (1) bdrv_replace_child_noperm() must not undrain the parent if both
+ *     children are drained.
+ * (2) bdrv_replace_child_noperm() must never flush I/O requests to a
+ *     drained child.  If the old child is drained, it must flush I/O
+ *     requests after the new one has been attached.  If the new child
+ *     is drained, it must flush I/O requests before the old one is
+ *     detached.
+ *
+ * To do so, we create one parent node and two child nodes; then
+ * attach one of the children (old_child_bs) to the parent, then
+ * drain both old_child_bs and new_child_bs according to
+ * old_drain_count and new_drain_count, respectively, and finally
+ * we invoke bdrv_replace_node() to replace old_child_bs by
+ * new_child_bs.
+ *
+ * The test block driver we use here (bdrv_replace_test) has a read
+ * function that:
+ * - For the parent node, can optionally yield, and then forwards the
+ *   read to bdrv_preadv(),
+ * - For the child node, just returns immediately.
+ *
+ * If the read yields, the drain_begin function will wake it up.
+ *
+ * The drain_end function issues a read on the parent once it is fully
+ * undrained (which simulates requests starting to come in again).
+ */
+static void do_test_replace_child_mid_drain(int old_drain_count,
+                                            int new_drain_count)
+{
+    BlockBackend *parent_blk;
+    BlockDriverState *parent_bs;
+    BlockDriverState *old_child_bs, *new_child_bs;
+    BDRVReplaceTestState *parent_s;
+    BDRVReplaceTestState *old_child_s, *new_child_s;
+    Coroutine *io_co;
+    int i;
+
+    parent_bs = bdrv_new_open_driver(&bdrv_replace_test, "parent", 0,
+                                     &error_abort);
+    parent_s = parent_bs->opaque;
+
+    parent_blk = blk_new(qemu_get_aio_context(),
+                         BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
+    blk_insert_bs(parent_blk, parent_bs, &error_abort);
+
+    old_child_bs = bdrv_new_open_driver(&bdrv_replace_test, "old-child", 0,
+                                        &error_abort);
+    new_child_bs = bdrv_new_open_driver(&bdrv_replace_test, "new-child", 0,
+                                        &error_abort);
+    old_child_s = old_child_bs->opaque;
+    new_child_s = new_child_bs->opaque;
+
+    /* So that we can read something */
+    parent_bs->total_sectors = 1;
+    old_child_bs->total_sectors = 1;
+    new_child_bs->total_sectors = 1;
+
+    bdrv_ref(old_child_bs);
+    parent_bs->backing = bdrv_attach_child(parent_bs, old_child_bs, "child",
+                                           &child_backing, &error_abort);
+
+    for (i = 0; i < old_drain_count; i++) {
+        bdrv_drained_begin(old_child_bs);
+    }
+    for (i = 0; i < new_drain_count; i++) {
+        bdrv_drained_begin(new_child_bs);
+    }
+
+    if (!old_drain_count) {
+        /*
+         * Start a read operation that will yield, so it will not
+         * complete before the node is drained.
+         */
+        parent_s->yield_before_read = true;
+        io_co = qemu_coroutine_create(test_replace_child_mid_drain_read_co,
+                                      parent_blk);
+        qemu_coroutine_enter(io_co);
+    }
+
+    /* If we have started a read operation, it should have yielded */
+    g_assert(!parent_s->has_read);
+
+    /* Reset drained status so we can see what bdrv_replace_node() does */
+    parent_s->was_drained = false;
+    parent_s->was_undrained = false;
+
+    g_assert(parent_bs->quiesce_counter == old_drain_count);
+    bdrv_replace_node(old_child_bs, new_child_bs, &error_abort);
+    g_assert(parent_bs->quiesce_counter == new_drain_count);
+
+    if (!old_drain_count && !new_drain_count) {
+        /*
+         * From undrained to undrained drains and undrains the parent,
+         * because bdrv_replace_node() contains a drained section for
+         * @old_child_bs.
+         */
+        g_assert(parent_s->was_drained && parent_s->was_undrained);
+    } else if (!old_drain_count && new_drain_count) {
+        /*
+         * From undrained to drained should drain the parent and keep
+         * it that way.
+         */
+        g_assert(parent_s->was_drained && !parent_s->was_undrained);
+    } else if (old_drain_count && !new_drain_count) {
+        /*
+         * From drained to undrained should undrain the parent and
+         * keep it that way.
+         */
+        g_assert(!parent_s->was_drained && parent_s->was_undrained);
+    } else /* if (old_drain_count && new_drain_count) */ {
+        /*
+         * From drained to drained must not undrain the parent at any
+         * point
+         */
+        g_assert(!parent_s->was_drained && !parent_s->was_undrained);
+    }
+
+    if (!old_drain_count || !new_drain_count) {
+        /*
+         * If !old_drain_count, we have started a read request before
+         * bdrv_replace_node().  If !new_drain_count, the parent must
+         * have been undrained at some point, and
+         * bdrv_replace_test_co_drain_end() starts a read request
+         * then.
+         */
+        g_assert(parent_s->has_read);
+    } else {
+        /*
+         * If the parent was never undrained, there is no way to start
+         * a read request.
+         */
+        g_assert(!parent_s->has_read);
+    }
+
+    /* A drained child must have not received any request */
+    g_assert(!(old_drain_count && old_child_s->has_read));
+    g_assert(!(new_drain_count && new_child_s->has_read));
+
+    for (i = 0; i < new_drain_count; i++) {
+        bdrv_drained_end(new_child_bs);
+    }
+    for (i = 0; i < old_drain_count; i++) {
+        bdrv_drained_end(old_child_bs);
+    }
+
+    /*
+     * By now, bdrv_replace_test_co_drain_end() must have been called
+     * at some point while the new child was attached to the parent.
+     */
+    g_assert(parent_s->has_read);
+    g_assert(new_child_s->has_read);
+
+    blk_unref(parent_blk);
+    bdrv_unref(parent_bs);
+    bdrv_unref(old_child_bs);
+    bdrv_unref(new_child_bs);
+}
+
+static void test_replace_child_mid_drain(void)
+{
+    int old_drain_count, new_drain_count;
+
+    for (old_drain_count = 0; old_drain_count < 2; old_drain_count++) {
+        for (new_drain_count = 0; new_drain_count < 2; new_drain_count++) {
+            do_test_replace_child_mid_drain(old_drain_count, new_drain_count);
+        }
+    }
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -1924,6 +2229,9 @@ int main(int argc, char **argv)
     g_test_add_func("/bdrv-drain/bdrv_drop_intermediate/poll",
                     test_drop_intermediate_poll);
 
+    g_test_add_func("/bdrv-drain/replace_child/mid-drain",
+                    test_replace_child_mid_drain);
+
     ret = g_test_run();
     qemu_event_destroy(&done_event);
     return ret;
-- 
2.21.0

[Qemu-devel] [PATCH v3 5/5] iotests: Add test for concurrent stream/commit

[Max Reitz]

We already have 030 for that in general, but this tests very specific
cases of both jobs finishing concurrently.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/258     | 163 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/258.out |  33 ++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 197 insertions(+)
 create mode 100755 tests/qemu-iotests/258
 create mode 100644 tests/qemu-iotests/258.out

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
new file mode 100755
index 0000000000..b84cf02254
--- /dev/null
+++ b/tests/qemu-iotests/258
@@ -0,0 +1,163 @@
+#!/usr/bin/env python
+#
+# Very specific tests for adjacent commit/stream block jobs
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Max Reitz <mreitz@redhat.com>
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent, \
+        filter_qmp_testfiles, filter_qmp_imgfmt
+
+# Need backing file and change-backing-file support
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+# Returns a node for blockdev-add
+def node(node_name, path, backing=None, fmt=None, throttle=None):
+    if fmt is None:
+        fmt = iotests.imgfmt
+
+    res = {
+        'node-name': node_name,
+        'driver': fmt,
+        'file': {
+            'driver': 'file',
+            'filename': path
+        }
+    }
+
+    if backing is not None:
+        res['backing'] = backing
+
+    if throttle:
+        res['file'] = {
+            'driver': 'throttle',
+            'throttle-group': throttle,
+            'file': res['file']
+        }
+
+    return res
+
+# Finds a node in the debug block graph
+def find_graph_node(graph, node_id):
+    return next(node for node in graph['nodes'] if node['id'] == node_id)
+
+
+def test_concurrent_finish(write_to_stream_node):
+    log('')
+    log('=== Commit and stream finish concurrently (letting %s write) ===' % \
+        ('stream' if write_to_stream_node else 'commit'))
+    log('')
+
+    # All chosen in such a way that when the commit job wants to
+    # finish, it polls and thus makes stream finish concurrently --
+    # and the other way around, depending on whether the commit job
+    # is finalized before stream completes or not.
+
+    with iotests.FilePath('node4.img') as node4_path, \
+         iotests.FilePath('node3.img') as node3_path, \
+         iotests.FilePath('node2.img') as node2_path, \
+         iotests.FilePath('node1.img') as node1_path, \
+         iotests.FilePath('node0.img') as node0_path, \
+         iotests.VM() as vm:
+
+        # It is important to use raw for the base layer (so that
+        # permissions are just handed through to the protocol layer)
+        assert qemu_img('create', '-f', 'raw', node0_path, '64M') == 0
+
+        stream_throttle=None
+        commit_throttle=None
+
+        for path in [node1_path, node2_path, node3_path, node4_path]:
+            assert qemu_img('create', '-f', iotests.imgfmt, path, '64M') == 0
+
+        if write_to_stream_node:
+            # This is what (most of the time) makes commit finish
+            # earlier and then pull in stream
+            assert qemu_io_silent(node2_path,
+                                  '-c', 'write %iK 64K' % (65536 - 192),
+                                  '-c', 'write %iK 64K' % (65536 -  64)) == 0
+
+            stream_throttle='tg'
+        else:
+            # And this makes stream finish earlier
+            assert qemu_io_silent(node1_path,
+                                  '-c', 'write %iK 64K' % (65536 - 64)) == 0
+
+            commit_throttle='tg'
+
+        vm.launch()
+
+        vm.qmp_log('object-add',
+                   qom_type='throttle-group',
+                   id='tg',
+                   props={
+                       'x-iops-write': 1,
+                       'x-iops-write-max': 1
+                   })
+
+        vm.qmp_log('blockdev-add',
+                   filters=[filter_qmp_testfiles, filter_qmp_imgfmt],
+                   **node('node4', node4_path, throttle=stream_throttle,
+                     backing=node('node3', node3_path,
+                     backing=node('node2', node2_path,
+                     backing=node('node1', node1_path,
+                     backing=node('node0', node0_path, throttle=commit_throttle,
+                                  fmt='raw'))))))
+
+        vm.qmp_log('block-commit',
+                   job_id='commit',
+                   device='node4',
+                   filter_node_name='commit-filter',
+                   top_node='node1',
+                   base_node='node0',
+                   auto_finalize=False)
+
+        vm.qmp_log('block-stream',
+                   job_id='stream',
+                   device='node3',
+                   base_node='commit-filter')
+
+        if write_to_stream_node:
+            vm.run_job('commit', auto_finalize=False, auto_dismiss=True)
+            vm.run_job('stream', auto_finalize=True, auto_dismiss=True)
+        else:
+            # No, the jobs do not really finish concurrently here,
+            # the stream job does complete strictly before commit.
+            # But still, this is close enough for what we want to
+            # test.
+            vm.run_job('stream', auto_finalize=True, auto_dismiss=True)
+            vm.run_job('commit', auto_finalize=False, auto_dismiss=True)
+
+        # Assert that the backing node of node3 is node 0 now
+        graph = vm.qmp('x-debug-query-block-graph')['return']
+        for edge in graph['edges']:
+            if edge['name'] == 'backing' and \
+               find_graph_node(graph, edge['parent'])['name'] == 'node3':
+                assert find_graph_node(graph, edge['child'])['name'] == 'node0'
+                break
+
+
+def main():
+    log('Running tests:')
+    test_concurrent_finish(True)
+    test_concurrent_finish(False)
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/qemu-iotests/258.out b/tests/qemu-iotests/258.out
new file mode 100644
index 0000000000..ce6e9ba3e5
--- /dev/null
+++ b/tests/qemu-iotests/258.out
@@ -0,0 +1,33 @@
+Running tests:
+
+=== Commit and stream finish concurrently (letting stream write) ===
+
+{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}}
+{"return": {}}
+{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}}
+{"return": {}}
+{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "commit"}}
+{"return": {}}
+{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+=== Commit and stream finish concurrently (letting commit write) ===
+
+{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}}
+{"return": {}}
+{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}}
+{"return": {}}
+{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}}
+{"return": {}}
+{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "commit"}}
+{"return": {}}
+{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b34c8e3c0c..a854ecdc80 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -269,3 +269,4 @@
 254 rw auto backing quick
 255 rw auto quick
 256 rw auto quick
+258 rw quick
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 0/5] block: Fixes for concurrent block jobs

[Kevin Wolf]

Am 22.07.2019 um 15:33 hat Max Reitz geschrieben:
> I think the patches speak for themselves now.
> 
> (The title of this series alludes to what the iotest added in the final
> patch tests.)
> 
> v3:
> - Rebased on master
> - Added two tests to test-bdrv-drain [Kevin]
> - Removed new iotest from auto [Thomas]

Thanks, applied to block-next.

Kevin