[PATCH 0/3] blk-throttle: Some bugfixes and modifications

[PATCH 0/3] blk-throttle: Some bugfixes and modifications

[Zizhi Wo]

The patchset mainly address the update logic of tg->[bytes/io]_disp and add
related overflow checks. The last patch also simplified the process.

Zizhi Wo (3):
  blk-throttle: Fix wrong tg->[bytes/io]_disp update in
    __tg_update_carryover()
  blk-throttle: Delete unnecessary carryover-related fields from
    throtl_grp
  blk-throttle: Add an additional overflow check to the call
    calculate_bytes/io_allowed

 block/blk-throttle.c | 131 +++++++++++++++++++++++++++++++++----------
 block/blk-throttle.h |  19 +++----
 2 files changed, 108 insertions(+), 42 deletions(-)

-- 
2.46.1

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

[Zizhi Wo]

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.
This patch fixes the issue.

And if the bps/iops limit was previously set to max and later changed to a
smaller value, we may not update tg->[bytes/io]_disp to 0 in
tg_update_carryover(). Relying solely on throtl_trim_slice() is not
sufficient, which can lead to subsequent bio dispatches not behaving as
expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case.
The same handling applies when nr_queued is 0.

Fixes: 6cc477c36875 ("blk-throttle: carry over directly")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 block/blk-throttle.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 91dab43c65ab..df9825eb83be 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -644,20 +644,39 @@ 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
 	 * carryover_bytes/ios will be used to calculate new wait time under new
 	 * configuration.
 	 */
-	if (bps_limit != U64_MAX)
+	if (bps_limit != U64_MAX) {
 		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
 			tg->bytes_disp[rw];
-	if (iops_limit != UINT_MAX)
+		tg->bytes_disp[rw] = -*bytes;
+	} else {
+		tg->bytes_disp[rw] = 0;
+	}
+
+	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->io_disp[rw] = -*ios;
+	} else {
+		tg->io_disp[rw] = 0;
+	}
 }
 
 static void tg_update_carryover(struct throtl_grp *tg)
@@ -665,10 +684,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__,
-- 
2.46.1

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

[Ming Lei]

On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote:
> 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.
> This patch fixes the issue.
> 
> And if the bps/iops limit was previously set to max and later changed to a
> smaller value, we may not update tg->[bytes/io]_disp to 0 in
> tg_update_carryover(). Relying solely on throtl_trim_slice() is not
> sufficient, which can lead to subsequent bio dispatches not behaving as
> expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case.
> The same handling applies when nr_queued is 0.
> 
> Fixes: 6cc477c36875 ("blk-throttle: carry over directly")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  block/blk-throttle.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 91dab43c65ab..df9825eb83be 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -644,20 +644,39 @@ 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
>  	 * carryover_bytes/ios will be used to calculate new wait time under new
>  	 * configuration.
>  	 */
> -	if (bps_limit != U64_MAX)
> +	if (bps_limit != U64_MAX) {
>  		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>  			tg->bytes_disp[rw];
> -	if (iops_limit != UINT_MAX)
> +		tg->bytes_disp[rw] = -*bytes;
> +	} else {
> +		tg->bytes_disp[rw] = 0;
> +	}

It should be fine to do	'tg->bytes_disp[rw] = -*bytes;' directly
because `*bytes` is initialized as zero.

> +
> +	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->io_disp[rw] = -*ios;
> +	} else {
> +		tg->io_disp[rw] = 0;
> +	}

Same with above.

Otherwise, this patch looks fine.


thanks,
Ming

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

[Zizhi Wo]

