[LTP] ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

Adding ioprio_set03.c author Wallejj.

On Mon, May 29, 2023 at 1:46 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:
>
> On Mon, May 29, 2023 at 10:28 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> >
> > +Jens
> >
> > and also +Jan as he did some work on ioprio previously.
> >
> > On 5/27/23 09:02, Murphy Zhou wrote:
> > > On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> > >>
> > >> On 5/26/23 16:27, Murphy Zhou wrote:
> > >>> Hi Damien,
> > >>>
> > >>> Since these commits:
> > >>>
> > >>>   scsi: block: Introduce ioprio hints
> > >>>   scsi: block: ioprio: Clean up interface definition
> > >>>
> > >>> go into linux-next tree, ioprio_set can take the value of 8
> > >>> as the PROCESS CLASS_BE ioprio parameter, returning
> > >>> success but actually it is setting to 0 due to the mask roundup.
> > >>>
> > >>> The LTP test case ioprio_set03[1] covers this boundary value
> > >>> testing, which starts to fail since then.
> > >>>
> > >>> This does not look as expected. Could you help to take a look?
> > >>
> > >> Before the patches, the ioprio level of 8 could indeed be set, but that was
> > >
> > > Before the patches, it can't be set to 8 because the check in
> > > ioprio_check_cap refused it.
> > >    >= IOPRIO_NR_LEVELS
> > > Before the patches, the value can be greater than 8, so it takes effect.
> > > After the patches, the value is limited to [0..7], this check always passes.
> > >
> > >> actually totally meaningless since the kernel components that use the priority
> > >> level all are limited to the range [0..7]. And why the level value 8 could be
> > >> seen, the effective level would have been 0. So at least, with the changes, we
> > >> are not lying to the user...
> > >>
> > >> I am not sure what this ioprio_set03 test is trying to check.
> > >
> > > I guess it is trying to make sure boundary values do not cause uncertaining.
> > > The test case can be updated according to intended kernel changes. So does
> > > other user space applications that may depend on this, or there is none of
> > > them to worry about.
> >
> > The checks before the patch was using IOPRIO_PRIO_DATA() to get the
> > level value, and that macro was doing a mask with IOPRIO_PRIO_MASK (1 <<
> > 13). So if the application was doing:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 8192 + 1)
> >
> > then the ioprio value would end up being the same as:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0)
> >
> > which is a valid ioprio value. So ioprio_check_cap() would not detect
> > that case either. The fact that class and level are combined into a
> > single value essentially prevents us to be exhaustive with the checks in
> > ioprio_check_cap().
> >
> > I am not sure if this is worth fixing. as no matter what we do, the
> > above problem remains: we cannot catch all bad cases and end up with a
> > valid value which will prevent the user from seeing an error and
> > catching his/hers invalid use of the ioprio values...
>
> Agree.  We can't prevent that. Please go ahead with any solution that
> makes sense to you.
>
> Thanks,
> Murphy
> >
> > We could do something like shown below, but I am not sure if it is worth
> > it as their are no guarantees that the user is actually using the
> > definitions in include/uapi/linux/ioprio.h
> > (/usr/include/linux/iorprio.h). E.g. see fio code which redefines the
> > values and macros locally.
> >
> > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> > index 607c7617b9d2..c09cfbee9117 100644
> > --- a/include/uapi/linux/ioprio.h
> > +++ b/include/uapi/linux/ioprio.h
> > @@ -6,15 +6,13 @@
> >   * Gives us 8 prio classes with 13-bits of data for each class
> >   */
> >  #define IOPRIO_CLASS_SHIFT     13
> > -#define IOPRIO_CLASS_MASK      0x07
> > +#define IOPRIO_NR_CLASSES      8
> > +#define IOPRIO_CLASS_MASK      (IOPRIO_NR_CLASSES - 1)
> >  #define IOPRIO_PRIO_MASK       ((1UL << IOPRIO_CLASS_SHIFT) - 1)
> >
> >  #define IOPRIO_PRIO_CLASS(ioprio)      \
> >         (((ioprio) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK)
> >  #define IOPRIO_PRIO_DATA(ioprio)       ((ioprio) & IOPRIO_PRIO_MASK)
> > -#define IOPRIO_PRIO_VALUE(class, data) \
> > -       ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \
> > -        ((data) & IOPRIO_PRIO_MASK))
> >
> >  /*
> >   * These are the io priority classes as implemented by the BFQ and
> > mq-deadline
> > @@ -73,15 +71,6 @@ enum {
> >  #define IOPRIO_PRIO_HINT(ioprio)       \
> >         (((ioprio) >> IOPRIO_HINT_SHIFT) & IOPRIO_HINT_MASK)
> >
> > -/*
> > - * Alternate macro for IOPRIO_PRIO_VALUE() to define an IO priority with
> > - * a class, level and hint.
> > - */
> > -#define IOPRIO_PRIO_VALUE_HINT(class, level, hint)              \
> > -       ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \
> > -        (((hint) & IOPRIO_HINT_MASK) << IOPRIO_HINT_SHIFT) |    \
> > -        ((level) & IOPRIO_LEVEL_MASK))
> > -
> >  /*
> >   * IO hints.
> >   */
> > @@ -107,4 +96,22 @@ enum {
> >         IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
> >  };
> >
> > +/*
> > + * Return an I/O priority value based on a class, a level and hints.
> > + */
> > +static inline u16 ioprio_value(int class, int level, int hint)
> > +{
> > +       if (class < 0 || class >= IOPRIO_NR_CLASSES ||
> > +           level < 0 || level >= IOPRIO_NR_LEVELS ||
> > +           hint < 0 || hint >= IOPRIO_NR_HINTS)
> > +               return USHRT_MAX;
> > +       return (class << IOPRIO_CLASS_SHIFT) |
> > +              (hint << IOPRIO_HINT_SHIFT) | level;
> > +}
> > +
> > +#define IOPRIO_PRIO_VALUE(class, level)                        \
> > +       ioprio_value(class, level, IOPRIO_HINT_NONE)
> > +#define IOPRIO_PRIO_VALUE_HINT(class, level, hint)     \
> > +       ioprio_value(class, level, hint)
> > +
> >  #endif /* _UAPI_LINUX_IOPRIO_H */
> >
> > --
> > Damien Le Moal
> > Western Digital Research
> >