On Tue, Nov 08, 2022 at 01:37:28PM +0100, Kevin Wolf wrote: > Polling during bdrv_drained_end() can be problematic (and in the future, > we may get cases for bdrv_drained_begin() where polling is forbidden, > and we don't care about already in-flight requests, but just want to > prevent new requests from arriving). > > The .bdrv_drained_begin/end callbacks running in a coroutine is the only > reason why we have to do this polling, so make them non-coroutine > callbacks again. None of the callers actually yield any more. > > This means that bdrv_drained_end() effectively doesn't poll any more, > even if AIO_WAIT_WHILE() loops are still there (their condition is false > from the beginning). This is generally not a problem, but in > test-bdrv-drain, some additional explicit aio_poll() calls need to be > added because the test case wants to verify the final state after BHs > have executed. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > include/block/block_int-common.h | 10 ++++--- > block.c | 4 +-- > block/io.c | 49 +++++--------------------------- > block/qed.c | 4 +-- > block/throttle.c | 6 ++-- > tests/unit/test-bdrv-drain.c | 18 ++++++------ > 6 files changed, 30 insertions(+), 61 deletions(-)
Wow, surprisingly little has to change to make these non-coroutine_fn.
>
> diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> index 5a2cc077a0..0956acbb60 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -735,17 +735,19 @@ struct BlockDriver {
> void (*bdrv_io_unplug)(BlockDriverState *bs);
>
> /**
> - * bdrv_co_drain_begin is called if implemented in the beginning of a
> + * bdrv_drain_begin is called if implemented in the beginning of a
> * drain operation to drain and stop any internal sources of requests in
> * the driver.
> - * bdrv_co_drain_end is called if implemented at the end of the drain.
> + * bdrv_drain_end is called if implemented at the end of the drain.
> *
> * They should be used by the driver to e.g. manage scheduled I/O
> * requests, or toggle an internal state. After the end of the drain new
> * requests will continue normally.
> + *
> + * Implementations of both functions must not call aio_poll().
> */
> - void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
> - void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
> + void (*bdrv_drain_begin)(BlockDriverState *bs);
> + void (*bdrv_drain_end)(BlockDriverState *bs);
>
> bool (*bdrv_supports_persistent_dirty_bitmap)(BlockDriverState *bs);
> bool coroutine_fn (*bdrv_co_can_store_new_dirty_bitmap)(
> diff --git a/block.c b/block.c
> index 3bd594eb2a..fed8077993 100644
> --- a/block.c
> +++ b/block.c
> @@ -1705,8 +1705,8 @@ static int bdrv_open_driver(BlockDriverState *bs,
> BlockDriver *drv,
> assert(is_power_of_2(bs->bl.request_alignment));
>
> for (i = 0; i < bs->quiesce_counter; i++) {
> - if (drv->bdrv_co_drain_begin) {
> - drv->bdrv_co_drain_begin(bs);
> + if (drv->bdrv_drain_begin) {
> + drv->bdrv_drain_begin(bs);
> }
> }
>
> diff --git a/block/io.c b/block/io.c
> index 34b30e304e..183b407f5b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -250,55 +250,20 @@ typedef struct {
> int *drained_end_counter;
> } BdrvCoDrainData;
>
> -static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
> -{
> - BdrvCoDrainData *data = opaque;
> - BlockDriverState *bs = data->bs;
> -
> - if (data->begin) {
> - bs->drv->bdrv_co_drain_begin(bs);
> - } else {
> - bs->drv->bdrv_co_drain_end(bs);
> - }
> -
> - /* Set data->done and decrement drained_end_counter before bdrv_wakeup()
> */
> - qatomic_mb_set(&data->done, true);
> - if (!data->begin) {
> - qatomic_dec(data->drained_end_counter);
> - }
> - bdrv_dec_in_flight(bs);
> -
> - g_free(data);
> -}
> -
> -/* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
> +/* Recursively call BlockDriver.bdrv_drain_begin/end callbacks */
> static void bdrv_drain_invoke(BlockDriverState *bs, bool begin,
> int *drained_end_counter)
> {
> - BdrvCoDrainData *data;
> -
> - if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
> - (!begin && !bs->drv->bdrv_co_drain_end)) {
> + if (!bs->drv || (begin && !bs->drv->bdrv_drain_begin) ||
> + (!begin && !bs->drv->bdrv_drain_end)) {
> return;
> }
>
> - data = g_new(BdrvCoDrainData, 1);
> - *data = (BdrvCoDrainData) {
> - .bs = bs,
> - .done = false,
> - .begin = begin,
> - .drained_end_counter = drained_end_counter,
> - };
> -
> - if (!begin) {
> - qatomic_inc(drained_end_counter);
> + if (begin) {
> + bs->drv->bdrv_drain_begin(bs);
> + } else {
> + bs->drv->bdrv_drain_end(bs);
> }
> -
> - /* Make sure the driver callback completes during the polling phase for
> - * drain_begin. */
> - bdrv_inc_in_flight(bs);
> - data->co = qemu_coroutine_create(bdrv_drain_invoke_entry, data);
> - aio_co_schedule(bdrv_get_aio_context(bs), data->co);
> }
>
> /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
> diff --git a/block/qed.c b/block/qed.c
> index 013f826c44..301ff8fd86 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -365,7 +365,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState
> *bs,
> }
> }
>
> -static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
> +static void bdrv_qed_co_drain_begin(BlockDriverState *bs)
This function needs to be renamed s/_co_//.
> {
> BDRVQEDState *s = bs->opaque;
>
> @@ -1661,7 +1661,7 @@ static BlockDriver bdrv_qed = {
> .bdrv_co_check = bdrv_qed_co_check,
> .bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
> .bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
> - .bdrv_co_drain_begin = bdrv_qed_co_drain_begin,
> + .bdrv_drain_begin = bdrv_qed_co_drain_begin,
> };
>
> static void bdrv_qed_init(void)
> diff --git a/block/throttle.c b/block/throttle.c
> index 131eba3ab4..6e3ae1b355 100644
> --- a/block/throttle.c
> +++ b/block/throttle.c
> @@ -214,7 +214,7 @@ static void throttle_reopen_abort(BDRVReopenState
> *reopen_state)
> reopen_state->opaque = NULL;
> }
>
> -static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
> +static void throttle_co_drain_begin(BlockDriverState *bs)
Same here.
> {
> ThrottleGroupMember *tgm = bs->opaque;
> if (qatomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
> @@ -261,8 +261,8 @@ static BlockDriver bdrv_throttle = {
> .bdrv_reopen_commit = throttle_reopen_commit,
> .bdrv_reopen_abort = throttle_reopen_abort,
>
> - .bdrv_co_drain_begin = throttle_co_drain_begin,
> - .bdrv_co_drain_end = throttle_co_drain_end,
Is throttle_co_drain_end() still marked coroutine_fn? It also need to be
renamed to throttle_drain_end().
> + .bdrv_drain_begin = throttle_co_drain_begin,
> + .bdrv_drain_end = throttle_co_drain_end,
>
> .is_filter = true,
> .strong_runtime_opts = throttle_strong_runtime_opts,
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index 24f34e24ad..695519ee02 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -46,7 +46,7 @@ static void coroutine_fn sleep_in_drain_begin(void *opaque)
> bdrv_dec_in_flight(bs);
> }
>
> -static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
> +static void bdrv_test_drain_begin(BlockDriverState *bs)
> {
> BDRVTestState *s = bs->opaque;
> s->drain_count++;
> @@ -57,7 +57,7 @@ static void coroutine_fn
> bdrv_test_co_drain_begin(BlockDriverState *bs)
> }
> }
>
> -static void coroutine_fn bdrv_test_co_drain_end(BlockDriverState *bs)
> +static void bdrv_test_drain_end(BlockDriverState *bs)
> {
> BDRVTestState *s = bs->opaque;
> s->drain_count--;
> @@ -111,8 +111,8 @@ static BlockDriver bdrv_test = {
> .bdrv_close = bdrv_test_close,
> .bdrv_co_preadv = bdrv_test_co_preadv,
>
> - .bdrv_co_drain_begin = bdrv_test_co_drain_begin,
> - .bdrv_co_drain_end = bdrv_test_co_drain_end,
> + .bdrv_drain_begin = bdrv_test_drain_begin,
> + .bdrv_drain_end = bdrv_test_drain_end,
>
> .bdrv_child_perm = bdrv_default_perms,
>
> @@ -1703,6 +1703,7 @@ static void test_blockjob_commit_by_drained_end(void)
> bdrv_drained_begin(bs_child);
> g_assert(!job_has_completed);
> bdrv_drained_end(bs_child);
> + aio_poll(qemu_get_aio_context(), false);
> g_assert(job_has_completed);
>
> bdrv_unref(bs_parents[0]);
> @@ -1858,6 +1859,7 @@ static void test_drop_intermediate_poll(void)
>
> g_assert(!job_has_completed);
> ret = bdrv_drop_intermediate(chain[1], chain[0], NULL);
> + aio_poll(qemu_get_aio_context(), false);
> g_assert(ret == 0);
> g_assert(job_has_completed);
>
> @@ -1946,7 +1948,7 @@ static void coroutine_fn
> bdrv_replace_test_drain_co(void *opaque)
> * .was_drained.
> * Increment .drain_count.
> */
> -static void coroutine_fn bdrv_replace_test_co_drain_begin(BlockDriverState
> *bs)
> +static void bdrv_replace_test_drain_begin(BlockDriverState *bs)
> {
> BDRVReplaceTestState *s = bs->opaque;
>
> @@ -1977,7 +1979,7 @@ static void coroutine_fn
> bdrv_replace_test_read_entry(void *opaque)
> * If .drain_count reaches 0 and the node has a backing file, issue a
> * read request.
> */
> -static void coroutine_fn bdrv_replace_test_co_drain_end(BlockDriverState *bs)
> +static void bdrv_replace_test_drain_end(BlockDriverState *bs)
> {
> BDRVReplaceTestState *s = bs->opaque;
>
> @@ -2002,8 +2004,8 @@ static BlockDriver bdrv_replace_test = {
> .bdrv_close = bdrv_replace_test_close,
> .bdrv_co_preadv = bdrv_replace_test_co_preadv,
>
> - .bdrv_co_drain_begin = bdrv_replace_test_co_drain_begin,
> - .bdrv_co_drain_end = bdrv_replace_test_co_drain_end,
> + .bdrv_drain_begin = bdrv_replace_test_drain_begin,
> + .bdrv_drain_end = bdrv_replace_test_drain_end,
>
> .bdrv_child_perm = bdrv_default_perms,
> };
> --
> 2.38.1
>
signature.asc
Description: PGP signature
