diff mbox series

[V2,1/3] blk-throttle: Fix wrong tg->[bytes/io]_disp update in __tg_update_carryover()

Message ID 20250417132054.2866409-2-wozizhi@huaweicloud.com (mailing list archive)
State New
Headers show
Series blk-throttle: Some bugfixes and modifications | expand

Commit Message

Zizhi Wo April 17, 2025, 1:20 p.m. UTC 
From: Zizhi Wo <wozizhi@huawei.com>

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@huawei.com>
---
 block/blk-throttle.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Ming Lei April 17, 2025, 1:49 p.m. UTC | #1 
On Thu, Apr 17, 2025 at 9:31 PM Zizhi Wo <wozizhi@huaweicloud.com> wrote:
>
> From: Zizhi Wo <wozizhi@huawei.com>
>
> 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@huawei.com>
> ---
>  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@redhat.com>
Yu Kuai April 18, 2025, 1:19 a.m. UTC | #2 
在 2025/04/17 21:20, Zizhi Wo 写道:
> From: Zizhi Wo <wozizhi@huawei.com>
> 
> 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@huawei.com>
> ---
>   block/blk-throttle.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
LGTM

Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> 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__,
>
diff mbox series

Patch

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__,