[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 04/15] test-bdrv-drain.c: remove test_detach_by_parent_cb()
From: |
Paolo Bonzini |
Subject: |
[PATCH 04/15] test-bdrv-drain.c: remove test_detach_by_parent_cb() |
Date: |
Mon, 12 Dec 2022 13:59:09 +0100 |
From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220314131854.2202651-8-eesposit@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
tests/unit/test-bdrv-drain.c | 42 +++++++++---------------------------
1 file changed, 10 insertions(+), 32 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 172bc6debc23..3be214f30186 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1106,7 +1106,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;
@@ -1124,12 +1123,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 void detach_by_driver_cb_drained_begin(BdrvChild *child)
@@ -1148,33 +1142,26 @@ static BdrvChildClass detach_by_driver_cb_class;
* \ / \
* 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.drained_begin =
- detach_by_driver_cb_drained_begin;
- }
+ detach_by_driver_cb_class = child_of_bds;
+ detach_by_driver_cb_class.drained_begin =
+ detach_by_driver_cb_drained_begin;
/* Create all involved nodes */
parent_a = bdrv_new_open_driver(&bdrv_test, "parent-a", BDRV_O_RDWR,
@@ -1193,10 +1180,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);
@@ -1208,7 +1193,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);
@@ -1226,7 +1211,6 @@ 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);
@@ -1271,14 +1255,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)
@@ -2029,7 +2008,6 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/deletion/drain", test_delete_by_drain);
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/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);
--
2.38.1
- [PATCH 00/12] More cleanups and fixes for drain, Paolo Bonzini, 2022/12/12
- [PATCH 01/15] Revert "block: Remove poll parameter from bdrv_parent_drained_begin_single()", Paolo Bonzini, 2022/12/12
- [PATCH 02/15] Revert "block: Don't poll in bdrv_replace_child_noperm()", Paolo Bonzini, 2022/12/12
- [PATCH 03/15] block: Pull polling out of bdrv_parent_drained_begin_single(), Paolo Bonzini, 2022/12/12
- [PATCH 04/15] test-bdrv-drain.c: remove test_detach_by_parent_cb(),
Paolo Bonzini <=
- [PATCH 05/15] tests/unit/test-bdrv-drain.c: graph setup functions can't run in coroutines, Paolo Bonzini, 2022/12/12
- [PATCH 06/15] tests/qemu-iotests/030: test_stream_parallel should use auto_finalize=False, Paolo Bonzini, 2022/12/12
- [PATCH 07/15] block-backend: enter aio coroutine only after drain, Paolo Bonzini, 2022/12/12
- [PATCH 08/15] nbd: a BlockExport always has a BlockBackend, Paolo Bonzini, 2022/12/12
- [PATCH 09/15] block-backend: make global properties write-once, Paolo Bonzini, 2022/12/12
- [PATCH 11/15] block-backend: make queued_requests thread-safe, Paolo Bonzini, 2022/12/12
- [PATCH 10/15] block-backend: always wait for drain before starting operation, Paolo Bonzini, 2022/12/12
- [PATCH 12/15] block: limit bdrv_co_yield_to_drain to drain_begin, Paolo Bonzini, 2022/12/12
- [PATCH 15/15] block: only get out of coroutine context for polling, Paolo Bonzini, 2022/12/12
- [PATCH 13/15] block: second argument of bdrv_do_drained_end is always NULL, Paolo Bonzini, 2022/12/12