Re: [PATCH 2/2] qapi: Fix qmp_block_set_io_throttle blocked for too long

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

On Sat, Mar 08, 2025 at 06:16:18PM +0800, zoudongjie wrote:
> From: Zhu Yangyang <zhuyangyang14@huawei.com>
> 
> bdrv_drained_begin() is blocked for a long time when network storage is used
> and the network link has just failed.
> Therefore, the timeout period is set here.
> 
> Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com>
> ---
>  block/block-backend.c                       | 14 +++++++++++++-
>  block/qapi-system.c                         |  7 ++++++-
>  include/system/block-backend-global-state.h |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 9288f7e1c6..409d4559c3 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2691,20 +2691,32 @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
>  }
>  
>  void blk_io_limits_disable(BlockBackend *blk)
> +{
> +    blk_io_limits_disable_timeout(blk, -1);
> +}
> +
> +int blk_io_limits_disable_timeout(BlockBackend *blk, int64_t timeout_ms)
>  {
>      BlockDriverState *bs = blk_bs(blk);
>      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> +    int ret = 0;
> +
>      assert(tgm->throttle_state);
>      GLOBAL_STATE_CODE();
>      if (bs) {
>          bdrv_ref(bs);
> -        bdrv_drained_begin(bs);
> +        ret = bdrv_drained_begin_timeout(bs, timeout_ms);
> +        if (ret < 0) {
> +            goto out;
> +        }
>      }
>      throttle_group_unregister_tgm(tgm);
> +out:
>      if (bs) {
>          bdrv_drained_end(bs);
>          bdrv_unref(bs);
>      }
> +    return ret;
>  }
>  
>  /* should be called before blk_set_io_limits if a limit is set */
> diff --git a/block/qapi-system.c b/block/qapi-system.c
> index 54b7409b2b..96fa8e5e51 100644
> --- a/block/qapi-system.c
> +++ b/block/qapi-system.c
> @@ -39,6 +39,8 @@
>  #include "system/block-backend.h"
>  #include "system/blockdev.h"
>  
> +#define QMP_BLOCKING_TIMEOUT 5000 /* ms */
> +
>  static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
>                                   Error **errp)
>  {
> @@ -502,7 +504,10 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
>          blk_set_io_limits(blk, &cfg);
>      } else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
>          /* If all throttling settings are set to 0, disable I/O limits */
> -        blk_io_limits_disable(blk);
> +        if (blk_io_limits_disable_timeout(blk, QMP_BLOCKING_TIMEOUT) < 0) {

Please add a timeout parameter to the QMP command instead of hardcoding
the timeout. It should also support an infinite timeout and that should
be the default (so that existing users aren't surprised with a new error
they don't handle).

> +            error_setg(errp, "Blk io limits disable timeout");
> +            return;
> +        }
>      }
>  }
>  
> diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h
> index 9cc9b008ec..e55ea9dc64 100644
> --- a/include/system/block-backend-global-state.h
> +++ b/include/system/block-backend-global-state.h
> @@ -111,6 +111,7 @@ int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
>  
>  void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
>  void blk_io_limits_disable(BlockBackend *blk);
> +int blk_io_limits_disable_timeout(BlockBackend *blk, int64_t timeout_ms);
>  void blk_io_limits_enable(BlockBackend *blk, const char *group);
>  void blk_io_limits_update_group(BlockBackend *blk, const char *group);
>  void blk_set_force_allow_inactivate(BlockBackend *blk);
> -- 
> 2.33.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]