在 2025/4/17 10:27, Ming Lei 写道:
> On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote:
>> 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.
>> This patch fixes the issue.
>>
>> And if the bps/iops limit was previously set to max and later changed to a
>> smaller value, we may not update tg->[bytes/io]_disp to 0 in
>> tg_update_carryover(). Relying solely on throtl_trim_slice() is not
>> sufficient, which can lead to subsequent bio dispatches not behaving as
>> expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case.
>> The same handling applies when nr_queued is 0.
>>
>> Fixes: 6cc477c36875 ("blk-throttle: carry over directly")
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   block/blk-throttle.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 91dab43c65ab..df9825eb83be 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -644,20 +644,39 @@ 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
>>   	 * carryover_bytes/ios will be used to calculate new wait time under new
>>   	 * configuration.
>>   	 */
>> -	if (bps_limit != U64_MAX)
>> +	if (bps_limit != U64_MAX) {
>>   		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>>   			tg->bytes_disp[rw];
>> -	if (iops_limit != UINT_MAX)
>> +		tg->bytes_disp[rw] = -*bytes;
>> +	} else {
>> +		tg->bytes_disp[rw] = 0;
>> +	}
> 
> It should be fine to do	'tg->bytes_disp[rw] = -*bytes;' directly
> because `*bytes` is initialized as zero.

Indeed, I didn't notice that the incoming bytes/io is initialized to 0.

Thanks,
Zizhi Wo

> 
>> +
>> +	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->io_disp[rw] = -*ios;
>> +	} else {
>> +		tg->io_disp[rw] = 0;
>> +	}
> 
> Same with above.
> 
> Otherwise, this patch looks fine.
> 
> 
> thanks,
> Ming
> 
> 

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

[Yu Kuai]

Hi,

在 2025/04/17 10:27, Ming Lei 写道:
> On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote:
>> 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.
>> This patch fixes the issue.
>>
>> And if the bps/iops limit was previously set to max and later changed to a
>> smaller value, we may not update tg->[bytes/io]_disp to 0 in
>> tg_update_carryover(). Relying solely on throtl_trim_slice() is not
>> sufficient, which can lead to subsequent bio dispatches not behaving as
>> expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case.
>> The same handling applies when nr_queued is 0.
>>
>> Fixes: 6cc477c36875 ("blk-throttle: carry over directly")
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   block/blk-throttle.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 91dab43c65ab..df9825eb83be 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -644,20 +644,39 @@ 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
>>   	 * carryover_bytes/ios will be used to calculate new wait time under new
>>   	 * configuration.
>>   	 */
>> -	if (bps_limit != U64_MAX)
>> +	if (bps_limit != U64_MAX) {
>>   		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>>   			tg->bytes_disp[rw];
>> -	if (iops_limit != UINT_MAX)
>> +		tg->bytes_disp[rw] = -*bytes;
>> +	} else {
>> +		tg->bytes_disp[rw] = 0;
>> +	}
> 
> It should be fine to do	'tg->bytes_disp[rw] = -*bytes;' directly
> because `*bytes` is initialized as zero.

It took me a while to understand, I think you mean

if ()
  *bytes =xxx;

tg->bytes_disp[rw] = -*bytes;

I'm good with or without this change.

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

> 
>> +
>> +	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->io_disp[rw] = -*ios;
>> +	} else {
>> +		tg->io_disp[rw] = 0;
>> +	}
> 
> Same with above.
> 
> Otherwise, this patch looks fine.
> 
> 
> thanks,
> Ming
> 
> 
> .
> 

[PATCH 2/3] blk-throttle: Delete unnecessary carryover-related fields from throtl_grp

[Zizhi Wo]

