Re: [PATCH v2 5/9] block/io: expand in_flight inc/dec section: simple cases

[Kevin Wolf]

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 <address@hidden>

> 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