Re: [PATCH 5/6] test-bdrv-drain.c: remove test_detach_by_parent_cb()

[Paolo Bonzini]

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