Am 27.04.2020 um 16:39 hat Vladimir Sementsov-Ogievskiy geschrieben: > It's safer to expand in_flight request to start before enter to > coroutine in synchronous wrappers, due to the following (theoretical) > problem: > > Consider write. > It's possible, that qemu_coroutine_enter only schedules execution, > assume such case. > > Then we may possibly have the following: > > 1. Somehow check that we are not in drained section in outer code. > > 2. Call bdrv_pwritev(), assuming that it will increase in_flight, which > will protect us from starting drained section. > > 3. It calls bdrv_prwv_co() -> bdrv_coroutine_enter() (not yet increased > in_flight). > > 4. Assume coroutine not yet actually entered, only scheduled, and we go > to some code, which starts drained section (as in_flight is zero). > > 5. Scheduled coroutine starts, and blindly increases in_flight, and we > are in drained section with in_flight request. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> diff --git a/block/io.c b/block/io.c > index 061f3f2590..a91d8c1e21 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1511,7 +1511,8 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child, > return bdrv_co_preadv_part(child, offset, bytes, qiov, 0, flags); > } > > -int coroutine_fn bdrv_co_preadv_part(BdrvChild *child, > +/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */ You have lots of comments like this one. Isn't this condition too strict, though? In the BlockBackend layer, it needs to be true because blk_wait_while_drained() decreases in_flight only once (which is an ugly hack, honestly, but it works...). It's comparable to how AIO_WAIT_WHILE() relies on having locked the context exactly once even though it is a recursive lock, because it wants to drop the lock temporarily. I don't think the same reasoning applies to BDS in_flight, does it? We can potentially simplify the code if we don't have to fulfill the condition. Kevin