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

[Kevin Wolf]

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