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
