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.
> > 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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
