* [PATCH v2 0/2] qapi/throttle: Fix qmp_block_set_io_throttle blocked for too long
@ 2025-03-21 7:09 zoudongjie via
2025-03-21 7:09 ` [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism zoudongjie via
2025-03-21 7:09 ` [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() zoudongjie via
0 siblings, 2 replies; 6+ messages in thread
From: zoudongjie via @ 2025-03-21 7:09 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, kwolf, fam, hreitz, alex.chen, chenjianfei3,
eric.fangyi, luolongmin, mujinsheng, qemu-block, qemu-stable,
renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang,
zhuyangyang14, zoudongjie
From: Zhu Yangyang <zhuyangyang14@huawei.com>
First of all, thanks to Stefan Hajnoczi for his suggestions,
I made the following changes in this patch:
1. Support 0 in BDRV_POLL_WHILE_TIMEOUT(), 0 means infinite.
2. Use uint64_t timeout_ns instead of int64 timeout to name variables.
3. Use timer_pending() to check for expiry instead of explicitly checking
against the deadline for BDRV_POLL_WHILE_TIMEOUT().
4. Add documentation for bdrv_drained_begin_timeout(), note that bdrv_drained_end()
must be called when -ETIMEDOUT is returned.
5. Add a timeout parameter to the qmp_block_set_io_throttle() instead of hardcoding
the timeout, and the default value is 0, mean an infinite timeout.
v1 patch link:
https://lore.kernel.org/qemu-devel/20250308101618.721954-1-zoudongjie@huawei.com/
Zhu Yangyang (2):
io/block: Refactoring the bdrv_drained_begin() function and implement
a timeout mechanism.
qapi/throttle: add timeout parameter for qmp_block_set_io_throttle()
block/block-backend.c | 14 ++++-
block/io.c | 58 +++++++++++++++++----
block/qapi-system.c | 10 +++-
include/block/aio-wait.h | 49 +++++++++++++++++
include/block/block-io.h | 22 +++++++-
include/system/block-backend-global-state.h | 1 +
qapi/block-core.json | 5 +-
util/aio-wait.c | 5 ++
8 files changed, 150 insertions(+), 14 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism. 2025-03-21 7:09 [PATCH v2 0/2] qapi/throttle: Fix qmp_block_set_io_throttle blocked for too long zoudongjie via @ 2025-03-21 7:09 ` zoudongjie via 2025-03-24 18:09 ` Stefan Hajnoczi 2025-03-21 7:09 ` [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() zoudongjie via 1 sibling, 1 reply; 6+ messages in thread From: zoudongjie via @ 2025-03-21 7:09 UTC (permalink / raw) To: qemu-devel Cc: stefanha, kwolf, fam, hreitz, alex.chen, chenjianfei3, eric.fangyi, luolongmin, mujinsheng, qemu-block, qemu-stable, renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang, zhuyangyang14, zoudongjie From: Zhu Yangyang <zhuyangyang14@huawei.com> The bdrv_drained_begin() function is a blocking function. In scenarios where network storage is used and network links fail, it may block for a long time. Therefore, we add a timeout parameter to control the duration of the block. Since bdrv_drained_begin() has been widely adopted, both bdrv_drained_begin() and bdrv_drained_begin_timeout() will be retained. Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com> --- block/io.c | 58 +++++++++++++++++++++++++++++++++------- include/block/aio-wait.h | 49 +++++++++++++++++++++++++++++++++ include/block/block-io.h | 22 ++++++++++++++- util/aio-wait.c | 5 ++++ 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/block/io.c b/block/io.c index 1ba8d1aeea..912b76c4a4 100644 --- a/block/io.c +++ b/block/io.c @@ -255,6 +255,8 @@ typedef struct { bool begin; bool poll; BdrvChild *parent; + uint64_t timeout_ns; + int ret; } BdrvCoDrainData; /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */ @@ -283,6 +285,10 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, return bdrv_drain_poll(bs, ignore_parent, false); } +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs, + BdrvChild *parent, + bool poll, + uint64_t timeout_ns); static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, bool poll); static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent); @@ -296,7 +302,9 @@ static void bdrv_co_drain_bh_cb(void *opaque) if (bs) { bdrv_dec_in_flight(bs); if (data->begin) { - bdrv_do_drained_begin(bs, data->parent, data->poll); + data->ret = bdrv_do_drained_begin_timeout(bs, data->parent, + data->poll, + data->timeout_ns); } else { assert(!data->poll); bdrv_do_drained_end(bs, data->parent); @@ -310,10 +318,11 @@ static void bdrv_co_drain_bh_cb(void *opaque) aio_co_wake(co); } -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, - bool begin, - BdrvChild *parent, - bool poll) +static int coroutine_fn bdrv_co_yield_to_drain_timeout(BlockDriverState *bs, + bool begin, + BdrvChild *parent, + bool poll, + uint64_t timeout_ns) { BdrvCoDrainData data; Coroutine *self = qemu_coroutine_self(); @@ -329,6 +338,8 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, .begin = begin, .parent = parent, .poll = poll, + .timeout_ns = timeout_ns, + .ret = 0 }; if (bs) { @@ -342,16 +353,27 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, /* If we are resumed from some other event (such as an aio completion or a * timer callback), it is a bug in the caller that should be fixed. */ assert(data.done); + return data.ret; } -static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, - bool poll) +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, + bool begin, + BdrvChild *parent, + bool poll) +{ + bdrv_co_yield_to_drain_timeout(bs, begin, parent, poll, 0); +} + +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs, + BdrvChild *parent, + bool poll, + uint64_t timeout_ns) { IO_OR_GS_CODE(); if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(bs, true, parent, poll); - return; + return bdrv_co_yield_to_drain_timeout(bs, true, parent, poll, + timeout_ns); } GLOBAL_STATE_CODE(); @@ -375,8 +397,17 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, * nodes. */ if (poll) { - BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent)); + return BDRV_POLL_WHILE_TIMEOUT(bs, + bdrv_drain_poll_top_level(bs, parent), + timeout_ns); } + return 0; +} + +static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, + bool poll) +{ + bdrv_do_drained_begin_timeout(bs, parent, poll, 0); } void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent) @@ -391,6 +422,13 @@ bdrv_drained_begin(BlockDriverState *bs) bdrv_do_drained_begin(bs, NULL, true); } +int coroutine_mixed_fn +bdrv_drained_begin_timeout(BlockDriverState *bs, uint64_t timeout_ns) +{ + IO_OR_GS_CODE(); + return bdrv_do_drained_begin_timeout(bs, NULL, true, timeout_ns); +} + /** * This function does not poll, nor must any of its recursively called * functions. diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h index cf5e8bde1c..24c31df620 100644 --- a/include/block/aio-wait.h +++ b/include/block/aio-wait.h @@ -99,6 +99,53 @@ extern AioWait global_aio_wait; qatomic_dec(&wait_->num_waiters); \ waited_; }) +/** + * AIO_WAIT_WHILE_TIMEOUT: + * + * Refer to the implementation of AIO_WAIT_WHILE_INTERNAL, + * the timeout parameter is added. + * @timeout_ns: maximum duration to wait, in nanoseconds, except the value + * is unsigned, 0 means infinite. + */ +#define AIO_WAIT_WHILE_TIMEOUT(ctx, cond, timeout_ns) ({ \ + int ret_ = 0; \ + uint64_t timeout_ = (timeout_ns); \ + AioWait *wait_ = &global_aio_wait; \ + AioContext *ctx_ = (ctx); \ + AioContext *current_ctx_ = NULL; \ + QEMUTimer timer_; \ + /* Increment wait_->num_waiters before evaluating cond. */ \ + qatomic_inc(&wait_->num_waiters); \ + /* Paired with smp_mb in aio_wait_kick(). */ \ + smp_mb__after_rmw(); \ + if (ctx_ && in_aio_context_home_thread(ctx_)) { \ + current_ctx_ = ctx_; \ + } else { \ + assert(qemu_get_current_aio_context() == \ + qemu_get_aio_context()); \ + current_ctx_ = qemu_get_aio_context(); \ + } \ + if (timeout_ > 0) { \ + timer_init_full(&timer_, ¤t_ctx_->tlg, \ + QEMU_CLOCK_REALTIME, \ + SCALE_NS, 0, aio_wait_timer_cb, NULL); \ + timer_mod_ns(&timer_, \ + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + \ + timeout_); \ + } \ + while ((cond)) { \ + aio_poll(current_ctx_, true); \ + if (timeout_ > 0 && !timer_pending(&timer_)) { \ + ret_ = -ETIMEDOUT; \ + break; \ + } \ + } \ + if (timeout_ > 0) { \ + timer_del(&timer_); \ + } \ + qatomic_dec(&wait_->num_waiters); \ + ret_; }) + #define AIO_WAIT_WHILE(ctx, cond) \ AIO_WAIT_WHILE_INTERNAL(ctx, cond) @@ -149,4 +196,6 @@ static inline bool in_aio_context_home_thread(AioContext *ctx) } } +void aio_wait_timer_cb(void *opaque); + #endif /* QEMU_AIO_WAIT_H */ diff --git a/include/block/block-io.h b/include/block/block-io.h index b49e0537dd..844e9cf350 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -354,6 +354,11 @@ bdrv_co_copy_range(BdrvChild *src, int64_t src_offset, AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ cond); }) +#define BDRV_POLL_WHILE_TIMEOUT(bs, cond, timeout_ns) ({ \ + BlockDriverState *bs_ = (bs); \ + AIO_WAIT_WHILE_TIMEOUT(bdrv_get_aio_context(bs_), \ + cond, timeout_ns); }) + void bdrv_drain(BlockDriverState *bs); int co_wrapper_mixed_bdrv_rdlock @@ -432,7 +437,22 @@ bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, void bdrv_drained_begin(BlockDriverState *bs); /** - * bdrv_do_drained_begin_quiesce: + * bdrv_drained_begin_timeout: + * + * Added timeout parameter for bdrv_drained_begin() to make a time limited. + * + * @timeout_ns: maximum duration to wait; 0 means infinite, equal to call + * bdrv_drained_begin(). + * + * Returns: 0 if succeeded; -ETIMEDOUT when a timeout occurs. + * + * Note: when the timeout fails, we've already begin aquiesced section, so we + * still need to call bdrv_drained_end() to end the quiescent section. + */ +int bdrv_drained_begin_timeout(BlockDriverState *bs, uint64_t timeout_ns); + +/** + * bdrv_do_drained_badegin_quiesce: * * Quiesces a BDS like bdrv_drained_begin(), but does not wait for already * running requests to complete. diff --git a/util/aio-wait.c b/util/aio-wait.c index b5336cf5fd..64c3714fb8 100644 --- a/util/aio-wait.c +++ b/util/aio-wait.c @@ -84,3 +84,8 @@ void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque) aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data); AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done); } + +void aio_wait_timer_cb(void *opaque) +{ + /* The point is to make AIO_WAIT_WHILE_TIMEOUT()'s aio_poll() return */ +} -- 2.33.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism. 2025-03-21 7:09 ` [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism zoudongjie via @ 2025-03-24 18:09 ` Stefan Hajnoczi 2025-03-26 7:30 ` zoudongjie via 0 siblings, 1 reply; 6+ messages in thread From: Stefan Hajnoczi @ 2025-03-24 18:09 UTC (permalink / raw) To: zoudongjie Cc: qemu-devel, kwolf, fam, hreitz, alex.chen, chenjianfei3, eric.fangyi, luolongmin, mujinsheng, qemu-block, qemu-stable, renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang, zhuyangyang14 [-- Attachment #1: Type: text/plain, Size: 12371 bytes --] On Fri, Mar 21, 2025 at 03:09:16PM +0800, zoudongjie wrote: > From: Zhu Yangyang <zhuyangyang14@huawei.com> > > The bdrv_drained_begin() function is a blocking function. In scenarios where network storage > is used and network links fail, it may block for a long time. > Therefore, we add a timeout parameter to control the duration of the block. > > Since bdrv_drained_begin() has been widely adopted, both bdrv_drained_begin() > and bdrv_drained_begin_timeout() will be retained. > > Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com> > --- > block/io.c | 58 +++++++++++++++++++++++++++++++++------- > include/block/aio-wait.h | 49 +++++++++++++++++++++++++++++++++ > include/block/block-io.h | 22 ++++++++++++++- > util/aio-wait.c | 5 ++++ > 4 files changed, 123 insertions(+), 11 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 1ba8d1aeea..912b76c4a4 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -255,6 +255,8 @@ typedef struct { > bool begin; > bool poll; > BdrvChild *parent; > + uint64_t timeout_ns; > + int ret; > } BdrvCoDrainData; > > /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */ > @@ -283,6 +285,10 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, > return bdrv_drain_poll(bs, ignore_parent, false); > } > > +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs, > + BdrvChild *parent, > + bool poll, > + uint64_t timeout_ns); > static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, > bool poll); > static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent); > @@ -296,7 +302,9 @@ static void bdrv_co_drain_bh_cb(void *opaque) > if (bs) { > bdrv_dec_in_flight(bs); > if (data->begin) { > - bdrv_do_drained_begin(bs, data->parent, data->poll); > + data->ret = bdrv_do_drained_begin_timeout(bs, data->parent, > + data->poll, > + data->timeout_ns); > } else { > assert(!data->poll); > bdrv_do_drained_end(bs, data->parent); > @@ -310,10 +318,11 @@ static void bdrv_co_drain_bh_cb(void *opaque) > aio_co_wake(co); > } > > -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > - bool begin, > - BdrvChild *parent, > - bool poll) > +static int coroutine_fn bdrv_co_yield_to_drain_timeout(BlockDriverState *bs, > + bool begin, > + BdrvChild *parent, > + bool poll, > + uint64_t timeout_ns) > { > BdrvCoDrainData data; > Coroutine *self = qemu_coroutine_self(); > @@ -329,6 +338,8 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > .begin = begin, > .parent = parent, > .poll = poll, > + .timeout_ns = timeout_ns, > + .ret = 0 > }; > > if (bs) { > @@ -342,16 +353,27 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > /* If we are resumed from some other event (such as an aio completion or a > * timer callback), it is a bug in the caller that should be fixed. */ > assert(data.done); > + return data.ret; > } > > -static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, > - bool poll) > +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > + bool begin, > + BdrvChild *parent, > + bool poll) > +{ > + bdrv_co_yield_to_drain_timeout(bs, begin, parent, poll, 0); > +} > + > +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs, > + BdrvChild *parent, > + bool poll, > + uint64_t timeout_ns) > { > IO_OR_GS_CODE(); > > if (qemu_in_coroutine()) { > - bdrv_co_yield_to_drain(bs, true, parent, poll); > - return; > + return bdrv_co_yield_to_drain_timeout(bs, true, parent, poll, > + timeout_ns); > } > > GLOBAL_STATE_CODE(); > @@ -375,8 +397,17 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, > * nodes. > */ > if (poll) { > - BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent)); > + return BDRV_POLL_WHILE_TIMEOUT(bs, > + bdrv_drain_poll_top_level(bs, parent), > + timeout_ns); > } > + return 0; > +} > + > +static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, > + bool poll) > +{ > + bdrv_do_drained_begin_timeout(bs, parent, poll, 0); > } > > void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent) > @@ -391,6 +422,13 @@ bdrv_drained_begin(BlockDriverState *bs) > bdrv_do_drained_begin(bs, NULL, true); > } > > +int coroutine_mixed_fn > +bdrv_drained_begin_timeout(BlockDriverState *bs, uint64_t timeout_ns) > +{ > + IO_OR_GS_CODE(); > + return bdrv_do_drained_begin_timeout(bs, NULL, true, timeout_ns); > +} > + > /** > * This function does not poll, nor must any of its recursively called > * functions. > diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h > index cf5e8bde1c..24c31df620 100644 > --- a/include/block/aio-wait.h > +++ b/include/block/aio-wait.h > @@ -99,6 +99,53 @@ extern AioWait global_aio_wait; > qatomic_dec(&wait_->num_waiters); \ > waited_; }) > > +/** > + * AIO_WAIT_WHILE_TIMEOUT: > + * > + * Refer to the implementation of AIO_WAIT_WHILE_INTERNAL, > + * the timeout parameter is added. > + * @timeout_ns: maximum duration to wait, in nanoseconds, except the value > + * is unsigned, 0 means infinite. > + */ > +#define AIO_WAIT_WHILE_TIMEOUT(ctx, cond, timeout_ns) ({ \ > + int ret_ = 0; \ > + uint64_t timeout_ = (timeout_ns); \ > + AioWait *wait_ = &global_aio_wait; \ > + AioContext *ctx_ = (ctx); \ > + AioContext *current_ctx_ = NULL; \ > + QEMUTimer timer_; \ > + /* Increment wait_->num_waiters before evaluating cond. */ \ > + qatomic_inc(&wait_->num_waiters); \ > + /* Paired with smp_mb in aio_wait_kick(). */ \ > + smp_mb__after_rmw(); \ > + if (ctx_ && in_aio_context_home_thread(ctx_)) { \ > + current_ctx_ = ctx_; \ > + } else { \ > + assert(qemu_get_current_aio_context() == \ > + qemu_get_aio_context()); \ > + current_ctx_ = qemu_get_aio_context(); \ > + } \ > + if (timeout_ > 0) { \ > + timer_init_full(&timer_, ¤t_ctx_->tlg, \ > + QEMU_CLOCK_REALTIME, \ > + SCALE_NS, 0, aio_wait_timer_cb, NULL); \ > + timer_mod_ns(&timer_, \ > + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + \ > + timeout_); \ > + } \ > + while ((cond)) { \ > + aio_poll(current_ctx_, true); \ > + if (timeout_ > 0 && !timer_pending(&timer_)) { \ > + ret_ = -ETIMEDOUT; \ > + break; \ > + } \ > + } \ > + if (timeout_ > 0) { \ > + timer_del(&timer_); \ > + } \ > + qatomic_dec(&wait_->num_waiters); \ > + ret_; }) > + > #define AIO_WAIT_WHILE(ctx, cond) \ > AIO_WAIT_WHILE_INTERNAL(ctx, cond) It would be nice to unify AIO_WAIT_WHILE_INTERNAL() and AIO_WAIT_WHILE_TIMEOUT(). I don't see any caller that uses the waited_ return value from AIO_WAIT_WHILE_INTERNAL(), so I think it's possible to replace AIO_WAIT_WHILE_INTERNAL() with AIO_WAIT_WHILE_TIMEOUT(.., ..., 0). > > @@ -149,4 +196,6 @@ static inline bool in_aio_context_home_thread(AioContext *ctx) > } > } > > +void aio_wait_timer_cb(void *opaque); > + > #endif /* QEMU_AIO_WAIT_H */ > diff --git a/include/block/block-io.h b/include/block/block-io.h > index b49e0537dd..844e9cf350 100644 > --- a/include/block/block-io.h > +++ b/include/block/block-io.h > @@ -354,6 +354,11 @@ bdrv_co_copy_range(BdrvChild *src, int64_t src_offset, > AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \ > cond); }) > > +#define BDRV_POLL_WHILE_TIMEOUT(bs, cond, timeout_ns) ({ \ > + BlockDriverState *bs_ = (bs); \ > + AIO_WAIT_WHILE_TIMEOUT(bdrv_get_aio_context(bs_), \ > + cond, timeout_ns); }) > + > void bdrv_drain(BlockDriverState *bs); > > int co_wrapper_mixed_bdrv_rdlock > @@ -432,7 +437,22 @@ bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, > void bdrv_drained_begin(BlockDriverState *bs); > > /** > - * bdrv_do_drained_begin_quiesce: > + * bdrv_drained_begin_timeout: > + * > + * Added timeout parameter for bdrv_drained_begin() to make a time limited. > + * > + * @timeout_ns: maximum duration to wait; 0 means infinite, equal to call > + * bdrv_drained_begin(). > + * > + * Returns: 0 if succeeded; -ETIMEDOUT when a timeout occurs. > + * > + * Note: when the timeout fails, we've already begin aquiesced section, so we > + * still need to call bdrv_drained_end() to end the quiescent section. > + */ > +int bdrv_drained_begin_timeout(BlockDriverState *bs, uint64_t timeout_ns); > + > +/** > + * bdrv_do_drained_badegin_quiesce: > * > * Quiesces a BDS like bdrv_drained_begin(), but does not wait for already > * running requests to complete. > diff --git a/util/aio-wait.c b/util/aio-wait.c > index b5336cf5fd..64c3714fb8 100644 > --- a/util/aio-wait.c > +++ b/util/aio-wait.c > @@ -84,3 +84,8 @@ void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque) > aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data); > AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done); > } > + > +void aio_wait_timer_cb(void *opaque) > +{ > + /* The point is to make AIO_WAIT_WHILE_TIMEOUT()'s aio_poll() return */ > +} > -- > 2.33.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism. 2025-03-24 18:09 ` Stefan Hajnoczi @ 2025-03-26 7:30 ` zoudongjie via 0 siblings, 0 replies; 6+ messages in thread From: zoudongjie via @ 2025-03-26 7:30 UTC (permalink / raw) To: stefanha Cc: alex.chen, chenjianfei3, eric.fangyi, fam, hreitz, kwolf, luolongmin, mujinsheng, qemu-block, qemu-devel, qemu-stable, renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang, zhuyangyang14, zoudongjie From: Zhu Yangyang <zhuyangyang14@huawei.com> On Mon, 24 Mar 2025 14:09:13 -0400, Stefan Hajnoczi wrote: > On Fri, Mar 21, 2025 at 03:09:16PM +0800, zoudongjie wrote: > > From: Zhu Yangyang <zhuyangyang14@huawei.com> > > > > The bdrv_drained_begin() function is a blocking function. In scenarios where network storage > > is used and network links fail, it may block for a long time. > > Therefore, we add a timeout parameter to control the duration of the block. > > > > Since bdrv_drained_begin() has been widely adopted, both bdrv_drained_begin() > > and bdrv_drained_begin_timeout() will be retained. > ... > > > +/** > > + * AIO_WAIT_WHILE_TIMEOUT: > > + * > > + * Refer to the implementation of AIO_WAIT_WHILE_INTERNAL, > > + * the timeout parameter is added. > > + * @timeout_ns: maximum duration to wait, in nanoseconds, except the value > > + * is unsigned, 0 means infinite. > > + */ > > +#define AIO_WAIT_WHILE_TIMEOUT(ctx, cond, timeout_ns) ({ \ > > + int ret_ = 0; \ > > + uint64_t timeout_ = (timeout_ns); \ > > + AioWait *wait_ = &global_aio_wait; \ > > + AioContext *ctx_ = (ctx); \ > > + AioContext *current_ctx_ = NULL; \ > > + QEMUTimer timer_; \ > > + /* Increment wait_->num_waiters before evaluating cond. */ \ > > + qatomic_inc(&wait_->num_waiters); \ > > + /* Paired with smp_mb in aio_wait_kick(). */ \ > > + smp_mb__after_rmw(); \ > > + if (ctx_ && in_aio_context_home_thread(ctx_)) { \ > > + current_ctx_ = ctx_; \ > > + } else { \ > > + assert(qemu_get_current_aio_context() == \ > > + qemu_get_aio_context()); \ > > + current_ctx_ = qemu_get_aio_context(); \ > > + } \ > > + if (timeout_ > 0) { \ > > + timer_init_full(&timer_, ¤t_ctx_->tlg, \ > > + QEMU_CLOCK_REALTIME, \ > > + SCALE_NS, 0, aio_wait_timer_cb, NULL); \ > > + timer_mod_ns(&timer_, \ > > + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + \ > > + timeout_); \ > > + } \ > > + while ((cond)) { \ > > + aio_poll(current_ctx_, true); \ > > + if (timeout_ > 0 && !timer_pending(&timer_)) { \ > > + ret_ = -ETIMEDOUT; \ > > + break; \ > > + } \ > > + } \ > > + if (timeout_ > 0) { \ > > + timer_del(&timer_); \ > > + } \ > > + qatomic_dec(&wait_->num_waiters); \ > > + ret_; }) > > + > > #define AIO_WAIT_WHILE(ctx, cond) \ > > AIO_WAIT_WHILE_INTERNAL(ctx, cond) > > It would be nice to unify AIO_WAIT_WHILE_INTERNAL() and > AIO_WAIT_WHILE_TIMEOUT(). I don't see any caller that uses the waited_ > return value from AIO_WAIT_WHILE_INTERNAL(), so I think it's possible to > replace AIO_WAIT_WHILE_INTERNAL() with AIO_WAIT_WHILE_TIMEOUT(.., ..., 0). > Yes, there is no difference between AIO_WAIT_WHILE_INTERNAL() and AIO_WAIT_WHILE_TIMEOUT() except the return value. It's right to keep the code uniform. I will modify it in the next version. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() 2025-03-21 7:09 [PATCH v2 0/2] qapi/throttle: Fix qmp_block_set_io_throttle blocked for too long zoudongjie via 2025-03-21 7:09 ` [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism zoudongjie via @ 2025-03-21 7:09 ` zoudongjie via 2025-03-24 18:12 ` Stefan Hajnoczi 1 sibling, 1 reply; 6+ messages in thread From: zoudongjie via @ 2025-03-21 7:09 UTC (permalink / raw) To: qemu-devel Cc: stefanha, kwolf, fam, hreitz, alex.chen, chenjianfei3, eric.fangyi, luolongmin, mujinsheng, qemu-block, qemu-stable, renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang, zhuyangyang14, zoudongjie From: Zhu Yangyang <zhuyangyang14@huawei.com> Calling qmp_block_set_io_throttle() will be blocked for a long time when a network disk is configured and the network failure is just about to occur. Therefore, we add a timeout parameter for qmp_block_set_io_throttle to control its execution duration. The default value of timeout is 0, that is infinite wait, consistent with previous behavior. Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com> --- block/block-backend.c | 14 +++++++++++++- block/qapi-system.c | 10 +++++++++- include/system/block-backend-global-state.h | 1 + qapi/block-core.json | 5 ++++- 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index a402db13f2..fc1554ea55 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2679,20 +2679,32 @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg) } void blk_io_limits_disable(BlockBackend *blk) +{ + blk_io_limits_disable_timeout(blk, 0); +} + +int blk_io_limits_disable_timeout(BlockBackend *blk, uint64_t timeout_ns) { 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_ns); + 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..3b7ba0959c 100644 --- a/block/qapi-system.c +++ b/block/qapi-system.c @@ -423,6 +423,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) ThrottleConfig cfg; BlockDriverState *bs; BlockBackend *blk; + uint64_t timeout_ns; blk = qmp_get_blk(arg->device, arg->id, errp); if (!blk) { @@ -444,6 +445,10 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; + timeout_ns = 0; /* 0 means infinite */ + if (arg->has_timeout) { + timeout_ns = arg->timeout * NANOSECONDS_PER_SECOND; + } if (arg->has_bps_max) { cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; } @@ -502,7 +507,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, timeout_ns) < 0) { + 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 35b5e8380b..8a2a6cbda4 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -110,6 +110,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, uint64_t timeout_ns); 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); diff --git a/qapi/block-core.json b/qapi/block-core.json index b1937780e1..88ef593efd 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2626,6 +2626,9 @@ # # @group: throttle group name (Since 2.4) # +# @timeout: In seconds. Timeout for block_set_io_throttle, +# 0 means no limit. Defaults to 0. (Since 10.0) +# # Features: # # @deprecated: Member @device is deprecated. Use @id instead. @@ -2642,7 +2645,7 @@ '*bps_max_length': 'int', '*bps_rd_max_length': 'int', '*bps_wr_max_length': 'int', '*iops_max_length': 'int', '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int', - '*iops_size': 'int', '*group': 'str' } } + '*iops_size': 'int', '*group': 'str', '*timeout': 'uint32'} } ## # @ThrottleLimits: -- 2.33.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() 2025-03-21 7:09 ` [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() zoudongjie via @ 2025-03-24 18:12 ` Stefan Hajnoczi 0 siblings, 0 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2025-03-24 18:12 UTC (permalink / raw) To: zoudongjie Cc: qemu-devel, kwolf, fam, hreitz, alex.chen, chenjianfei3, eric.fangyi, luolongmin, mujinsheng, qemu-block, qemu-stable, renxuming, suxiaodong1, wangjian161, wangyan122, yebiaoxiang, zhuyangyang14 [-- Attachment #1: Type: text/plain, Size: 912 bytes --] On Fri, Mar 21, 2025 at 03:09:17PM +0800, zoudongjie wrote: > From: Zhu Yangyang <zhuyangyang14@huawei.com> > > Calling qmp_block_set_io_throttle() will be blocked for a long time > when a network disk is configured and the network failure is just about > to occur. > > Therefore, we add a timeout parameter for qmp_block_set_io_throttle to control > its execution duration. > > The default value of timeout is 0, that is infinite wait, consistent with > previous behavior. > > Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com> > --- > block/block-backend.c | 14 +++++++++++++- > block/qapi-system.c | 10 +++++++++- > include/system/block-backend-global-state.h | 1 + > qapi/block-core.json | 5 ++++- > 4 files changed, 27 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-26 7:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-21 7:09 [PATCH v2 0/2] qapi/throttle: Fix qmp_block_set_io_throttle blocked for too long zoudongjie via 2025-03-21 7:09 ` [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism zoudongjie via 2025-03-24 18:09 ` Stefan Hajnoczi 2025-03-26 7:30 ` zoudongjie via 2025-03-21 7:09 ` [PATCH v2 2/2] qapi/throttle: add timeout parameter for qmp_block_set_io_throttle() zoudongjie via 2025-03-24 18:12 ` Stefan Hajnoczi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.