+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
