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

[Ming Lei <ming.lei@redhat.com>]

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