We no longer need carryover_[bytes/ios] in tg, so it is removed. The
related comments about carryover in tg are also merged into
[bytes/io]_disp, and modify other related comments.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 block/blk-throttle.c |  8 ++++----
 block/blk-throttle.h | 19 ++++++++-----------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index df9825eb83be..4e4609dce85d 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -658,9 +658,9 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
 
 	/*
 	 * If config is updated while bios are still throttled, calculate and
-	 * accumulate how many bytes/ios are waited across changes. And
-	 * carryover_bytes/ios will be used to calculate new wait time under new
-	 * configuration.
+	 * accumulate how many bytes/ios are waited across changes. And use the
+	 * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which
+	 * will be used to calculate new wait time under new configuration.
 	 */
 	if (bps_limit != U64_MAX) {
 		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
@@ -687,7 +687,7 @@ static void tg_update_carryover(struct throtl_grp *tg)
 	__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. */
+	/* see comments in struct throtl_grp for meaning of carryover. */
 	throtl_log(&tg->service_queue, "%s: %lld %lld %d %d\n", __func__,
 		   bytes[READ], bytes[WRITE], ios[READ], ios[WRITE]);
 }
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 7964cc041e06..8bd16535302c 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -101,19 +101,16 @@ struct throtl_grp {
 	/* IOPS limits */
 	unsigned int iops[2];
 
-	/* Number of bytes dispatched in current slice */
-	int64_t bytes_disp[2];
-	/* Number of bio's dispatched in current slice */
-	int io_disp[2];
-
 	/*
-	 * The following two fields are updated when new configuration is
-	 * submitted while some bios are still throttled, they record how many
-	 * bytes/ios are waited already in previous configuration, and they will
-	 * be used to calculate wait time under new configuration.
+	 * Number of bytes/bio's dispatched in current slice.
+	 * When new configuration is submitted while some bios are still throttled,
+	 * first calculate the carryover: the amount of bytes/IOs already waited
+	 * under the previous configuration. Then, [bytes/io]_disp are represented
+	 * as the negative of the carryover, and they will be used to calculate the
+	 * wait time under the new configuration.
 	 */
-	long long carryover_bytes[2];
-	int carryover_ios[2];
+	int64_t bytes_disp[2];
+	int io_disp[2];
 
 	unsigned long last_check_time;
 
-- 
2.46.1

Re: [PATCH 2/3] blk-throttle: Delete unnecessary carryover-related fields from throtl_grp

[Ming Lei]

On Thu, Apr 17, 2025 at 09:50:32AM +0800, Zizhi Wo wrote:
> We no longer need carryover_[bytes/ios] in tg, so it is removed. The
> related comments about carryover in tg are also merged into
> [bytes/io]_disp, and modify other related comments.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH 2/3] blk-throttle: Delete unnecessary carryover-related fields from throtl_grp

[Yu Kuai]

在 2025/04/17 9:50, Zizhi Wo 写道:
> We no longer need carryover_[bytes/ios] in tg, so it is removed. The
> related comments about carryover in tg are also merged into
> [bytes/io]_disp, and modify other related comments.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>   block/blk-throttle.c |  8 ++++----
>   block/blk-throttle.h | 19 ++++++++-----------
>   2 files changed, 12 insertions(+), 15 deletions(-)
> 

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

> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index df9825eb83be..4e4609dce85d 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -658,9 +658,9 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
>   
>   	/*
>   	 * If config is updated while bios are still throttled, calculate and
> -	 * accumulate how many bytes/ios are waited across changes. And
> -	 * carryover_bytes/ios will be used to calculate new wait time under new
> -	 * configuration.
> +	 * accumulate how many bytes/ios are waited across changes. And use the
> +	 * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which
> +	 * will be used to calculate new wait time under new configuration.
>   	 */
>   	if (bps_limit != U64_MAX) {
>   		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> @@ -687,7 +687,7 @@ static void tg_update_carryover(struct throtl_grp *tg)
>   	__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. */
> +	/* see comments in struct throtl_grp for meaning of carryover. */
>   	throtl_log(&tg->service_queue, "%s: %lld %lld %d %d\n", __func__,
>   		   bytes[READ], bytes[WRITE], ios[READ], ios[WRITE]);
>   }
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 7964cc041e06..8bd16535302c 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -101,19 +101,16 @@ struct throtl_grp {
>   	/* IOPS limits */
>   	unsigned int iops[2];
>   
> -	/* Number of bytes dispatched in current slice */
> -	int64_t bytes_disp[2];
> -	/* Number of bio's dispatched in current slice */
> -	int io_disp[2];
> -
>   	/*
> -	 * The following two fields are updated when new configuration is
> -	 * submitted while some bios are still throttled, they record how many
> -	 * bytes/ios are waited already in previous configuration, and they will
> -	 * be used to calculate wait time under new configuration.
> +	 * Number of bytes/bio's dispatched in current slice.
> +	 * When new configuration is submitted while some bios are still throttled,
> +	 * first calculate the carryover: the amount of bytes/IOs already waited
> +	 * under the previous configuration. Then, [bytes/io]_disp are represented
> +	 * as the negative of the carryover, and they will be used to calculate the
> +	 * wait time under the new configuration.
>   	 */
> -	long long carryover_bytes[2];
> -	int carryover_ios[2];
> +	int64_t bytes_disp[2];
> +	int io_disp[2];
>   
>   	unsigned long last_check_time;
>   
> 

[PATCH 3/3] blk-throttle: Add an additional overflow check to the call calculate_bytes/io_allowed

[Zizhi Wo]

Now the tg->[bytes/io]_disp type is signed, and calculate_bytes/io_allowed
return type is unsigned. Even if the bps/iops limit is not set to max, the
return value of the function may still exceed INT_MAX or LLONG_MAX, which
can cause overflow in outer variables. In such cases, we can add additional
checks accordingly.

And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's
no need to call calculate_bytes/io_allowed(). Introduces the helper
functions throtl_trim_bps/iops to simplifies the process. For cases when
the calculated trim value exceeds INT_MAX (causing an overflow), we reset
tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp because
it is the size that is actually trimmed.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 4e4609dce85d..cfb9c47d1af7 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
 	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
 }
 
+static long long throtl_trim_bps(struct throtl_grp *tg, bool rw,
+				 unsigned long time_elapsed)
+{
+	u64 bps_limit = tg_bps_limit(tg, rw);
+	long long bytes_trim;
+
+	if (bps_limit == U64_MAX)
+		return 0;
+
+	/* Need to consider the case of bytes_allowed overflow. */
+	bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed);
+	if (bytes_trim <= 0) {
+		bytes_trim = tg->bytes_disp[rw];
+		tg->bytes_disp[rw] = 0;
+		goto out;
+	}
+
+	if (tg->bytes_disp[rw] >= bytes_trim)
+		tg->bytes_disp[rw] -= bytes_trim;
+	else
+		tg->bytes_disp[rw] = 0;
+out:
+	return bytes_trim;
+}
+
+static int throtl_trim_iops(struct throtl_grp *tg, bool rw,
+			    unsigned long time_elapsed)
+{
+	u32 iops_limit = tg_iops_limit(tg, rw);
+	int io_trim;
+
+	if (iops_limit == UINT_MAX)
+		return 0;
+
+	/* Need to consider the case of io_allowed overflow. */
+	io_trim = calculate_io_allowed(iops_limit, time_elapsed);
+	if (io_trim <= 0) {
+		io_trim = tg->io_disp[rw];
+		tg->io_disp[rw] = 0;
+		goto out;
+	}
+
+	if (tg->io_disp[rw] >= io_trim)
+		tg->io_disp[rw] -= io_trim;
+	else
+		tg->io_disp[rw] = 0;
+out:
+	return io_trim;
+}
+
 /* Trim the used slices and adjust slice start accordingly */
 static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 {
@@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 	 * one extra slice is preserved for deviation.
 	 */
 	time_elapsed -= tg->td->throtl_slice;
-	bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw),
-					     time_elapsed);
-	io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed);
-	if (bytes_trim <= 0 && io_trim <= 0)
+	bytes_trim = throtl_trim_bps(tg, rw, time_elapsed);
+	io_trim = throtl_trim_iops(tg, rw, time_elapsed);
+	if (!bytes_trim && !io_trim)
 		return;
 
