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@xxxxxxx>
>
> ...
>
>>> +/*
>>> + * 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
