On 3/14/24 11:08 AM, Bart Van Assche wrote:
> On 3/13/24 18:03, ??? (Zhiguo Niu) wrote:
>> Just as mentioned in original patch, "dd->async_depth = max(1UL, 3 * q->nr_requests / 4);", this limitation methods look likes won't have a limit effect, because tag allocated is based on sbitmap, not based the whole nr_requests.
>> Right?
>> Thanks!
>>
>> For write requests, when we assign a tags from sched_tags,
>> data->shallow_depth will be passed to sbitmap_find_bit,
>> see the following code:
>>
>> nr = sbitmap_find_bit_in_word(&sb->map[index],
>> min_t (unsigned int,
>> __map_depth(sb, index),
>> depth),
>> alloc_hint, wrap);
>>
>> The smaller of data->shallow_depth and __map_depth(sb, index)
>> will be used as the maximum range when allocating bits.
>>
>> For a mmc device (one hw queue, deadline I/O scheduler):
>> q->nr_requests = sched_tags = 128, so according to the previous
>> calculation method, dd->async_depth = data->shallow_depth = 96,
>> and the platform is 64bits with 8 cpus, sched_tags.bitmap_tags.sb.shift=5,
>> sb.maps[]=32/32/32/32, 32 is smaller than 96, whether it is a read or
>> a write I/O, tags can be allocated to the maximum range each time,
>> which has not throttling effect.
> Whether or not the code in my patch effectively performs throttling,
> we need this revert to be merged. The patch that is being reverted
> ("block/mq-deadline: use correct way to throttling write requests")
> ended up in Greg KH's stable branches. Hence, the first step is to
> revert that patch and tag it with "Cc: stable" such that the revert
> lands in the stable branches.
Indeed, no amount of arguing is going to change that fact. Zhiguo, it
caused a regression. Rather than argue on why the change is correct,
it'd be much more productive to figure out a future solution.
--
Jens Axboe