-	if ((long long)tg->bytes_disp[rw] >= bytes_trim)
-		tg->bytes_disp[rw] -= bytes_trim;
-	else
-		tg->bytes_disp[rw] = 0;
-
-	if ((int)tg->io_disp[rw] >= io_trim)
-		tg->io_disp[rw] -= io_trim;
-	else
-		tg->io_disp[rw] = 0;
-
 	tg->slice_start[rw] += time_elapsed;
 
 	throtl_log(&tg->service_queue,
@@ -643,6 +682,8 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
 	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
 	u64 bps_limit = tg_bps_limit(tg, rw);
 	u32 iops_limit = tg_iops_limit(tg, rw);
+	long long bytes_allowed;
+	int io_allowed;
 
 	/*
 	 * If the queue is empty, carryover handling is not needed. In such cases,
@@ -661,19 +702,28 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
 	 * accumulate how many bytes/ios are waited across changes. And use the
 	 * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which
 	 * will be used to calculate new wait time under new configuration.
+	 * And we need to consider the case of bytes/io_allowed overflow.
 	 */
 	if (bps_limit != U64_MAX) {
-		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
-			tg->bytes_disp[rw];
-		tg->bytes_disp[rw] = -*bytes;
+		bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed);
+		if (bytes_allowed < 0) {
+			tg->bytes_disp[rw] = 0;
+		} else {
+			*bytes = bytes_allowed - tg->bytes_disp[rw];
+			tg->bytes_disp[rw] = -*bytes;
+		}
 	} else {
 		tg->bytes_disp[rw] = 0;
 	}
 
 	if (iops_limit != UINT_MAX) {
-		*ios = calculate_io_allowed(iops_limit, jiffy_elapsed) -
-			tg->io_disp[rw];
-		tg->io_disp[rw] = -*ios;
+		io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed);
+		if (io_allowed < 0) {
+			tg->io_disp[rw] = 0;
+		} else {
+			*ios = io_allowed - tg->io_disp[rw];
+			tg->io_disp[rw] = -*ios;
+		}
 	} else {
 		tg->io_disp[rw] = 0;
 	}
@@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio,
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
 	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
-	if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
+	/* Need to consider the case of bytes_allowed overflow. */
+	if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
+	    || bytes_allowed < 0)
 		return 0;
 
 	/* Calc approx time to dispatch */
-- 
2.46.1

Re: [PATCH 3/3] blk-throttle: Add an additional overflow check to the call calculate_bytes/io_allowed

[Yu Kuai]

Hi,

在 2025/04/17 9:50, Zizhi Wo 写道:
> Now the tg->[bytes/io]_disp type is signed, and calculate_bytes/io_allowed
> return type is unsigned. Even if the bps/iops limit is not set to max, the
> return value of the function may still exceed INT_MAX or LLONG_MAX, which
> can cause overflow in outer variables. In such cases, we can add additional
> checks accordingly.
> 
> And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's
> no need to call calculate_bytes/io_allowed(). Introduces the helper
> functions throtl_trim_bps/iops to simplifies the process. For cases when
> the calculated trim value exceeds INT_MAX (causing an overflow), we reset
> tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp because
> it is the size that is actually trimmed.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>   block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++----------
>   1 file changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 4e4609dce85d..cfb9c47d1af7 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
>   	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
>   }
>   
> +static long long throtl_trim_bps(struct throtl_grp *tg, bool rw,
> +				 unsigned long time_elapsed)
> +{
> +	u64 bps_limit = tg_bps_limit(tg, rw);
> +	long long bytes_trim;
> +
> +	if (bps_limit == U64_MAX)
> +		return 0;
> +
> +	/* Need to consider the case of bytes_allowed overflow. */
> +	bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed);
> +	if (bytes_trim <= 0) {
> +		bytes_trim = tg->bytes_disp[rw];
> +		tg->bytes_disp[rw] = 0;
> +		goto out;
> +	}
> +
> +	if (tg->bytes_disp[rw] >= bytes_trim)
> +		tg->bytes_disp[rw] -= bytes_trim;
> +	else
> +		tg->bytes_disp[rw] = 0;

Perhaps

if (bytes_trim <=0 || tg->bytes_disp[rw] < bytes_trim) {
	bytes_trim = tg->bytes_disp[rw];
	tg->bytes_disp[rw] = 0;
} else {
	tg->bytes_disp[rw] -= bytes_trim;
}

And you don't need the out tag.
> +out:
> +	return bytes_trim;
> +}
> +
> +static int throtl_trim_iops(struct throtl_grp *tg, bool rw,
> +			    unsigned long time_elapsed)
> +{
> +	u32 iops_limit = tg_iops_limit(tg, rw);
> +	int io_trim;
> +
> +	if (iops_limit == UINT_MAX)
> +		return 0;
> +
> +	/* Need to consider the case of io_allowed overflow. */
> +	io_trim = calculate_io_allowed(iops_limit, time_elapsed);
> +	if (io_trim <= 0) {
> +		io_trim = tg->io_disp[rw];
> +		tg->io_disp[rw] = 0;
> +		goto out;
> +	}
> +
> +	if (tg->io_disp[rw] >= io_trim)
> +		tg->io_disp[rw] -= io_trim;
> +	else
> +		tg->io_disp[rw] = 0;
> +out:
> +	return io_trim;

Same as above.

Thanks,
Kuai

> +}
> +
>   /* Trim the used slices and adjust slice start accordingly */
>   static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
>   {
> @@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
>   	 * one extra slice is preserved for deviation.
>   	 */
>   	time_elapsed -= tg->td->throtl_slice;
> -	bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw),
> -					     time_elapsed);
> -	io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed);
> -	if (bytes_trim <= 0 && io_trim <= 0)
> +	bytes_trim = throtl_trim_bps(tg, rw, time_elapsed);
> +	io_trim = throtl_trim_iops(tg, rw, time_elapsed);
> +	if (!bytes_trim && !io_trim)
>   		return;
>   
> -	if ((long long)tg->bytes_disp[rw] >= bytes_trim)
> -		tg->bytes_disp[rw] -= bytes_trim;
> -	else
> -		tg->bytes_disp[rw] = 0;
> -
> -	if ((int)tg->io_disp[rw] >= io_trim)
> -		tg->io_disp[rw] -= io_trim;
> -	else
> -		tg->io_disp[rw] = 0;
> -
>   	tg->slice_start[rw] += time_elapsed;
>   
>   	throtl_log(&tg->service_queue,
> @@ -643,6 +682,8 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
>   	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
>   	u64 bps_limit = tg_bps_limit(tg, rw);
>   	u32 iops_limit = tg_iops_limit(tg, rw);
> +	long long bytes_allowed;
> +	int io_allowed;
>   
>   	/*
>   	 * If the queue is empty, carryover handling is not needed. In such cases,
> @@ -661,19 +702,28 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
>   	 * accumulate how many bytes/ios are waited across changes. And use the
>   	 * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which
>   	 * will be used to calculate new wait time under new configuration.
> +	 * And we need to consider the case of bytes/io_allowed overflow.
>   	 */
>   	if (bps_limit != U64_MAX) {
> -		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> -			tg->bytes_disp[rw];
> -		tg->bytes_disp[rw] = -*bytes;
> +		bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed);
> +		if (bytes_allowed < 0) {
> +			tg->bytes_disp[rw] = 0;
> +		} else {
> +			*bytes = bytes_allowed - tg->bytes_disp[rw];
> +			tg->bytes_disp[rw] = -*bytes;
> +		}
>   	} else {
>   		tg->bytes_disp[rw] = 0;
>   	}
>   
>   	if (iops_limit != UINT_MAX) {
> -		*ios = calculate_io_allowed(iops_limit, jiffy_elapsed) -
> -			tg->io_disp[rw];
> -		tg->io_disp[rw] = -*ios;
> +		io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed);
> +		if (io_allowed < 0) {
> +			tg->io_disp[rw] = 0;
> +		} else {
> +			*ios = io_allowed - tg->io_disp[rw];
> +			tg->io_disp[rw] = -*ios;
> +		}
>   	} else {
>   		tg->io_disp[rw] = 0;
>   	}
> @@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio,
>   
>   	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
>   	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
> -	if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
> +	/* Need to consider the case of bytes_allowed overflow. */
> +	if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
> +	    || bytes_allowed < 0)
>   		return 0;
>   
>   	/* Calc approx time to dispatch */
> 

