Re: [PATCH 4/4] block/mq-deadline: Prioritize high-priority requests

[Bart Van Assche <bvanassche@acm.org>]

On 9/24/21 4:08 AM, Damien Le Moal wrote:
> On 2021/09/24 8:27, Bart Van Assche wrote:
>> +/*
>> + * Time after which to dispatch lower priority requests even if higher
>> + * priority requests are pending.
>> + */
>> +static const int aging_expire = 10 * HZ;
> 
> What about calling this prio_starved, to be consistent with writes_starved ?
> Or at least let's call it prio_aging_expire to show that it is for priority
> levels, and not for requests within a priority queue.

The name 'prio_starved' probably would cause confusion since this parameter
represents a timeout while 'writes_starved' represents a count. I will rename
it into prio_aging_expire.

>>   dispatch_request:
>> +	if (started_after(dd, rq, latest_start))
>> +		return NULL;
> 
> Nit: add a blank line here ? (for aesthetic :))

Will do.

>> +/*
>> + * Check whether there are any requests with a deadline that expired more than
>> + * aging_expire jiffies ago.
> 
> Hmm... requests do not have a "deadline", so this is a little hard to
> understand. What about something like this:
> 
> * Check whether there are any requests at a low priority level inserted
> * more than aging_expire jiffies ago.

Agreed, I will clarify this comment.

>> + */
>> +static struct request *dd_dispatch_aged_requests(struct deadline_data *dd,
>> +						 unsigned long now)
> 
> Same remark as for the aging_expire variable: it may be good to have prio in the
> name of this function. Something like:
> 
> dd_dispatch_prio_starved_requests() ?

The name 'aging' has a well-defined meaning so I would like to keep it. See also
https://en.wikipedia.org/wiki/Aging_(scheduling).

>>   	spin_lock(&dd->lock);
> 
> Nit: Add a blank line here to have the spin_lock() stand out ?
> Easier to notice it that way...

The convention in kernel code is a blank line above statements that grab a lock
but not below these statements.

Thanks,

Bart.