From: Damien Le Moal <hidden> Date: 2021-08-06 05:11:46
This series fixes problems with IO priority values handling and cleans
up several macro names and code for clarity.
Changes from v1:
* Added patch 4 to unify the default priority value used in various
places.
* Fixed patch 2 as suggested by Bart: remove extra parenthesis and move
ioprio_valid() from the uapi header to the kernel header.
* In patch 2, add priority value masking.
Damien Le Moal (4):
block: bfq: fix bfq_set_next_ioprio_data()
block: fix ioprio interface
block: rename IOPRIO_BE_NR
block: fix default IO priority handling
block/bfq-iosched.c | 10 +++++-----
block/bfq-iosched.h | 4 ++--
block/bfq-wf2q.c | 6 +++---
block/ioprio.c | 9 ++++-----
drivers/nvme/host/lightnvm.c | 2 +-
fs/f2fs/sysfs.c | 2 +-
include/linux/ioprio.h | 22 ++++++++++++++++++----
include/uapi/linux/ioprio.h | 23 +++++++++++++----------
8 files changed, 47 insertions(+), 31 deletions(-)
--
2.31.1
From: Damien Le Moal <hidden> Date: 2021-08-06 05:11:46
For a request that has a priority level equal to or larger than
IOPRIO_BE_NR, bfq_set_next_ioprio_data() prints a critical warning but
defaults to setting the request new_ioprio field to IOPRIO_BE_NR. This
is not consistent with the warning and the allowed values for priority
levels. Fix this by setting the request new_ioprio field to
IOPRIO_BE_NR - 1, the lowest priority level allowed.
Cc: <redacted>
Fixes: aee69d78dec0 ("block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler")
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Damien Le Moal <hidden> Date: 2021-08-06 05:11:48
An iocb aio_reqprio field is 16-bits (u16) but often handled as an int
in the block layer. E.g. ioprio_check_cap() takes an int as argument.
With such implicit int casting function calls, the upper 16-bits of the
int argument may be left uninitialized by the compiler, resulting in
invalid values for the IOPRIO_PRIO_CLASS() macro (garbage upper bits)
and in an error return for functions such as ioprio_check_cap().
Fix this by masking the result of the shift by IOPRIO_CLASS_SHIFT bits
in the IOPRIO_PRIO_CLASS() macro. The new macro IOPRIO_CLASS_MASK
defines the 3-bits mask for the priority class.
While at it, cleanup the following:
* Apply the mask IOPRIO_PRIO_MASK to the data argument of the
IOPRIO_PRIO_VALUE() macro to ignore upper bits of the data value.
* Remove unnecessary parenthesis around fixed values in the macro
definitions in include/uapi/linux/ioprio.h.
* Update the outdated mention of CFQ in the comment describing priority
classes and instead mention BFQ and mq-deadline.
* Change the argument name of the IOPRIO_PRIO_CLASS() and
IOPRIO_PRIO_DATA() macros from "mask" to "ioprio" to reflect the fact
that an IO priority value should be passed rather than a mask.
* Change the ioprio_valid() macro into an inline function, adding a
check on the maximum value of the class of a priority value as
defined by the IOPRIO_CLASS_MAX enum value. Move this function to
the kernel side in include/linux/ioprio.h.
* Remove the unnecessary "else" after the return statements in
task_nice_ioclass().
Signed-off-by: Damien Le Moal <redacted>
---
include/linux/ioprio.h | 15 ++++++++++++---
include/uapi/linux/ioprio.h | 19 +++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
From: Damien Le Moal <hidden> Date: 2021-08-06 05:11:49
The BFQ scheduler and ioprio_check_cap() both assume that the RT
priority class (IOPRIO_CLASS_RT) can have up to 8 different priority
levels. This is controlled using the macro IOPRIO_BE_NR, which is badly
named as the number of levels applies to the RT class.
Rename IOPRIO_BE_NR to the class independent IOPRIO_NR_LEVELS to make
things clear.
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 8 ++++----
block/bfq-iosched.h | 4 ++--
block/bfq-wf2q.c | 6 +++---
block/ioprio.c | 3 +--
fs/f2fs/sysfs.c | 2 +-
include/uapi/linux/ioprio.h | 4 ++--
6 files changed, 13 insertions(+), 14 deletions(-)
@@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)fallthrough;/* rt has prio field too */caseIOPRIO_CLASS_BE:-if(data>=IOPRIO_BE_NR||data<0)+if(data>=IOPRIO_NR_LEVELS||data<0)return-EINVAL;-break;caseIOPRIO_CLASS_IDLE:break;
From: Damien Le Moal <hidden> Date: 2021-08-06 05:11:50
The default IO priority is the best effort (BE) class with the
normal priority level IOPRIO_NORM (4). However, get_task_ioprio()
returns IOPRIO_CLASS_NONE/IOPRIO_NORM as the default priority and
get_current_ioprio() returns IOPRIO_CLASS_NONE/0. Let's be consistent
with the defined default and have both of these functions return the
default priority IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) when
the user did not define another default IO priority for the task.
In include/linux/ioprio.h, rename the IOPRIO_NORM macro to
IOPRIO_BE_NORM to clarify that this default level applies to the BE
priotity class. Also, define the macro IOPRIO_DEFAULT as
IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM) and use this new
macro when setting a priority to the default.
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 2 +-
block/ioprio.c | 6 +++---
drivers/nvme/host/lightnvm.c | 2 +-
include/linux/ioprio.h | 7 ++++++-
include/uapi/linux/ioprio.h | 4 ++--
5 files changed, 13 insertions(+), 8 deletions(-)
From: Hannes Reinecke <hare@suse.de> Date: 2021-08-06 06:33:56
On 8/6/21 7:11 AM, Damien Le Moal wrote:
For a request that has a priority level equal to or larger than
IOPRIO_BE_NR, bfq_set_next_ioprio_data() prints a critical warning but
defaults to setting the request new_ioprio field to IOPRIO_BE_NR. This
is not consistent with the warning and the allowed values for priority
levels. Fix this by setting the request new_ioprio field to
IOPRIO_BE_NR - 1, the lowest priority level allowed.
Cc: <redacted>
Fixes: aee69d78dec0 ("block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler")
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
From: Hannes Reinecke <hare@suse.de> Date: 2021-08-06 06:35:38
On 8/6/21 7:11 AM, Damien Le Moal wrote:
quoted hunk
An iocb aio_reqprio field is 16-bits (u16) but often handled as an int
in the block layer. E.g. ioprio_check_cap() takes an int as argument.
With such implicit int casting function calls, the upper 16-bits of the
int argument may be left uninitialized by the compiler, resulting in
invalid values for the IOPRIO_PRIO_CLASS() macro (garbage upper bits)
and in an error return for functions such as ioprio_check_cap().
Fix this by masking the result of the shift by IOPRIO_CLASS_SHIFT bits
in the IOPRIO_PRIO_CLASS() macro. The new macro IOPRIO_CLASS_MASK
defines the 3-bits mask for the priority class.
While at it, cleanup the following:
* Apply the mask IOPRIO_PRIO_MASK to the data argument of the
IOPRIO_PRIO_VALUE() macro to ignore upper bits of the data value.
* Remove unnecessary parenthesis around fixed values in the macro
definitions in include/uapi/linux/ioprio.h.
* Update the outdated mention of CFQ in the comment describing priority
classes and instead mention BFQ and mq-deadline.
* Change the argument name of the IOPRIO_PRIO_CLASS() and
IOPRIO_PRIO_DATA() macros from "mask" to "ioprio" to reflect the fact
that an IO priority value should be passed rather than a mask.
* Change the ioprio_valid() macro into an inline function, adding a
check on the maximum value of the class of a priority value as
defined by the IOPRIO_CLASS_MAX enum value. Move this function to
the kernel side in include/linux/ioprio.h.
* Remove the unnecessary "else" after the return statements in
task_nice_ioclass().
Signed-off-by: Damien Le Moal <redacted>
---
include/linux/ioprio.h | 15 ++++++++++++---
include/uapi/linux/ioprio.h | 19 +++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
Wouldn't it be better to use 'u16' here as type, as we're relying on the
number of bits?
quoted hunk
+{
+ unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
+
+ return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX;
+}
+
/*
* if process has set io priority explicitly, use that. if not, convert
* the cpu scheduler nice value to an io priority
@@ -25,10 +35,9 @@ static inline int task_nice_ioclass(struct task_struct *task) { if (task->policy == SCHED_IDLE) return IOPRIO_CLASS_IDLE;- else if (task_is_realtime(task))+ if (task_is_realtime(task)) return IOPRIO_CLASS_RT;- else- return IOPRIO_CLASS_BE;+ return IOPRIO_CLASS_BE; } /*
From: Hannes Reinecke <hare@suse.de> Date: 2021-08-06 06:38:05
On 8/6/21 7:11 AM, Damien Le Moal wrote:
quoted hunk
The BFQ scheduler and ioprio_check_cap() both assume that the RT
priority class (IOPRIO_CLASS_RT) can have up to 8 different priority
levels. This is controlled using the macro IOPRIO_BE_NR, which is badly
named as the number of levels applies to the RT class.
Rename IOPRIO_BE_NR to the class independent IOPRIO_NR_LEVELS to make
things clear.
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 8 ++++----
block/bfq-iosched.h | 4 ++--
block/bfq-wf2q.c | 6 +++---
block/ioprio.c | 3 +--
fs/f2fs/sysfs.c | 2 +-
include/uapi/linux/ioprio.h | 4 ++--
6 files changed, 13 insertions(+), 14 deletions(-)
@@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)fallthrough;/* rt has prio field too */caseIOPRIO_CLASS_BE:-if(data>=IOPRIO_BE_NR||data<0)+if(data>=IOPRIO_NR_LEVELS||data<0)return-EINVAL;-break;caseIOPRIO_CLASS_IDLE:break;
This sentence no verb :-)
(maybe 'The RT class is an BE priority ...'?)
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
From: Hannes Reinecke <hare@suse.de> Date: 2021-08-06 06:39:31
On 8/6/21 7:11 AM, Damien Le Moal wrote:
The default IO priority is the best effort (BE) class with the
normal priority level IOPRIO_NORM (4). However, get_task_ioprio()
returns IOPRIO_CLASS_NONE/IOPRIO_NORM as the default priority and
get_current_ioprio() returns IOPRIO_CLASS_NONE/0. Let's be consistent
with the defined default and have both of these functions return the
default priority IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) when
the user did not define another default IO priority for the task.
In include/linux/ioprio.h, rename the IOPRIO_NORM macro to
IOPRIO_BE_NORM to clarify that this default level applies to the BE
priotity class. Also, define the macro IOPRIO_DEFAULT as
IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM) and use this new
macro when setting a priority to the default.
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 2 +-
block/ioprio.c | 6 +++---
drivers/nvme/host/lightnvm.c | 2 +-
include/linux/ioprio.h | 7 ++++++-
include/uapi/linux/ioprio.h | 4 ++--
5 files changed, 13 insertions(+), 8 deletions(-)
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
From: Damien Le Moal <hidden> Date: 2021-08-06 06:52:16
On 2021/08/06 15:38, Hannes Reinecke wrote:
On 8/6/21 7:11 AM, Damien Le Moal wrote:
quoted
The BFQ scheduler and ioprio_check_cap() both assume that the RT
priority class (IOPRIO_CLASS_RT) can have up to 8 different priority
levels. This is controlled using the macro IOPRIO_BE_NR, which is badly
named as the number of levels applies to the RT class.
Rename IOPRIO_BE_NR to the class independent IOPRIO_NR_LEVELS to make
things clear.
Signed-off-by: Damien Le Moal <redacted>
---
block/bfq-iosched.c | 8 ++++----
block/bfq-iosched.h | 4 ++--
block/bfq-wf2q.c | 6 +++---
block/ioprio.c | 3 +--
fs/f2fs/sysfs.c | 2 +-
include/uapi/linux/ioprio.h | 4 ++--
6 files changed, 13 insertions(+), 14 deletions(-)
@@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)fallthrough;/* rt has prio field too */caseIOPRIO_CLASS_BE:-if(data>=IOPRIO_BE_NR||data<0)+if(data>=IOPRIO_NR_LEVELS||data<0)return-EINVAL;-break;caseIOPRIO_CLASS_IDLE:break;
Hu ? "support" is a verb (to support). It is a noun too, but here, I use the verb :)
There is a typo though: s/an/and
"The RT and BE priority classes support up to 8 priority levels."
Arg... Sending v3 :)
(maybe 'The RT class is an BE priority ...'?)
Cheers,
Hannes
From: Damien Le Moal <hidden> Date: 2021-08-06 06:57:08
On 2021/08/06 15:35, Hannes Reinecke wrote:
On 8/6/21 7:11 AM, Damien Le Moal wrote:
quoted
An iocb aio_reqprio field is 16-bits (u16) but often handled as an int
in the block layer. E.g. ioprio_check_cap() takes an int as argument.
With such implicit int casting function calls, the upper 16-bits of the
int argument may be left uninitialized by the compiler, resulting in
invalid values for the IOPRIO_PRIO_CLASS() macro (garbage upper bits)
and in an error return for functions such as ioprio_check_cap().
Fix this by masking the result of the shift by IOPRIO_CLASS_SHIFT bits
in the IOPRIO_PRIO_CLASS() macro. The new macro IOPRIO_CLASS_MASK
defines the 3-bits mask for the priority class.
While at it, cleanup the following:
* Apply the mask IOPRIO_PRIO_MASK to the data argument of the
IOPRIO_PRIO_VALUE() macro to ignore upper bits of the data value.
* Remove unnecessary parenthesis around fixed values in the macro
definitions in include/uapi/linux/ioprio.h.
* Update the outdated mention of CFQ in the comment describing priority
classes and instead mention BFQ and mq-deadline.
* Change the argument name of the IOPRIO_PRIO_CLASS() and
IOPRIO_PRIO_DATA() macros from "mask" to "ioprio" to reflect the fact
that an IO priority value should be passed rather than a mask.
* Change the ioprio_valid() macro into an inline function, adding a
check on the maximum value of the class of a priority value as
defined by the IOPRIO_CLASS_MAX enum value. Move this function to
the kernel side in include/linux/ioprio.h.
* Remove the unnecessary "else" after the return statements in
task_nice_ioclass().
Signed-off-by: Damien Le Moal <redacted>
---
include/linux/ioprio.h | 15 ++++++++++++---
include/uapi/linux/ioprio.h | 19 +++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
Wouldn't it be better to use 'u16' here as type, as we're relying on the
number of bits?
Other functions in block/ioprio.c and in include/linux/ioprio.h use "unsigned
short", so I followed. But many functions, if not most, use "int". This is all a
bit of a mess. I think we need a "typedef ioprio_t u16;" to clean things up. But
there are a lot of places to fix. I can add such patch... Worth it ?
quoted
+{
+ unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
+
+ return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX;
+}
+
/*
* if process has set io priority explicitly, use that. if not, convert
* the cpu scheduler nice value to an io priority
@@ -25,10 +35,9 @@ static inline int task_nice_ioclass(struct task_struct *task) { if (task->policy == SCHED_IDLE) return IOPRIO_CLASS_IDLE;- else if (task_is_realtime(task))+ if (task_is_realtime(task)) return IOPRIO_CLASS_RT;- else- return IOPRIO_CLASS_BE;+ return IOPRIO_CLASS_BE; } /*
From: Hannes Reinecke <hare@suse.de> Date: 2021-08-06 08:38:07
On 8/6/21 8:57 AM, Damien Le Moal wrote:
On 2021/08/06 15:35, Hannes Reinecke wrote:
quoted
On 8/6/21 7:11 AM, Damien Le Moal wrote:
quoted
An iocb aio_reqprio field is 16-bits (u16) but often handled as an int
in the block layer. E.g. ioprio_check_cap() takes an int as argument.
With such implicit int casting function calls, the upper 16-bits of the
int argument may be left uninitialized by the compiler, resulting in
invalid values for the IOPRIO_PRIO_CLASS() macro (garbage upper bits)
and in an error return for functions such as ioprio_check_cap().
Fix this by masking the result of the shift by IOPRIO_CLASS_SHIFT bits
in the IOPRIO_PRIO_CLASS() macro. The new macro IOPRIO_CLASS_MASK
defines the 3-bits mask for the priority class.
While at it, cleanup the following:
* Apply the mask IOPRIO_PRIO_MASK to the data argument of the
IOPRIO_PRIO_VALUE() macro to ignore upper bits of the data value.
* Remove unnecessary parenthesis around fixed values in the macro
definitions in include/uapi/linux/ioprio.h.
* Update the outdated mention of CFQ in the comment describing priority
classes and instead mention BFQ and mq-deadline.
* Change the argument name of the IOPRIO_PRIO_CLASS() and
IOPRIO_PRIO_DATA() macros from "mask" to "ioprio" to reflect the fact
that an IO priority value should be passed rather than a mask.
* Change the ioprio_valid() macro into an inline function, adding a
check on the maximum value of the class of a priority value as
defined by the IOPRIO_CLASS_MAX enum value. Move this function to
the kernel side in include/linux/ioprio.h.
* Remove the unnecessary "else" after the return statements in
task_nice_ioclass().
Signed-off-by: Damien Le Moal <redacted>
---
include/linux/ioprio.h | 15 ++++++++++++---
include/uapi/linux/ioprio.h | 19 +++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
Wouldn't it be better to use 'u16' here as type, as we're relying on the
number of bits?
Other functions in block/ioprio.c and in include/linux/ioprio.h use "unsigned
short", so I followed. But many functions, if not most, use "int". This is all a
bit of a mess. I think we need a "typedef ioprio_t u16;" to clean things up. But
there are a lot of places to fix. I can add such patch... Worth it ?
Possibly not.
Consider my comment retracted :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer