aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMax Reitz <mreitz@redhat.com>2019-07-19 11:26:15 +0200
committerKevin Wolf <kwolf@redhat.com>2019-07-19 13:19:16 +0200
commit2afdc790ec0e5cb61d2f738adafa93f990ca553d (patch)
tree665696c6a58e7f5f37985b1ccee279b8b8741cc9
parente037c09c78520cbdb6da7cfc6ad0256d5870b814 (diff)
tests: Extend commit by drained_end test
Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
-rw-r--r--tests/test-bdrv-drain.c36
1 files changed, 32 insertions, 4 deletions
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 3503ce3b69..03fa1142a1 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1532,6 +1532,7 @@ typedef struct TestDropBackingBlockJob {
BlockJob common;
bool should_complete;
bool *did_complete;
+ BlockDriverState *detach_also;
} TestDropBackingBlockJob;
static int coroutine_fn test_drop_backing_job_run(Job *job, Error **errp)
@@ -1552,6 +1553,7 @@ static void test_drop_backing_job_commit(Job *job)
container_of(job, TestDropBackingBlockJob, common.job);
bdrv_set_backing_hd(blk_bs(s->common.blk), NULL, &error_abort);
+ bdrv_set_backing_hd(s->detach_also, NULL, &error_abort);
*s->did_complete = true;
}
@@ -1571,9 +1573,6 @@ static const BlockJobDriver test_drop_backing_job_driver = {
* Creates a child node with three parent nodes on it, and then runs a
* block job on the final one, parent-node-2.
*
- * (TODO: parent-node-0 currently serves no purpose, but will as of a
- * follow-up patch.)
- *
* The job is then asked to complete before a section where the child
* is drained.
*
@@ -1585,7 +1584,7 @@ static const BlockJobDriver test_drop_backing_job_driver = {
*
* Ending the drain on parent-node-1 will poll the AioContext, which
* lets job_exit() and thus test_drop_backing_job_commit() run. That
- * function removes the child as parent-node-2's backing file.
+ * function first removes the child as parent-node-2's backing file.
*
* In old (and buggy) implementations, there are two problems with
* that:
@@ -1604,6 +1603,34 @@ static const BlockJobDriver test_drop_backing_job_driver = {
* bdrv_replace_child_noperm() therefore must call drained_end() on
* the parent only if it really is still drained because the child is
* drained.
+ *
+ * If removing child from parent-node-2 was successful (as it should
+ * be), test_drop_backing_job_commit() will then also remove the child
+ * from parent-node-0.
+ *
+ * With an old version of our drain infrastructure ((A) above), that
+ * resulted in the following flow:
+ *
+ * 1. child attempts to leave its drained section. The call recurses
+ * to its parents.
+ *
+ * 2. parent-node-2 leaves the drained section. Polling in
+ * bdrv_drain_invoke() will schedule job_exit().
+ *
+ * 3. parent-node-1 leaves the drained section. Polling in
+ * bdrv_drain_invoke() will run job_exit(), thus disconnecting
+ * parent-node-0 from the child node.
+ *
+ * 4. bdrv_parent_drained_end() uses a QLIST_FOREACH_SAFE() loop to
+ * iterate over the parents. Thus, it now accesses the BdrvChild
+ * object that used to connect parent-node-0 and the child node.
+ * However, that object no longer exists, so it accesses a dangling
+ * pointer.
+ *
+ * The solution is to only poll once when running a bdrv_drained_end()
+ * operation, specifically at the end when all drained_end()
+ * operations for all involved nodes have been scheduled.
+ * Note that this also solves (A) above, thus hiding (B).
*/
static void test_blockjob_commit_by_drained_end(void)
{
@@ -1627,6 +1654,7 @@ static void test_blockjob_commit_by_drained_end(void)
bs_parents[2], 0, BLK_PERM_ALL, 0, 0, NULL, NULL,
&error_abort);
+ job->detach_also = bs_parents[0];
job->did_complete = &job_has_completed;
job_start(&job->common.job);