Re: [PATCH 3/3] blk-throttle: Add an additional overflow check to the call calculate_bytes/io_allowed

[Zizhi Wo]

在 2025/4/17 10:39, Yu Kuai 写道:
> Hi,
> 
> 在 2025/04/17 9:50, Zizhi Wo 写道:
>> Now the tg->[bytes/io]_disp type is signed, and 
>> calculate_bytes/io_allowed
>> return type is unsigned. Even if the bps/iops limit is not set to max, 
>> the
>> return value of the function may still exceed INT_MAX or LLONG_MAX, which
>> can cause overflow in outer variables. In such cases, we can add 
>> additional
>> checks accordingly.
>>
>> And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's
>> no need to call calculate_bytes/io_allowed(). Introduces the helper
>> functions throtl_trim_bps/iops to simplifies the process. For cases when
>> the calculated trim value exceeds INT_MAX (causing an overflow), we reset
>> tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp 
>> because
>> it is the size that is actually trimmed.
>>
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 73 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 4e4609dce85d..cfb9c47d1af7 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, 
>> unsigned long jiffy_elapsed)
>>       return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
>>   }
>> +static long long throtl_trim_bps(struct throtl_grp *tg, bool rw,
>> +                 unsigned long time_elapsed)
>> +{
>> +    u64 bps_limit = tg_bps_limit(tg, rw);
>> +    long long bytes_trim;
>> +
>> +    if (bps_limit == U64_MAX)
>> +        return 0;
>> +
>> +    /* Need to consider the case of bytes_allowed overflow. */
>> +    bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed);
>> +    if (bytes_trim <= 0) {
>> +        bytes_trim = tg->bytes_disp[rw];
>> +        tg->bytes_disp[rw] = 0;
>> +        goto out;
>> +    }
>> +
>> +    if (tg->bytes_disp[rw] >= bytes_trim)
>> +        tg->bytes_disp[rw] -= bytes_trim;
>> +    else
>> +        tg->bytes_disp[rw] = 0;
> 
> Perhaps
> 
> if (bytes_trim <=0 || tg->bytes_disp[rw] < bytes_trim) {
>      bytes_trim = tg->bytes_disp[rw];
>      tg->bytes_disp[rw] = 0;
> } else {
>      tg->bytes_disp[rw] -= bytes_trim;
> }
> 
> And you don't need the out tag.
>> +out:
>> +    return bytes_trim;
>> +}
>> +

Yeah, it's definitely simpler. I'll send out the second version shortly.

Thanks,
Zizhi Wo

