On Thu, Apr 17, 2025 at 9:31 PM Zizhi Wo <wozizhi@xxxxxxxxxxxxxxx> wrote:
>
> From: Zizhi Wo <wozizhi@xxxxxxxxxx>
>
> In commit 6cc477c36875 ("blk-throttle: carry over directly"), the carryover
> bytes/ios was be carried to [bytes/io]_disp. However, its update mechanism
> has some issues.
>
> In __tg_update_carryover(), we calculate "bytes" and "ios" to represent the
> carryover, but the computation when updating [bytes/io]_disp is incorrect.
> And if the sq->nr_queued is empty, we may not update tg->[bytes/io]_disp to
> 0 in tg_update_carryover(). We should set it to 0 in non carryover case.
> This patch fixes the issue.
>
> Fixes: 6cc477c36875 ("blk-throttle: carry over directly")
> Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
> ---
> block/blk-throttle.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 91dab43c65ab..8ac8db116520 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -644,6 +644,18 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
> u64 bps_limit = tg_bps_limit(tg, rw);
> u32 iops_limit = tg_iops_limit(tg, rw);
>
> + /*
> + * If the queue is empty, carryover handling is not needed. In such cases,
> + * tg->[bytes/io]_disp should be reset to 0 to avoid impacting the dispatch
> + * of subsequent bios. The same handling applies when the previous BPS/IOPS
> + * limit was set to max.
> + */
> + if (tg->service_queue.nr_queued[rw] == 0) {
> + tg->bytes_disp[rw] = 0;
> + tg->io_disp[rw] = 0;
> + return;
> + }
> +
> /*
> * If config is updated while bios are still throttled, calculate and
> * accumulate how many bytes/ios are waited across changes. And
> @@ -656,8 +668,8 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
> if (iops_limit != UINT_MAX)
> *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) -
> tg->io_disp[rw];
> - tg->bytes_disp[rw] -= *bytes;
> - tg->io_disp[rw] -= *ios;
> + tg->bytes_disp[rw] = -*bytes;
> + tg->io_disp[rw] = -*ios;
> }
>
> static void tg_update_carryover(struct throtl_grp *tg)
> @@ -665,10 +677,8 @@ static void tg_update_carryover(struct throtl_grp *tg)
> long long bytes[2] = {0};
> int ios[2] = {0};
>
> - if (tg->service_queue.nr_queued[READ])
> - __tg_update_carryover(tg, READ, &bytes[READ], &ios[READ]);
> - if (tg->service_queue.nr_queued[WRITE])
> - __tg_update_carryover(tg, WRITE, &bytes[WRITE], &ios[WRITE]);
> + __tg_update_carryover(tg, READ, &bytes[READ], &ios[READ]);
> + __tg_update_carryover(tg, WRITE, &bytes[WRITE], &ios[WRITE]);
>
> /* see comments in struct throtl_grp for meaning of these fields. */
> throtl_log(&tg->service_queue, "%s: %lld %lld %d %d\n", __func__,
Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
