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

[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, 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 <vsementsov@virtuozzo.com>
> ---
>   block/io.c | 161 +++++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 124 insertions(+), 37 deletions(-)

>   
> +int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
> +    int64_t offset, unsigned int bytes,
> +    QEMUIOVector *qiov, size_t qiov_offset,
> +    BdrvRequestFlags flags)
> +{

Doesn't seem to be the usual indentation in this file.

> @@ -1922,7 +1934,8 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
>       return bdrv_co_pwritev_part(child, offset, bytes, qiov, 0, flags);
>   }
>   
> -int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
> +/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */
> +static int coroutine_fn bdrv_do_pwritev_part(BdrvChild *child,
>       int64_t offset, unsigned int bytes, QEMUIOVector *qiov, size_t qiov_offset,
>       BdrvRequestFlags flags)
>   {

then again, it was in use here, and saves reindenting the remaining 
lines.  I'll let the maintainer decide which style is preferred.

> @@ -2014,17 +2038,18 @@ typedef struct RwCo {
>       BdrvRequestFlags flags;
>   } RwCo;
>   
> +/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */
>   static void coroutine_fn bdrv_rw_co_entry(void *opaque)
>   {
>       RwCo *rwco = opaque;
>   
>       if (!rwco->is_write) {
> -        rwco->ret = bdrv_co_preadv(rwco->child, rwco->offset,
> -                                   rwco->qiov->size, rwco->qiov,
> +        rwco->ret = bdrv_do_preadv_part(rwco->child, rwco->offset,
> +                                   rwco->qiov->size, rwco->qiov, 0,
>                                      rwco->flags);

Indentation is now off.

>       } else {
> -        rwco->ret = bdrv_co_pwritev(rwco->child, rwco->offset,
> -                                    rwco->qiov->size, rwco->qiov,
> +        rwco->ret = bdrv_do_pwritev_part(rwco->child, rwco->offset,
> +                                    rwco->qiov->size, rwco->qiov, 0,
>                                       rwco->flags);

and again

> @@ -3411,9 +3478,12 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
>    * If 'exact' is true, the file must be resized to exactly the given
>    * 'offset'.  Otherwise, it is sufficient for the node to be at least
>    * 'offset' bytes in length.
> + *
> + * To be called between exactly one pair of bdrv_inc/dec_in_flight()
>    */
> -int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
> -                                  PreallocMode prealloc, Error **errp)
> +static int coroutine_fn bdrv_do_truncate(BdrvChild *child,
> +                                         int64_t offset, bool exact,
> +                                         PreallocMode prealloc, Error **errp)

Needs to be rebased, now that master has Kevin's patches addeing a 
'BdrvRequestFlags flags' parameter.  But the rebase should be obvious.

Otherwise looks sane to me, but I may be missing one of the finer points 
on which functions should be decorated with 'coroutine_fn'.

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

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