Re: [PATCH v2] block/mq-deadline: adjust the timeout period of the per_prio->dispatch

[Damien Le Moal <dlemoal@kernel.org>]

On 2025/10/10 1:50, Bart Van Assche wrote:
> On 10/9/25 8:52 AM, chengkaitao wrote:
>> On the other hand, the Commit (725f22a1477c) merges the effects of
>> fifo_expire and prio_aging_expire on the same code behavior, creating
>> redundant interactions. To address this, our new patch introduces
>> numerical compensation for {dd->fifo_expire[data_dir]} when adding
>> requests to dispatch lists. To maintain original logic as much as
>> possible while enhancing dispatch list priority, we additionally
>> subtract {dd->prio_aging_expire / 2} from the fifo_time, with default
>> values, {dd->prio_aging_expire / 2} equals {dd->fifo_expire[DD_WRITE]}.
> 
> No assumptions should be made about the relative values of
> dd->prio_aging_expire and dd->fifo_expire[DD_WRITE] since these values
> can be modified via sysfs.
> 
>> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
>> index 3e741d33142d..fedc66187150 100644
>> --- a/block/mq-deadline.c
>> +++ b/block/mq-deadline.c
>> @@ -659,7 +659,8 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>   
>>   	if (flags & BLK_MQ_INSERT_AT_HEAD) {
>>   		list_add(&rq->queuelist, &per_prio->dispatch);
>> -		rq->fifo_time = jiffies;
>> +		rq->fifo_time = jiffies + dd->fifo_expire[data_dir]
>> +				- dd->prio_aging_expire / 2;
>>   	} else {
>>   		deadline_add_rq_rb(per_prio, rq);
> 
> Thanks for having added a detailed patch description. Please remove
> "/ 2" from the above patch to make sure that BLK_MQ_INSERT_AT_HEAD
> requests are submitted to the block driver before other requests. This
> is important if a request is requeued. Additionally, a comment should be
> added above the modified line of code that explains the purpose of the
> calculation. How about this untested patch?
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index 3e741d33142d..566646591ddd 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -659,7 +659,14 @@ static void dd_insert_request(struct blk_mq_hw_ctx 
> *hctx, struct request *rq,
> 
>   	if (flags & BLK_MQ_INSERT_AT_HEAD) {
>   		list_add(&rq->queuelist, &per_prio->dispatch);
> -		rq->fifo_time = jiffies;
> +		/*
> +		 * started_after() subtracts dd->fifo_expire[data_dir] from
> +		 * rq->fifo_time. Hence, add it here. Subtract
> +		 * dd->prio_aging_expire to ensure that AT HEAD requests are
> +		 * submitted before higher priority requests.
> +		 */
> +		rq->fifo_time = jiffies + dd->fifo_expire[data_dir] -
> +				dd->prio_aging_expire;

There is still something bothering me with this: the request is added to the
dispatch list, and *NOT* to the fifo/sort list. So this should be considered as
a scheduling decision in itself, and __dd_dispatch_request() reflects that as
the first thing it does is pick the requests that are in the dispatch list
already. However, __dd_dispatch_request() also has the check:

		if (started_after(dd, rq, latest_start))
                        return NULL;

for requests that are already in the dispatch list. That is what does not make
sense to me. Why ? There is no comment describing this. And I do not understand
why we should bother with any time for requests that are in the dispatch list
already. These should be sent to the drive first, always.

This patch seems to be fixing a problem that is introduced by the above check.
But why this check ? What am I missing here ?

>   	} else {
>   		deadline_add_rq_rb(per_prio, rq);
> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research