Re: [PATCH v2 6/9] block/io: expand in_flight inc/dec section: block-status

[Eric Blake <eblake@redhat.com>]

On 4/27/20 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's safer to expand in_flight request to start before enter to
> coroutine in synchronous wrappers and end after BDRV_POLL_WHILE loop.
> Note that qemu_coroutine_enter may only schedule the coroutine in some
> circumstances.

Wording suggestion:

It's safer to expand the region protected by an in_flight request to 
begin in the synchronous wrapper and end after the BDRV_POLL_WHILE loop. 
  Leaving the in_flight request in the coroutine itself risks a race 
where calling qemu_coroutine_enter() may have only scheduled, rather 
than started, the coroutine, allowing some other thread a chance to not 
realize an operation is in flight.

> 
> block-status requests are complex, they involve querying different
> block driver states across backing chain. Let's expand only in_flight
> section for the top bs, keeping other sections as is.

block-status requests are complex, involving a query of different block 
driver states across the backing chain.  Let's expand only the in_flight 
section for the top bs, and keep the other sections as-is.

I'd welcome Kevin's review on my next comment, but if I'm correct, I 
think we can further add the following justification to the commit message:

Gathering block status only requires reads from the block device, and 
backing devices are typically read-only, so losing any in_flight race on 
a backing device is less likely to cause problems with concurrent 
modifications on the overall backing chain.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/io.c | 65 ++++++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index a91d8c1e21..1cb6f433e5 100644
> --- a/block/io.c

> @@ -2624,15 +2646,19 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
>    * words, the result is not necessarily the maximum possible range);
>    * but 'pnum' will only be 0 when end of file is reached.
>    *
> + * To be called between exactly one pair of bdrv_inc/dec_in_flight() for top bs.
> + * bdrv_do_is_allocated_above takes care of increasing in_fligth for other block

in_flight

> + * driver states from bs backing chain.
>    */
>   static int coroutine_fn
> -bdrv_co_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> +bdrv_do_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>                              bool include_base, int64_t offset, int64_t bytes,
>                              int64_t *pnum)

> @@ -2682,11 +2710,16 @@ typedef struct BdrvCoIsAllocatedAboveData {
>       bool done;
>   } BdrvCoIsAllocatedAboveData;
>   
> +/*
> + * To be called between exactly one pair of bdrv_inc/dec_in_flight() for top bs.
> + * bdrv_do_is_allocated_above takes care of increasing in_fligth for other block
> + * driver states from the backing chain.
> + */
>   static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)

and again

Otherwise looks reasonable to me.  Fixing typos is trivial, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org