[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] block: Factor out bdrv_run_co()
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2] block: Factor out bdrv_run_co() |
Date: |
Wed, 20 May 2020 16:05:00 +0200 |
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 <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> [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