On Mon, May 29, 2023 at 10:28 AM Damien Le Moal <dlemoal@xxxxxxxxxx> 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@xxxxxxxxxx> 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
>
