+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... 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