Re: [PATCH 1/2] block/blk-throttle: Fix throttle slice time for SSDs

[Yu Kuai <yukuai@kernel.org>]

Hi,

在 2025/7/31 0:48, Guenter Roeck 写道:
> Commit d61fcfa4bb18 ("blk-throttle: choose a small throtl_slice for SSD")
> introduced device type specific throttle slices if BLK_DEV_THROTTLING_LOW
> was enabled. Commit bf20ab538c81 ("blk-throttle: remove
> CONFIG_BLK_DEV_THROTTLING_LOW") removed support for BLK_DEV_THROTTLING_LOW,
> but left the device type specific throttle slices in place. This
> effectively changed throttling behavior on systems with SSD which now use
> a different and non-configurable slice time compared to non-SSD devices.
> Practical impact is that throughput tests with low configured throttle
> values (65536 bps) experience less than expected throughput on SSDs,
> presumably due to rounding errors associated with the small throttle slice
> time used for those devices. The same tests pass when setting the throttle
> values to 65536 * 4 = 262144 bps.
>
> The original code sets the throttle slice time to DFL_THROTL_SLICE_HD if
> CONFIG_BLK_DEV_THROTTLING_LOW is disabled. Restore that code to fix the
> problem. With that, DFL_THROTL_SLICE_SSD is no longer necessary. Revert to
> the original code and re-introduce DFL_THROTL_SLICE to replace both
> DFL_THROTL_SLICE_HD and DFL_THROTL_SLICE_SSD. This effectively reverts
> commit d61fcfa4bb18 ("blk-throttle: choose a small throtl_slice for SSD").
>
> After the removal of CONFIG_BLK_DEV_THROTTLING_LOW, it is no longer
> necessary to enable block accounting, so remove the call to
> blk_stat_enable_accounting(). With that, the track_bio_latency variable
> is no longer used and can be deleted from struct throtl_data. Also,
> including blk-stat.h is no longer necessary.
>
> While at it, also remove MAX_THROTL_SLICE since it is not used anymore.
>
> Fixes: bf20ab538c81 ("blk-throttle: remove CONFIG_BLK_DEV_THROTTLING_LOW")
> Cc: Yu Kuai <yukuai3@huawei.com>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   block/blk-throttle.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 397b6a410f9e..924d09b51b69 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -12,7 +12,6 @@
>   #include <linux/blktrace_api.h>
>   #include "blk.h"
>   #include "blk-cgroup-rwstat.h"
> -#include "blk-stat.h"
>   #include "blk-throttle.h"
>   
>   /* Max dispatch from a group in 1 round */
> @@ -22,9 +21,7 @@
>   #define THROTL_QUANTUM 32
>   
>   /* Throttling is performed over a slice and after that slice is renewed */
> -#define DFL_THROTL_SLICE_HD (HZ / 10)
> -#define DFL_THROTL_SLICE_SSD (HZ / 50)
> -#define MAX_THROTL_SLICE (HZ)
> +#define DFL_THROTL_SLICE (HZ / 10)
>   
>   /* A workqueue to queue throttle related work */
>   static struct workqueue_struct *kthrotld_workqueue;
> @@ -45,8 +42,6 @@ struct throtl_data
>   
>   	/* Work for dispatching throttled bios */
>   	struct work_struct dispatch_work;
> -
> -	bool track_bio_latency;
>   };
>   
>   static void throtl_pending_timer_fn(struct timer_list *t);
> @@ -1345,13 +1340,7 @@ static int blk_throtl_init(struct gendisk *disk)
>   		goto out;
>   	}
>   
> -	if (blk_queue_nonrot(q))
> -		td->throtl_slice = DFL_THROTL_SLICE_SSD;
> -	else
> -		td->throtl_slice = DFL_THROTL_SLICE_HD;
> -	td->track_bio_latency = !queue_is_mq(q);
> -	if (!td->track_bio_latency)
> -		blk_stat_enable_accounting(q);
> +	td->throtl_slice = DFL_THROTL_SLICE;
>   
>   out:
>   	blk_mq_unquiesce_queue(disk->queue);
This looks correct, I do missed the throtl_slice for ssd is only used with
BLK_DEV_THROTTLING_LOW. However, I think it's better to factor the
track_bio_latency changes into a separate patch.

Thanks,
Kuai