Re: [PATCH v2] block: Factor out bdrv_run_co()

[Kevin Wolf <kwolf@redhat.com>]

Am 19.05.2020 um 19:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We have a few bdrv_*() functions that can either spawn a new coroutine
> and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
> alreeady running in a coroutine. All of them duplicate basically the
> same code.
> 
> Factor the common code into a new function bdrv_run_co().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>    [Factor out bdrv_run_co_entry too]
> ---
> 
> Hi!
> 
> I'm a bit lost on rebasing "block/io: safer inc/dec in_flight sections"
> (is it needed or not?), so, I decided to send just this one patch:
> 
> I suggest to go a bit further, and refactor that bdrv_run_co don't need
> additional *ret argument neither NOT_DONE logic.

Hm, this approach adds another indirection and bdrv_pread/pwrite still
seems to be on some hot paths. But maybe this is just the right
motivation to clean up qcow2 a bit and use explicit bdrv_co_*() where it
is possible. I might take a look later.

>  block/io.c | 191 ++++++++++++++++++++---------------------------------
>  1 file changed, 70 insertions(+), 121 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 121ce17a49..794eebbd0c 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -35,8 +35,6 @@
>  #include "qemu/main-loop.h"
>  #include "sysemu/replay.h"
>  
> -#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
> -
>  /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
>  #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
>  
> @@ -891,29 +889,61 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>      return 0;
>  }
>  
> +typedef int coroutine_fn BdrvRequestEntry(void *opaque);
> +typedef struct BdrvRunCo {
> +    BdrvRequestEntry *entry;
> +    void *opaque;
> +    int ret;
> +    bool done;
> +} BdrvRunCo;
> +
> +static void coroutine_fn bdrv_run_co_entry(void *opaque)
> +{
> +    BdrvRunCo *arg = opaque;
> +
> +    arg->ret = arg->entry(arg->opaque);
> +    arg->done = true;
> +    aio_wait_kick();
> +}
> +
> +static int bdrv_run_co(BlockDriverState *bs, BdrvRequestEntry *entry,
> +                       void *opaque)
> +{
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        return entry(opaque);
> +    } else {
> +        BdrvRunCo s = { .entry = entry, .opaque = opaque };
> +
> +        bdrv_coroutine_enter(bs, qemu_coroutine_create(bdrv_run_co_entry, &s));

Let's keep the coroutine in a separate variable, maybe inside BdrvRunCo.
It's important for debugging BDRV_POLL_WHILE() hangs in gdb.

> +
> +        BDRV_POLL_WHILE(bs, !s.done);
> +
> +        return s.ret;
> +    }
> +}

Kevin