>> +static int throtl_trim_iops(struct throtl_grp *tg, bool rw,
>> +                unsigned long time_elapsed)
>> +{
>> +    u32 iops_limit = tg_iops_limit(tg, rw);
>> +    int io_trim;
>> +
>> +    if (iops_limit == UINT_MAX)
>> +        return 0;
>> +
>> +    /* Need to consider the case of io_allowed overflow. */
>> +    io_trim = calculate_io_allowed(iops_limit, time_elapsed);
>> +    if (io_trim <= 0) {
>> +        io_trim = tg->io_disp[rw];
>> +        tg->io_disp[rw] = 0;
>> +        goto out;
>> +    }
>> +
>> +    if (tg->io_disp[rw] >= io_trim)
>> +        tg->io_disp[rw] -= io_trim;
>> +    else
>> +        tg->io_disp[rw] = 0;
>> +out:
>> +    return io_trim;
> 
> Same as above.
> 
> Thanks,
> Kuai
> 
>> +}
>> +
>>   /* Trim the used slices and adjust slice start accordingly */
>>   static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
>>   {
>> @@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct 
>> throtl_grp *tg, bool rw)
>>        * one extra slice is preserved for deviation.
>>        */
>>       time_elapsed -= tg->td->throtl_slice;
>> -    bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw),
>> -                         time_elapsed);
>> -    io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed);
>> -    if (bytes_trim <= 0 && io_trim <= 0)
>> +    bytes_trim = throtl_trim_bps(tg, rw, time_elapsed);
>> +    io_trim = throtl_trim_iops(tg, rw, time_elapsed);
>> +    if (!bytes_trim && !io_trim)
>>           return;
>> -    if ((long long)tg->bytes_disp[rw] >= bytes_trim)
>> -        tg->bytes_disp[rw] -= bytes_trim;
>> -    else
>> -        tg->bytes_disp[rw] = 0;
>> -
>> -    if ((int)tg->io_disp[rw] >= io_trim)
>> -        tg->io_disp[rw] -= io_trim;
>> -    else
>> -        tg->io_disp[rw] = 0;
>> -
>>       tg->slice_start[rw] += time_elapsed;
>>       throtl_log(&tg->service_queue,
>> @@ -643,6 +682,8 @@ static void __tg_update_carryover(struct 
>> throtl_grp *tg, bool rw,
>>       unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
>>       u64 bps_limit = tg_bps_limit(tg, rw);
>>       u32 iops_limit = tg_iops_limit(tg, rw);
>> +    long long bytes_allowed;
>> +    int io_allowed;
>>       /*
>>        * If the queue is empty, carryover handling is not needed. In 
>> such cases,
>> @@ -661,19 +702,28 @@ static void __tg_update_carryover(struct 
>> throtl_grp *tg, bool rw,
>>        * accumulate how many bytes/ios are waited across changes. And 
>> use the
>>        * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, 
>> which
>>        * will be used to calculate new wait time under new configuration.
>> +     * And we need to consider the case of bytes/io_allowed overflow.
>>        */
>>       if (bps_limit != U64_MAX) {
>> -        *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>> -            tg->bytes_disp[rw];
>> -        tg->bytes_disp[rw] = -*bytes;
>> +        bytes_allowed = calculate_bytes_allowed(bps_limit, 
>> jiffy_elapsed);
>> +        if (bytes_allowed < 0) {
>> +            tg->bytes_disp[rw] = 0;
>> +        } else {
>> +            *bytes = bytes_allowed - tg->bytes_disp[rw];
>> +            tg->bytes_disp[rw] = -*bytes;
>> +        }
>>       } else {
>>           tg->bytes_disp[rw] = 0;
>>       }
>>       if (iops_limit != UINT_MAX) {
>> -        *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) -
>> -            tg->io_disp[rw];
>> -        tg->io_disp[rw] = -*ios;
>> +        io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed);
>> +        if (io_allowed < 0) {
>> +            tg->io_disp[rw] = 0;
>> +        } else {
>> +            *ios = io_allowed - tg->io_disp[rw];
>> +            tg->io_disp[rw] = -*ios;
>> +        }
>>       } else {
>>           tg->io_disp[rw] = 0;
>>       }
>> @@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct 
>> throtl_grp *tg, struct bio *bio,
>>       jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, 
>> tg->td->throtl_slice);
>>       bytes_allowed = calculate_bytes_allowed(bps_limit, 
>> jiffy_elapsed_rnd);
>> -    if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= 
>> bytes_allowed)
>> +    /* Need to consider the case of bytes_allowed overflow. */
>> +    if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= 
>> bytes_allowed)
>> +        || bytes_allowed < 0)
>>           return 0;
>>       /* Calc approx time to dispatch */
>>
>