Re: [Qemu-devel] [PATCH 03/16] block: introduce bdrv_no_throttling_begin/end

[Fam Zheng <famz@redhat.com>]

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 <pbonzini@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
> 
> 
>