On Tue, 02/16 18:56, Paolo Bonzini wrote:
> Extract the handling of throttling from bdrv_flush_io_queue.
Looks good overall. Have two questions below.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> block.c | 1 -
> block/io.c | 56
> +++++++++++++++++++++++++++++------------------
> block/throttle-groups.c | 4 ++++
> include/block/block_int.h | 6 ++---
> 4 files changed, 42 insertions(+), 25 deletions(-)
>
> diff --git a/block.c b/block.c
> index efc3c43..b4f328f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2314,7 +2314,6 @@ static void swap_feature_fields(BlockDriverState
> *bs_top,
>
> assert(!bs_new->throttle_state);
> if (bs_top->throttle_state) {
> - assert(bs_top->io_limits_enabled);
> bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
> bdrv_io_limits_disable(bs_top);
> }
> diff --git a/block/io.c b/block/io.c
> index 5ee5032..272caac 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -69,28 +69,43 @@ void bdrv_set_io_limits(BlockDriverState *bs,
> throttle_group_config(bs, cfg);
> }
>
> -static void bdrv_start_throttled_reqs(BlockDriverState *bs)
> +static void bdrv_no_throttling_begin(BlockDriverState *bs)
> {
> - bool enabled = bs->io_limits_enabled;
> + BdrvChild *child;
> +
> + QLIST_FOREACH(child, &bs->children, next) {
> + bdrv_no_throttling_begin(child->bs);
> + }
>
> - bs->io_limits_enabled = false;
> - throttle_group_restart_bs(bs);
> - bs->io_limits_enabled = enabled;
> + if (bs->io_limits_disabled++ == 0) {
> + throttle_group_restart_bs(bs);
> + }
> +}
> +
> +static void bdrv_no_throttling_end(BlockDriverState *bs)
> +{
> + BdrvChild *child;
> +
> + --bs->io_limits_disabled;
> +
> + QLIST_FOREACH(child, &bs->children, next) {
> + bdrv_no_throttling_end(child->bs);
> + }
> }
>
> void bdrv_io_limits_disable(BlockDriverState *bs)
> {
> - bs->io_limits_enabled = false;
> - bdrv_start_throttled_reqs(bs);
> + assert(bs->throttle_state);
> + bdrv_no_throttling_begin(bs);
> throttle_group_unregister_bs(bs);
> + bdrv_no_throttling_end(bs);
> }
>
> /* should be called before bdrv_set_io_limits if a limit is set */
> void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
> {
> - assert(!bs->io_limits_enabled);
> + assert(!bs->throttle_state);
> throttle_group_register_bs(bs, group);
> - bs->io_limits_enabled = true;
> }
>
> void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
> @@ -255,6 +270,7 @@ void bdrv_drain(BlockDriverState *bs)
> {
> bool busy = true;
>
> + bdrv_no_throttling_begin(bs);
> bdrv_drain_recurse(bs);
> while (busy) {
> /* Keep iterating */
> @@ -262,6 +278,7 @@ void bdrv_drain(BlockDriverState *bs)
> busy = bdrv_requests_pending(bs);
> busy |= aio_poll(bdrv_get_aio_context(bs), busy);
> }
> + bdrv_no_throttling_end(bs);
> }
>
> /*
> @@ -284,6 +301,7 @@ void bdrv_drain_all(void)
> if (bs->job) {
> block_job_pause(bs->job);
> }
> + bdrv_no_throttling_begin(bs);
> bdrv_drain_recurse(bs);
> aio_context_release(aio_context);
>
> @@ -325,6 +343,7 @@ void bdrv_drain_all(void)
> AioContext *aio_context = bdrv_get_aio_context(bs);
>
> aio_context_acquire(aio_context);
> + bdrv_no_throttling_end(bs);
> if (bs->job) {
> block_job_resume(bs->job);
> }
> @@ -555,11 +574,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t
> offset,
> * will not fire; so the I/O throttling function has to be disabled here
> * if it has been enabled.
> */
> - if (bs->io_limits_enabled) {
> - fprintf(stderr, "Disabling I/O throttling on '%s' due "
> - "to synchronous I/O.\n", bdrv_get_device_name(bs));
> - bdrv_io_limits_disable(bs);
> - }
> + bdrv_no_throttling_begin(bs);
>
> if (qemu_in_coroutine()) {
> /* Fast-path if already in coroutine context */
> @@ -573,6 +588,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t
> offset,
> aio_poll(aio_context, true);
> }
> }
> +
> + bdrv_no_throttling_end(bs);
Does this change the behavior? There wasn't a bdrv_io_limits_enable() here, and
the throttle doesn't come back automatically. Just want to make sure it's
intended.
> return rwco.ret;
> }
>
> @@ -608,13 +625,11 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
> uint8_t *buf, int nb_sectors)
> {
> - bool enabled;
> int ret;
>
> - enabled = bs->io_limits_enabled;
> - bs->io_limits_enabled = false;
> + bdrv_no_throttling_begin(bs);
> ret = bdrv_read(bs, sector_num, buf, nb_sectors);
> - bs->io_limits_enabled = enabled;
> + bdrv_no_throttling_end(bs);
> return ret;
> }
>
> @@ -952,7 +967,7 @@ static int coroutine_fn
> bdrv_co_do_preadv(BlockDriverState *bs,
> }
>
> /* throttling disk I/O */
> - if (bs->io_limits_enabled) {
> + if (bs->throttle_state) {
> throttle_group_co_io_limits_intercept(bs, bytes, false);
> }
>
> @@ -1294,7 +1309,7 @@ static int coroutine_fn
> bdrv_co_do_pwritev(BlockDriverState *bs,
> }
>
> /* throttling disk I/O */
> - if (bs->io_limits_enabled) {
> + if (bs->throttle_state) {
> throttle_group_co_io_limits_intercept(bs, bytes, true);
> }
>
> @@ -2749,7 +2764,6 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
> } else if (bs->file) {
> bdrv_flush_io_queue(bs->file->bs);
> }
> - bdrv_start_throttled_reqs(bs);
> }
>
> void bdrv_drained_begin(BlockDriverState *bs)
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index eccfc0d..8fe0a4f 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -219,6 +219,10 @@ static bool
> throttle_group_schedule_timer(BlockDriverState *bs,
> ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
> bool must_wait;
>
> + if (bs->io_limits_disabled) {
> + return false;
> + }
> +
Before, this function and its direct caller
throttle_group_co_io_limits_intercept are not called if !bs->io_limits_enabled,
so the accounting is not done. It's better, but just different.
Thanks,
Fam
> /* Check if any of the timers in this group is already armed */
> if (tg->any_timer_armed[is_write]) {
> return true;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9ef823a..ed2e034 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -412,10 +412,10 @@ struct BlockDriverState {
>
> /* I/O throttling.
> * throttle_state tells us if this BDS has I/O limits configured.
> - * io_limits_enabled tells us if they are currently being
> - * enforced, but it can be temporarily set to false */
> + * io_limits_disabled tells us if they are currently being enforced */
> CoQueue throttled_reqs[2];
> - bool io_limits_enabled;
> + unsigned int io_limits_disabled;
> +
> /* The following fields are protected by the ThrottleGroup lock.
> * See the ThrottleGroup documentation for details. */
> ThrottleState *throttle_state;
> --
> 2.5.0
>
>
>
