@@ -1296,7 +1296,6 @@ struct detach_by_parent_data {
BdrvChild *child_b;
BlockDriverState *c;
BdrvChild *child_c;
- bool by_parent_cb;
};
static struct detach_by_parent_data detach_by_parent_data;
@@ -1314,12 +1313,7 @@ static void detach_indirect_bh(void *opaque)
static void detach_by_parent_aio_cb(void *opaque, int ret)
{
- struct detach_by_parent_data *data = &detach_by_parent_data;
-
g_assert_cmpint(ret, ==, 0);
- if (data->by_parent_cb) {
- detach_indirect_bh(data);
- }
}
static BdrvChildClass detach_by_driver_cb_class;
@@ -1341,31 +1335,24 @@ static void detach_by_driver_cb_drained_begin(BdrvChild *child)
* \ / \
* A B C
*
- * by_parent_cb == true: Test that parent callbacks don't poll
- *
- * PA has a pending write request whose callback changes the child nodes of
- * PB: It removes B and adds C instead. The subtree of PB is drained, which
- * will indirectly drain the write request, too.
- *
- * by_parent_cb == false: Test that bdrv_drain_invoke() doesn't poll
+ * Test that bdrv_drain_invoke() doesn't poll
*
* PA's BdrvChildClass has a .drained_begin callback that schedules a BH
* that does the same graph change. If bdrv_drain_invoke() calls it, the
* state is messed up, but if it is only polled in the single
* BDRV_POLL_WHILE() at the end of the drain, this should work fine.
*/
-static void test_detach_indirect(bool by_parent_cb)
+static void test_detach_indirect(void)
{
BlockBackend *blk;
BlockDriverState *parent_a, *parent_b, *a, *b, *c;
BdrvChild *child_a, *child_b;
BlockAIOCB *acb;
+ BDRVTestState *s;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
- if (!by_parent_cb) {
- detach_by_driver_cb_class = child_of_bds;
- }
+ detach_by_driver_cb_class = child_of_bds;
/* Create all involved nodes */
parent_a = bdrv_new_open_driver(&bdrv_test, "parent-a", BDRV_O_RDWR,
@@ -1384,10 +1371,8 @@ static void test_detach_indirect(bool by_parent_cb)
/* If we want to get bdrv_drain_invoke() to call aio_poll(), the driver
* callback must not return immediately. */
- if (!by_parent_cb) {
- BDRVTestState *s = parent_a->opaque;
- s->sleep_in_drain_begin = true;
- }
+ s = parent_a->opaque;
+ s->sleep_in_drain_begin = true;
/* Set child relationships */
bdrv_ref(b);
@@ -1399,7 +1384,7 @@ static void test_detach_indirect(bool by_parent_cb)
bdrv_ref(a);
bdrv_attach_child(parent_a, a, "PA-A",
- by_parent_cb ? &child_of_bds : &detach_by_driver_cb_class,
+ &detach_by_driver_cb_class,
BDRV_CHILD_DATA, &error_abort);
g_assert_cmpint(parent_a->refcnt, ==, 1);
@@ -1417,16 +1402,13 @@ static void test_detach_indirect(bool by_parent_cb)
.parent_b = parent_b,
.child_b = child_b,
.c = c,
- .by_parent_cb = by_parent_cb,
};
acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, NULL);
g_assert(acb != NULL);
- if (!by_parent_cb) {
- /* set .drained_begin cb to run only in the following drain. */
- detach_by_driver_cb_class.drained_begin =
- detach_by_driver_cb_drained_begin;
- }
+ /* set .drained_begin cb to run only in the following drain. */
+ detach_by_driver_cb_class.drained_begin =
+ detach_by_driver_cb_drained_begin;
/* Drain and check the expected result */
bdrv_subtree_drained_begin(parent_b);
@@ -1462,14 +1444,9 @@ static void test_detach_indirect(bool by_parent_cb)
bdrv_unref(c);
}
-static void test_detach_by_parent_cb(void)
-{
- test_detach_indirect(true);
-}
-
static void test_detach_by_driver_cb(void)
{
- test_detach_indirect(false);
+ test_detach_indirect();
}
static void test_append_to_drained(void)
@@ -2224,7 +2201,6 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/detach/drain_all", test_detach_by_drain_all);
g_test_add_func("/bdrv-drain/detach/drain", test_detach_by_drain);
g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
- g_test_add_func("/bdrv-drain/detach/parent_cb", test_detach_by_parent_cb);
g_test_add_func("/bdrv-drain/detach/driver_cb", test_detach_by_driver_cb);
g_test_add_func("/bdrv-drain/attach/drain", test_append_to_drained);
This test uses a callback of an I/O function (blk_aio_preadv) to modify the graph, using bdrv_attach_child. This is simply not allowed anymore. I/O cannot change the graph. Before "block/io.c: make bdrv_do_drained_begin_quiesce static and introduce bdrv_drained_begin_no_poll", the test would simply be at risk of failure, because if bdrv_replace_child_noperm() (called to modify the graph) would call a drain, then one callback of .drained_begin() is bdrv_do_drained_begin_quiesce, that specifically asserts that we are not in a coroutine. Now that we fixed the behavior, the drain will invoke a bh in the main loop, so we don't have such problem. However, this test is still illegal and fails because we forbid graph changes from I/O paths. Once we add the required subtree_drains to protect bdrv_replace_child_noperm(), the key problem in this test is in: acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, NULL); /* Drain and check the expected result */ bdrv_subtree_drained_begin(parent_b); because the detach_by_parent_aio_cb calls detach_indirect_bh(), that modifies the graph and is invoked during bdrv_subtree_drained_begin(). The call stack is the following: 1. blk_aio_preadv() creates a coroutine, increments in_flight counter and enters the coroutine running blk_aio_read_entry() 2. blk_aio_read_entry() performs the read and then schedules a bh to complete (blk_aio_complete) 3. at this point, subtree_drained_begin() kicks in and waits for all in_flight requests, polling 4. polling allows the bh to be scheduled, so blk_aio_complete runs 5. blk_aio_complete *first* invokes the callback (detach_by_parent_aio_cb) and then decrements the in_flight counter 6. Here we have the problem: detach_by_parent_aio_cb modifies the graph, so both bdrv_unref_child() and bdrv_attach_child() will have subtree_drains inside. And this causes a deadlock, because the nested drain will wait for in_flight counter to go to zero, which is only happening once the drain itself finishes. Different story is test_detach_by_driver_cb(): in this case, detach_by_parent_aio_cb() does not call detach_indirect_bh(), but it is instead called as a bh running in the main loop by detach_by_driver_cb_drained_begin(), the callback for .drained_begin(). This test was added in 231281ab42 and part of the series "Drain fixes and cleanups, part 3" https://lists.nongnu.org/archive/html/qemu-block/2018-05/msg01132.html but as explained above I believe that it is not valid anymore, and can be discarded. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- tests/unit/test-bdrv-drain.c | 46 +++++++++--------------------------- 1 file changed, 11 insertions(+), 35 deletions(-)