Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

public inbox for linux-next@vger.kernel.org 
 help / color / mirror / Atom feed

From: Damien Le Moal <dlemoal@kernel•org>
To: Murphy Zhou <jencce.kernel@gmail•com>
Cc: linux-block@vger•kernel.org,
	Linux-Next <linux-next@vger•kernel.org>,
	Jens Axboe <axboe@kernel•dk>, Jan Kara <jack@suse•cz>
Subject: Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value
Date: Mon, 29 May 2023 11:28:02 +0900	[thread overview]
Message-ID: <036846b9-c757-450b-2612-6a649b901888@kernel.org> (raw)
In-Reply-To: <CADJHv_uXXVdog0GsFCTd+eouVAuX_wT9NRKJeUUqfZNvz34b0w@mail.gmail.com>

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

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

next prev parent reply	other threads:[~2023-05-29  2:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-26  7:27 ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value Murphy Zhou
2023-05-26  7:42 ` Damien Le Moal
2023-05-27  0:02   ` Murphy Zhou
2023-05-29  2:28     ` Damien Le Moal [this message]
2023-05-29  5:46       ` Murphy Zhou
2023-05-29 13:15         ` Murphy Zhou

find likely ancestor, descendant, or conflicting patches for this message:
( dfblob:607c7617b9d dfblob:c09cfbee911 )
 OR (
bs:"Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value" )
	(help)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=036846b9-c757-450b-2612-6a649b901888@kernel.org \
    --to=dlemoal@kernel$(echo .)org \
    --cc=axboe@kernel$(echo .)dk \
    --cc=jack@suse$(echo .)cz \
    --cc=jencce.kernel@gmail$(echo .)com \
    --cc=linux-block@vger$(echo .)kernel.org \
    --cc=linux-next@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox