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

[Guenter Roeck <linux@roeck-us.net>]

On 7/30/25 11:30, Yu Kuai wrote:
> 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.
> 

I had combined it because it is another left-over from bf20ab538c81 and
I don't know if enabling statistics has other side effects. But, sure,
I can split it out if that is preferred. Let's wait for feedback from
Jens and/or Tejun; I'll follow their guidance.

Thanks,
Guenter