Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 6/21/22 17:15, Jan Kara wrote:
> On Tue 21-06-22 08:57:29, Damien Le Moal wrote:
>> On 6/21/22 01:11, Jan Kara wrote:
>>> ioprio_get(2) can be asked to return the best IO priority from several
>>> tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
>>> tasks without set IO priority as having priority
>>> IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
>>> IO priority the task will get (which depends on task's nice value).
>>>
>>> Fix the code to use the real IO priority task's IO will use. For this we
>>> do some factoring out to share the code converting task's CPU priority
>>> to IO priority and we also have to modify code for
>>> ioprio_get(IOPRIO_WHO_PROCESS) to keep returning IOPRIO_CLASS_NONE
>>> priority for tasks without set IO priority as a special case to maintain
>>> userspace visible API.
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> ...
> 
>>> +/*
>>> + * Return raw IO priority value as set by userspace. We use this for
>>> + * ioprio_get(pid, IOPRIO_WHO_PROCESS) so that we keep historical behavior and
>>> + * also so that userspace can distinguish unset IO priority (which just gets
>>> + * overriden based on task's nice value) from IO priority set to some value.
>>> + */
>>> +static int get_task_raw_ioprio(struct task_struct *p) { int ret;
>>
>> The "int ret;" declaration is on the wrong line, so is the curly bracket.
> 
> Huh, don't know how that got messed up. Anyway fixed. Thanks for noticing.
> 
>>> +
>>> +	ret = security_task_getioprio(p);
>>> +	if (ret)
>>> +		goto out;
>>>  	task_lock(p);
>>>  	if (p->io_context)
>>>  		ret = p->io_context->ioprio;
>>> +	else
>>> +		ret = IOPRIO_DEFAULT;
>>>  	task_unlock(p);
>>>  out:
>>>  	return ret;
>>> @@ -156,11 +196,6 @@ static int get_task_ioprio(struct task_struct *p)
>>>  
>>>  static int ioprio_best(unsigned short aprio, unsigned short bprio)
>>>  {
>>> -	if (!ioprio_valid(aprio))
>>> -		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>>> -	if (!ioprio_valid(bprio))
>>> -		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>>> -
>>>  	return min(aprio, bprio);
>>>  }
>>
>> This function could be declared as inline now...
> 
> Yeah, but compilers inline (or not inline!) static functions as they see
> fit anyway. So 'inline' keyword for static functions is generally a noise
> which is why I just avoid it. But please let me know if you feel strongly
> about that.

Not at all. Fine as-is !

> 
>>>  static inline int get_current_ioprio(void)
>>>  {
>>> -	struct io_context *ioc = current->io_context;
>>> -	int prio;
>>> -
>>> -	if (ioc)
>>> -		prio = ioc->ioprio;
>>> -	else
>>> -		prio = IOPRIO_DEFAULT;
>>> -
>>> -	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
>>> -		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
>>> -					 task_nice_ioprio(current));
>>> -	return prio;
>>> +	return __get_task_ioprio(current);
>>
>> The build bot complained about this one, but I do not understand why.
>> Could it be because you do not have declared __get_task_ioprio() as "extern" ?
> 
> No, likely it is because !CONFIG_BLOCK kernel does not have ioprio support
> but something uses the get_current_ioprio() anyway. I'll fix it up.
> 
>> Also, to reduce refactoring changes in this patch, you could introduce
>> __get_task_ioprio() and make the above change in patch 2. No ?
> 
> OK, I will move some refactoring.
> 
> 								Honza


-- 
Damien Le Moal
Western Digital Research