| Message ID | 20220208153655.1251658-6-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: bug fixes in preparation of AioContext removal | expand |
On Tue, Feb 08, 2022 at 10:36:54AM -0500, Emanuele Giuseppe Esposito wrote: > 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(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben: > 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. The callback of an I/O function isn't I/O, though. It is code _after_ the I/O has completed. If this doesn't work any more, it feels like this is a bug. I think it becomes a bit more obvious when you translate the AIO into the equivalent coroutine function: void coroutine_fn foo(...) { GLOBAL_STATE_CODE(); blk_co_preadv(...); detach_by_parent_aio_cb(...); } This is obviously correct code that must work. Calling an I/O function from a GS function is allowed, and of course the GS function may continue to do GS stuff after the I/O function returned. (Actually, I'm not sure if it really works when blk is not in the main AioContext, but your API split patches claim that it does, so let's assume for the moment that this is already true :-)) > 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: Probably a question for a different patch, but why is a subtree drain required instead of just a normal node drain? It feels like a bigger hammer than what is needed for replacing a single child. > 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. So the problem has nothing to do with detach_by_parent_aio_cb() being an I/O function in the sense of locking rules (which it isn't), but with calling a drain operation while having the in_flight counter increased. In other words, an AIO callback like detach_by_parent_aio_cb() must never call drain - and it doesn't before your changes to bdrv_replace_child_noperm() break it. How confident are we that this only breaks tests and not real code? Part of the problem is probably that drain is serving two slightly different purposes inside the block layer (just make sure that nothing touches the node any more) and callers outside of the block layer (make sure that everything has completed and no more callbacks will be called). This bug sits exactly in the difference between those two concepts - we're waiting for more than we would have to wait for, and it causes a deadlock in this combination. I guess it could be fixed if BlockBackend accounted for requests that are already completed, but their callback hasn't yet. blk_drain() would then have to wait for those requests, but blk_root_drained_poll() wouldn't because these requests don't affect the root node any more. > 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> I feel throwing tests away just because they don't pass any more is a bit too simple. :-) Kevin
On 11/02/2022 16:38, Kevin Wolf wrote: > Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben: >> 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. > > The callback of an I/O function isn't I/O, though. It is code _after_ > the I/O has completed. If this doesn't work any more, it feels like this > is a bug. > > I think it becomes a bit more obvious when you translate the AIO into > the equivalent coroutine function: > > void coroutine_fn foo(...) > { > GLOBAL_STATE_CODE(); > > blk_co_preadv(...); > detach_by_parent_aio_cb(...); > } > > This is obviously correct code that must work. Calling an I/O function > from a GS function is allowed, and of course the GS function may > continue to do GS stuff after the I/O function returned. > > (Actually, I'm not sure if it really works when blk is not in the main > AioContext, but your API split patches claim that it does, so let's > assume for the moment that this is already true :-)) Uhmm why does my split claims that it is? all blk_aio_* are IO_CODE. Also, as you said, if blk is not in the main loop, the callback is not GS either. > >> 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: > > Probably a question for a different patch, but why is a subtree drain > required instead of just a normal node drain? It feels like a bigger > hammer than what is needed for replacing a single child. > >> 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. > > So the problem has nothing to do with detach_by_parent_aio_cb() being > an I/O function in the sense of locking rules (which it isn't), but with > calling a drain operation while having the in_flight counter increased. > > In other words, an AIO callback like detach_by_parent_aio_cb() must > never call drain - and it doesn't before your changes to > bdrv_replace_child_noperm() break it. How confident are we that this > only breaks tests and not real code? I am not sure. From a quick look, the AIO callback is really used pretty much everywhere. Maybe we should really find a way to asseert GLOBAL_STATE_CODE and friends? > > Part of the problem is probably that drain is serving two slightly > different purposes inside the block layer (just make sure that nothing > touches the node any more) and callers outside of the block layer (make > sure that everything has completed and no more callbacks will be > called). This bug sits exactly in the difference between those two > concepts - we're waiting for more than we would have to wait for, and it > causes a deadlock in this combination. > > I guess it could be fixed if BlockBackend accounted for requests that > are already completed, but their callback hasn't yet. blk_drain() would > then have to wait for those requests, but blk_root_drained_poll() > wouldn't because these requests don't affect the root node any more. > >> 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> > > I feel throwing tests away just because they don't pass any more is a > bit too simple. :-) Well if the test is doing something it is not supposed to do, why not :) And anyways this was obviously discussed in IRC with Paolo and somebody else, can't remember who. Emanuele > > Kevin >
On 2/11/22 16:38, Kevin Wolf wrote: > The callback of an I/O function isn't I/O, though. It is code_after_ > the I/O has completed. If this doesn't work any more, it feels like this > is a bug. The issue is that the I/O has *not* completed yet. blk_aio_preadv(..., cb, opaque) is not equivalent to ret = blk_co_preadv(...); cb(ret, opaque); but rather to blk_inc_in_flight(blk); ret = blk_co_preadv(...); cb(ret, opaque); blk_dec_in_flight(blk); Your own commit message (yeah I know we've all been through that :)) explains why, and notes that it is now invalid to drain in a callback: commit 46aaf2a566e364a62315219255099cbf1c9b990d Author: Kevin Wolf <kwolf@redhat.com> Date: Thu Sep 6 17:47:22 2018 +0200 block-backend: Decrease in_flight only after callback Request callbacks can do pretty much anything, including operations that will yield from the coroutine (such as draining the backend). In that case, a decreased in_flight would be visible to other code and could lead to a drain completing while the callback hasn't actually completed yet. Note that reordering these operations forbids calling drain directly inside an AIO callback. So the questions are: 1) is the above commit wrong though well-intentioned? 2) is it unexpected that bdrv_replace_child_noperm() drains (thus becoming invalid from the callback, albeit valid through a bottom half)? My answer is respectively 1) it's correct, many coroutines do inc_in_flight before creation and dec_in_flight at the end, we're just saying that it's _always_ the case for callback-based operations; 2) no, it's not unexpected and therefore the test is the incorrect one. Paolo
Am 14.02.2022 um 12:42 hat Paolo Bonzini geschrieben: > On 2/11/22 16:38, Kevin Wolf wrote: > > The callback of an I/O function isn't I/O, though. It is code_after_ > > the I/O has completed. If this doesn't work any more, it feels like this > > is a bug. > > The issue is that the I/O has *not* completed yet. blk_aio_preadv(..., cb, > opaque) is not equivalent to > > ret = blk_co_preadv(...); > cb(ret, opaque); > > but rather to > > blk_inc_in_flight(blk); > ret = blk_co_preadv(...); > cb(ret, opaque); > blk_dec_in_flight(blk); It depends on what you call "the I/O". The request to blk_bs(blk) has already completed, but the request to blk itself hasn't yet. This is exactly the difference I was talking about: bdrv_replace_child_noperm() really only cares about requests to the BlockDriverState, but drain currently waits for attached BlockBackends, too. The BlockBackend could safely return false from blk_root_drained_poll() while requests are still in their callbacks (if they do anything that touches a node, they would increase in_flight again), it just doesn't do it yet. It's only blk_drain(_all)() that would still have to wait for those. > Your own commit message (yeah I know we've all been through that :)) > explains why, and notes that it is now invalid to drain in a callback: > > commit 46aaf2a566e364a62315219255099cbf1c9b990d > Author: Kevin Wolf <kwolf@redhat.com> > Date: Thu Sep 6 17:47:22 2018 +0200 > > block-backend: Decrease in_flight only after callback > > Request callbacks can do pretty much anything, including operations > that will yield from the coroutine (such as draining the backend). > In that case, a decreased in_flight would be visible to other code > and could lead to a drain completing while the callback hasn't > actually completed yet. > > Note that reordering these operations forbids calling drain directly > inside an AIO callback. Yes, I wasn't aware of this any more, but I've come to the same conclusion in my previous message in this thread. > So the questions are: > > 1) is the above commit wrong though well-intentioned? > > 2) is it unexpected that bdrv_replace_child_noperm() drains (thus becoming > invalid from the callback, albeit valid through a bottom half)? > > > My answer is respectively 1) it's correct, many coroutines do inc_in_flight > before creation and dec_in_flight at the end, we're just saying that it's > _always_ the case for callback-based operations; 2) no, it's not unexpected > and therefore the test is the incorrect one. My question isn't really only about the test, though. If it is now forbidden to call bdrv_replace_child_noperm() in a callback, how do we verify that the test is the only incorrect one rather than just the obvious one? And is it better to throw away the test and find and fix all other places that are using something that is now forbidden, or wouldn't it be better to actually allow bdrv_replace_child_noperm() in callbacks? Kevin
On 2/14/22 16:28, Kevin Wolf wrote: > The BlockBackend could safely return false from blk_root_drained_poll() > while requests are still in their callbacks (if they do anything that > touches a node, they would increase in_flight again), it just doesn't do > it yet. It's only blk_drain(_all)() that would still have to wait for > those. That would be very subtle, especially it's not clear to me why this wouldn't be "a drain completing while the callback hasn't actually completed yet". The drain referred to in the commit message of 46aaf2a56 could *not* use blk_drain, because it is not coroutine friendly; it must use bdrv_drained_begin. >> My answer is respectively 1) it's correct, many coroutines do inc_in_flight >> before creation and dec_in_flight at the end, we're just saying that it's >> _always_ the case for callback-based operations; 2) no, it's not unexpected >> and therefore the test is the incorrect one. > My question isn't really only about the test, though. If it is now > forbidden to call bdrv_replace_child_noperm() in a callback, how do we > verify that the test is the only incorrect one rather than just the > obvious one? > > And is it better to throw away the test and find and fix all other > places that are using something that is now forbidden, or wouldn't it be > better to actually allow bdrv_replace_child_noperm() in callbacks? The question is would you ever need bdrv_replace_child_noperm() in callbacks? The AIO functions are called from any iothread and so are the callbacks. We do have a usecase (in block/mirror.c) for bdrv_drained_begin from a coroutine; do we have a usecase for calling global-state functions from a callback, in such a way that: 1) going through a bottom half would not be possible 2) it's only needed in the special case of a BlockBackend homed in the main event loop (because otherwise you'd have to go through a bottom half, and we have excluded that already)? Thanks, Paolo
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index c52ba2db4e..ef154b6536 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1316,7 +1316,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; @@ -1334,12 +1333,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; @@ -1361,31 +1355,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, @@ -1404,10 +1391,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); @@ -1419,7 +1404,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); @@ -1437,16 +1422,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); @@ -1482,14 +1464,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) @@ -2244,7 +2221,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(-)