* [PATCH v2 0/4] IO priority fixes and improvements
@ 2021-08-06 5:11 Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data() Damien Le Moal
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 5:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Paolo Valente, linux-f2fs-devel,
Jaegeuk Kim, Chao Yu
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data()
2021-08-06 5:11 [PATCH v2 0/4] IO priority fixes and improvements Damien Le Moal
@ 2021-08-06 5:11 ` Damien Le Moal
2021-08-06 6:33 ` Hannes Reinecke
2021-08-06 5:11 ` [PATCH v2 2/4] block: fix ioprio interface Damien Le Moal
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 5:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Paolo Valente, linux-f2fs-devel,
Jaegeuk Kim, Chao Yu
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: <stable@vger.kernel.org>
Fixes: aee69d78dec0 ("block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler")
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/bfq-iosched.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 727955918563..1f38d75524ae 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5293,7 +5293,7 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
if (bfqq->new_ioprio >= IOPRIO_BE_NR) {
pr_crit("bfq_set_next_ioprio_data: new_ioprio %d\n",
bfqq->new_ioprio);
- bfqq->new_ioprio = IOPRIO_BE_NR;
+ bfqq->new_ioprio = IOPRIO_BE_NR - 1;
}
bfqq->entity.new_weight = bfq_ioprio_to_weight(bfqq->new_ioprio);
--
2.31.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] block: fix ioprio interface
2021-08-06 5:11 [PATCH v2 0/4] IO priority fixes and improvements Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data() Damien Le Moal
@ 2021-08-06 5:11 ` Damien Le Moal
2021-08-06 6:35 ` Hannes Reinecke
2021-08-06 5:11 ` [PATCH v2 3/4] block: rename IOPRIO_BE_NR Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 4/4] block: fix default IO priority handling Damien Le Moal
3 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 5:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Paolo Valente, linux-f2fs-devel,
Jaegeuk Kim, Chao Yu
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 <damien.lemoal@wdc.com>
---
include/linux/ioprio.h | 15 ++++++++++++---
include/uapi/linux/ioprio.h | 19 +++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index ef9ad4fb245f..9b3a6d8172b4 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -8,6 +8,16 @@
#include <uapi/linux/ioprio.h>
+/*
+ * Check that a priority value has a valid class.
+ */
+static inline bool ioprio_valid(unsigned short ioprio)
+{
+ 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;
}
/*
diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
index 77b17e08b0da..abc40965aa96 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -5,12 +5,15 @@
/*
* Gives us 8 prio classes with 13-bits of data for each class
*/
-#define IOPRIO_CLASS_SHIFT (13)
+#define IOPRIO_CLASS_SHIFT 13
+#define IOPRIO_CLASS_MASK 0x07
#define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1)
-#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT)
-#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK)
-#define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data)
+#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_SHIFT) | ((data) & IOPRIO_PRIO_MASK))
/*
* These are the io priority groups as implemented by CFQ. RT is the realtime
@@ -23,14 +26,14 @@ enum {
IOPRIO_CLASS_RT,
IOPRIO_CLASS_BE,
IOPRIO_CLASS_IDLE,
-};
-#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
+ IOPRIO_CLASS_MAX,
+};
/*
* 8 best effort priority levels are supported
*/
-#define IOPRIO_BE_NR (8)
+#define IOPRIO_BE_NR 8
enum {
IOPRIO_WHO_PROCESS = 1,
@@ -41,6 +44,6 @@ enum {
/*
* Fallback BE priority
*/
-#define IOPRIO_NORM (4)
+#define IOPRIO_NORM 4
#endif /* _UAPI_LINUX_IOPRIO_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] block: rename IOPRIO_BE_NR
2021-08-06 5:11 [PATCH v2 0/4] IO priority fixes and improvements Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data() Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 2/4] block: fix ioprio interface Damien Le Moal
@ 2021-08-06 5:11 ` Damien Le Moal
2021-08-06 6:37 ` Hannes Reinecke
2021-08-06 5:11 ` [PATCH v2 4/4] block: fix default IO priority handling Damien Le Moal
3 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 5:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Paolo Valente, linux-f2fs-devel,
Jaegeuk Kim, Chao Yu
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 <damien.lemoal@wdc.com>
---
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(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 1f38d75524ae..d5824cab34d7 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2505,7 +2505,7 @@ void bfq_end_wr_async_queues(struct bfq_data *bfqd,
int i, j;
for (i = 0; i < 2; i++)
- for (j = 0; j < IOPRIO_BE_NR; j++)
+ for (j = 0; j < IOPRIO_NR_LEVELS; j++)
if (bfqg->async_bfqq[i][j])
bfq_bfqq_end_wr(bfqg->async_bfqq[i][j]);
if (bfqg->async_idle_bfqq)
@@ -5290,10 +5290,10 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
break;
}
- if (bfqq->new_ioprio >= IOPRIO_BE_NR) {
+ if (bfqq->new_ioprio >= IOPRIO_NR_LEVELS) {
pr_crit("bfq_set_next_ioprio_data: new_ioprio %d\n",
bfqq->new_ioprio);
- bfqq->new_ioprio = IOPRIO_BE_NR - 1;
+ bfqq->new_ioprio = IOPRIO_NR_LEVELS - 1;
}
bfqq->entity.new_weight = bfq_ioprio_to_weight(bfqq->new_ioprio);
@@ -6822,7 +6822,7 @@ void bfq_put_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg)
int i, j;
for (i = 0; i < 2; i++)
- for (j = 0; j < IOPRIO_BE_NR; j++)
+ for (j = 0; j < IOPRIO_NR_LEVELS; j++)
__bfq_put_async_bfqq(bfqd, &bfqg->async_bfqq[i][j]);
__bfq_put_async_bfqq(bfqd, &bfqg->async_idle_bfqq);
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 99c2a3cb081e..385e28a843d1 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -931,7 +931,7 @@ struct bfq_group {
void *bfqd;
- struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
+ struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
struct bfq_queue *async_idle_bfqq;
struct bfq_entity *my_entity;
@@ -948,7 +948,7 @@ struct bfq_group {
struct bfq_entity entity;
struct bfq_sched_data sched_data;
- struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
+ struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
struct bfq_queue *async_idle_bfqq;
struct rb_root rq_pos_tree;
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 7a462df71f68..b74cc0da118e 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -505,7 +505,7 @@ static void bfq_active_insert(struct bfq_service_tree *st,
*/
unsigned short bfq_ioprio_to_weight(int ioprio)
{
- return (IOPRIO_BE_NR - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
+ return (IOPRIO_NR_LEVELS - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
}
/**
@@ -514,12 +514,12 @@ unsigned short bfq_ioprio_to_weight(int ioprio)
*
* To preserve as much as possible the old only-ioprio user interface,
* 0 is used as an escape ioprio value for weights (numerically) equal or
- * larger than IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF.
+ * larger than IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF.
*/
static unsigned short bfq_weight_to_ioprio(int weight)
{
return max_t(int, 0,
- IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF - weight);
+ IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF - weight);
}
static void bfq_get_entity(struct bfq_entity *entity)
diff --git a/block/ioprio.c b/block/ioprio.c
index bee628f9f1b2..ca6b136c5586 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)
fallthrough;
/* rt has prio field too */
case IOPRIO_CLASS_BE:
- if (data >= IOPRIO_BE_NR || data < 0)
+ if (data >= IOPRIO_NR_LEVELS || data < 0)
return -EINVAL;
-
break;
case IOPRIO_CLASS_IDLE:
break;
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 6642246206bd..daad532a4e2b 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -378,7 +378,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
ret = kstrtol(name, 10, &data);
if (ret)
return ret;
- if (data >= IOPRIO_BE_NR || data < 0)
+ if (data >= IOPRIO_NR_LEVELS || data < 0)
return -EINVAL;
cprc->ckpt_thread_ioprio = IOPRIO_PRIO_VALUE(class, data);
diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
index abc40965aa96..b9d48744dacb 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -31,9 +31,9 @@ enum {
};
/*
- * 8 best effort priority levels are supported
+ * The RT an BE priority classes support up to 8 priority levels.
*/
-#define IOPRIO_BE_NR 8
+#define IOPRIO_NR_LEVELS 8
enum {
IOPRIO_WHO_PROCESS = 1,
--
2.31.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] block: fix default IO priority handling
2021-08-06 5:11 [PATCH v2 0/4] IO priority fixes and improvements Damien Le Moal
` (2 preceding siblings ...)
2021-08-06 5:11 ` [PATCH v2 3/4] block: rename IOPRIO_BE_NR Damien Le Moal
@ 2021-08-06 5:11 ` Damien Le Moal
2021-08-06 6:39 ` Hannes Reinecke
3 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 5:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Paolo Valente, linux-f2fs-devel,
Jaegeuk Kim, Chao Yu
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 <damien.lemoal@wdc.com>
---
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(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index d5824cab34d7..a07d630c6972 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5408,7 +5408,7 @@ static struct bfq_queue **bfq_async_queue_prio(struct bfq_data *bfqd,
case IOPRIO_CLASS_RT:
return &bfqg->async_bfqq[0][ioprio];
case IOPRIO_CLASS_NONE:
- ioprio = IOPRIO_NORM;
+ ioprio = IOPRIO_BE_NORM;
fallthrough;
case IOPRIO_CLASS_BE:
return &bfqg->async_bfqq[1][ioprio];
diff --git a/block/ioprio.c b/block/ioprio.c
index ca6b136c5586..0e4ff245f2bf 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -170,7 +170,7 @@ static int get_task_ioprio(struct task_struct *p)
ret = security_task_getioprio(p);
if (ret)
goto out;
- ret = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, IOPRIO_NORM);
+ ret = IOPRIO_DEFAULT;
task_lock(p);
if (p->io_context)
ret = p->io_context->ioprio;
@@ -182,9 +182,9 @@ static int get_task_ioprio(struct task_struct *p)
int ioprio_best(unsigned short aprio, unsigned short bprio)
{
if (!ioprio_valid(aprio))
- aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM);
+ aprio = IOPRIO_DEFAULT;
if (!ioprio_valid(bprio))
- bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM);
+ bprio = IOPRIO_DEFAULT;
return min(aprio, bprio);
}
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index e9d9ad47f70f..0fbbff0b3edb 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -662,7 +662,7 @@ static struct request *nvme_nvm_alloc_request(struct request_queue *q,
if (rqd->bio)
blk_rq_append_bio(rq, rqd->bio);
else
- rq->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM);
+ rq->ioprio = IOPRIO_DEFAULT;
return rq;
}
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 9b3a6d8172b4..2837c3a0d2e1 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -8,6 +8,11 @@
#include <uapi/linux/ioprio.h>
+/*
+ * Default IO priority.
+ */
+#define IOPRIO_DEFAULT IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM)
+
/*
* Check that a priority value has a valid class.
*/
@@ -50,7 +55,7 @@ static inline int get_current_ioprio(void)
if (ioc)
return ioc->ioprio;
- return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+ return IOPRIO_DEFAULT;
}
/*
diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
index b9d48744dacb..ccc633af44d5 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -42,8 +42,8 @@ enum {
};
/*
- * Fallback BE priority
+ * Fallback BE priority level.
*/
-#define IOPRIO_NORM 4
+#define IOPRIO_BE_NORM 4
#endif /* _UAPI_LINUX_IOPRIO_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data()
2021-08-06 5:11 ` [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data() Damien Le Moal
@ 2021-08-06 6:33 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2021-08-06 6:33 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
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: <stable@vger.kernel.org>
> Fixes: aee69d78dec0 ("block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler")
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] block: fix ioprio interface
2021-08-06 5:11 ` [PATCH v2 2/4] block: fix ioprio interface Damien Le Moal
@ 2021-08-06 6:35 ` Hannes Reinecke
2021-08-06 6:57 ` Damien Le Moal
0 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2021-08-06 6:35 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
On 8/6/21 7:11 AM, Damien Le Moal wrote:
> 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 <damien.lemoal@wdc.com>
> ---
> include/linux/ioprio.h | 15 ++++++++++++---
> include/uapi/linux/ioprio.h | 19 +++++++++++--------
> 2 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index ef9ad4fb245f..9b3a6d8172b4 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -8,6 +8,16 @@
>
> #include <uapi/linux/ioprio.h>
>
> +/*
> + * Check that a priority value has a valid class.
> + */
> +static inline bool ioprio_valid(unsigned short ioprio)
Wouldn't it be better to use 'u16' here as type, as we're relying on the
number of bits?
> +{
> + 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;
> }
>
> /*
> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> index 77b17e08b0da..abc40965aa96 100644
> --- a/include/uapi/linux/ioprio.h
> +++ b/include/uapi/linux/ioprio.h
> @@ -5,12 +5,15 @@
> /*
> * Gives us 8 prio classes with 13-bits of data for each class
> */
> -#define IOPRIO_CLASS_SHIFT (13)
> +#define IOPRIO_CLASS_SHIFT 13
> +#define IOPRIO_CLASS_MASK 0x07
> #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1)
>
> -#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT)
> -#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK)
> -#define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data)
> +#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_SHIFT) | ((data) & IOPRIO_PRIO_MASK))
>
> /*
> * These are the io priority groups as implemented by CFQ. RT is the realtime
> @@ -23,14 +26,14 @@ enum {
> IOPRIO_CLASS_RT,
> IOPRIO_CLASS_BE,
> IOPRIO_CLASS_IDLE,
> -};
>
> -#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
> + IOPRIO_CLASS_MAX,
> +};
>
> /*
> * 8 best effort priority levels are supported
> */
> -#define IOPRIO_BE_NR (8)
> +#define IOPRIO_BE_NR 8
>
> enum {
> IOPRIO_WHO_PROCESS = 1,
> @@ -41,6 +44,6 @@ enum {
> /*
> * Fallback BE prioritye@su
> */
> -#define IOPRIO_NORM (4)
> +#define IOPRIO_NORM 4
>
> #endif /* _UAPI_LINUX_IOPRIO_H */
>
Other than that:
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] block: rename IOPRIO_BE_NR
2021-08-06 5:11 ` [PATCH v2 3/4] block: rename IOPRIO_BE_NR Damien Le Moal
@ 2021-08-06 6:37 ` Hannes Reinecke
2021-08-06 6:52 ` Damien Le Moal
0 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2021-08-06 6:37 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
On 8/6/21 7:11 AM, Damien Le Moal wrote:
> 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 <damien.lemoal@wdc.com>
> ---
> 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(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1f38d75524ae..d5824cab34d7 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2505,7 +2505,7 @@ void bfq_end_wr_async_queues(struct bfq_data *bfqd,
> int i, j;
>
> for (i = 0; i < 2; i++)
> - for (j = 0; j < IOPRIO_BE_NR; j++)
> + for (j = 0; j < IOPRIO_NR_LEVELS; j++)
> if (bfqg->async_bfqq[i][j])
> bfq_bfqq_end_wr(bfqg->async_bfqq[i][j]);
> if (bfqg->async_idle_bfqq)
> @@ -5290,10 +5290,10 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
> break;
> }
>
> - if (bfqq->new_ioprio >= IOPRIO_BE_NR) {
> + if (bfqq->new_ioprio >= IOPRIO_NR_LEVELS) {
> pr_crit("bfq_set_next_ioprio_data: new_ioprio %d\n",
> bfqq->new_ioprio);
> - bfqq->new_ioprio = IOPRIO_BE_NR - 1;
> + bfqq->new_ioprio = IOPRIO_NR_LEVELS - 1;
> }
>
> bfqq->entity.new_weight = bfq_ioprio_to_weight(bfqq->new_ioprio);
> @@ -6822,7 +6822,7 @@ void bfq_put_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg)
> int i, j;
>
> for (i = 0; i < 2; i++)
> - for (j = 0; j < IOPRIO_BE_NR; j++)
> + for (j = 0; j < IOPRIO_NR_LEVELS; j++)
> __bfq_put_async_bfqq(bfqd, &bfqg->async_bfqq[i][j]);
>
> __bfq_put_async_bfqq(bfqd, &bfqg->async_idle_bfqq);
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index 99c2a3cb081e..385e28a843d1 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -931,7 +931,7 @@ struct bfq_group {
>
> void *bfqd;
>
> - struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
> + struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
> struct bfq_queue *async_idle_bfqq;
>
> struct bfq_entity *my_entity;
> @@ -948,7 +948,7 @@ struct bfq_group {
> struct bfq_entity entity;
> struct bfq_sched_data sched_data;
>
> - struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
> + struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
> struct bfq_queue *async_idle_bfqq;
>
> struct rb_root rq_pos_tree;
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index 7a462df71f68..b74cc0da118e 100644
> --- a/block/bfq-wf2q.c
> +++ b/block/bfq-wf2q.c
> @@ -505,7 +505,7 @@ static void bfq_active_insert(struct bfq_service_tree *st,
> */
> unsigned short bfq_ioprio_to_weight(int ioprio)
> {
> - return (IOPRIO_BE_NR - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
> + return (IOPRIO_NR_LEVELS - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
> }
>
> /**
> @@ -514,12 +514,12 @@ unsigned short bfq_ioprio_to_weight(int ioprio)
> *
> * To preserve as much as possible the old only-ioprio user interface,
> * 0 is used as an escape ioprio value for weights (numerically) equal or
> - * larger than IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF.
> + * larger than IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF.
> */
> static unsigned short bfq_weight_to_ioprio(int weight)
> {
> return max_t(int, 0,
> - IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF - weight);
> + IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF - weight);
> }
>
> static void bfq_get_entity(struct bfq_entity *entity)
> diff --git a/block/ioprio.c b/block/ioprio.c
> index bee628f9f1b2..ca6b136c5586 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)
> fallthrough;
> /* rt has prio field too */
> case IOPRIO_CLASS_BE:
> - if (data >= IOPRIO_BE_NR || data < 0)
> + if (data >= IOPRIO_NR_LEVELS || data < 0)
> return -EINVAL;
> -
> break;
> case IOPRIO_CLASS_IDLE:
> break;
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 6642246206bd..daad532a4e2b 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -378,7 +378,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
> ret = kstrtol(name, 10, &data);
> if (ret)
> return ret;
> - if (data >= IOPRIO_BE_NR || data < 0)
> + if (data >= IOPRIO_NR_LEVELS || data < 0)
> return -EINVAL;
>
> cprc->ckpt_thread_ioprio = IOPRIO_PRIO_VALUE(class, data);
> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> index abc40965aa96..b9d48744dacb 100644
> --- a/include/uapi/linux/ioprio.h
> +++ b/include/uapi/linux/ioprio.h
> @@ -31,9 +31,9 @@ enum {
> };
>
> /*
> - * 8 best effort priority levels are supported
> + * The RT an BE priority classes support up to 8 priority levels.
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] block: fix default IO priority handling
2021-08-06 5:11 ` [PATCH v2 4/4] block: fix default IO priority handling Damien Le Moal
@ 2021-08-06 6:39 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2021-08-06 6:39 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
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 <damien.lemoal@wdc.com>
> ---
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] block: rename IOPRIO_BE_NR
2021-08-06 6:37 ` Hannes Reinecke
@ 2021-08-06 6:52 ` Damien Le Moal
0 siblings, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 6:52 UTC (permalink / raw)
To: Hannes Reinecke, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
On 2021/08/06 15:38, Hannes Reinecke wrote:
> On 8/6/21 7:11 AM, Damien Le Moal wrote:
>> 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 <damien.lemoal@wdc.com>
>> ---
>> 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(-)
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 1f38d75524ae..d5824cab34d7 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -2505,7 +2505,7 @@ void bfq_end_wr_async_queues(struct bfq_data *bfqd,
>> int i, j;
>>
>> for (i = 0; i < 2; i++)
>> - for (j = 0; j < IOPRIO_BE_NR; j++)
>> + for (j = 0; j < IOPRIO_NR_LEVELS; j++)
>> if (bfqg->async_bfqq[i][j])
>> bfq_bfqq_end_wr(bfqg->async_bfqq[i][j]);
>> if (bfqg->async_idle_bfqq)
>> @@ -5290,10 +5290,10 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
>> break;
>> }
>>
>> - if (bfqq->new_ioprio >= IOPRIO_BE_NR) {
>> + if (bfqq->new_ioprio >= IOPRIO_NR_LEVELS) {
>> pr_crit("bfq_set_next_ioprio_data: new_ioprio %d\n",
>> bfqq->new_ioprio);
>> - bfqq->new_ioprio = IOPRIO_BE_NR - 1;
>> + bfqq->new_ioprio = IOPRIO_NR_LEVELS - 1;
>> }
>>
>> bfqq->entity.new_weight = bfq_ioprio_to_weight(bfqq->new_ioprio);
>> @@ -6822,7 +6822,7 @@ void bfq_put_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg)
>> int i, j;
>>
>> for (i = 0; i < 2; i++)
>> - for (j = 0; j < IOPRIO_BE_NR; j++)
>> + for (j = 0; j < IOPRIO_NR_LEVELS; j++)
>> __bfq_put_async_bfqq(bfqd, &bfqg->async_bfqq[i][j]);
>>
>> __bfq_put_async_bfqq(bfqd, &bfqg->async_idle_bfqq);
>> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
>> index 99c2a3cb081e..385e28a843d1 100644
>> --- a/block/bfq-iosched.h
>> +++ b/block/bfq-iosched.h
>> @@ -931,7 +931,7 @@ struct bfq_group {
>>
>> void *bfqd;
>>
>> - struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
>> + struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
>> struct bfq_queue *async_idle_bfqq;
>>
>> struct bfq_entity *my_entity;
>> @@ -948,7 +948,7 @@ struct bfq_group {
>> struct bfq_entity entity;
>> struct bfq_sched_data sched_data;
>>
>> - struct bfq_queue *async_bfqq[2][IOPRIO_BE_NR];
>> + struct bfq_queue *async_bfqq[2][IOPRIO_NR_LEVELS];
>> struct bfq_queue *async_idle_bfqq;
>>
>> struct rb_root rq_pos_tree;
>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>> index 7a462df71f68..b74cc0da118e 100644
>> --- a/block/bfq-wf2q.c
>> +++ b/block/bfq-wf2q.c
>> @@ -505,7 +505,7 @@ static void bfq_active_insert(struct bfq_service_tree *st,
>> */
>> unsigned short bfq_ioprio_to_weight(int ioprio)
>> {
>> - return (IOPRIO_BE_NR - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
>> + return (IOPRIO_NR_LEVELS - ioprio) * BFQ_WEIGHT_CONVERSION_COEFF;
>> }
>>
>> /**
>> @@ -514,12 +514,12 @@ unsigned short bfq_ioprio_to_weight(int ioprio)
>> *
>> * To preserve as much as possible the old only-ioprio user interface,
>> * 0 is used as an escape ioprio value for weights (numerically) equal or
>> - * larger than IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF.
>> + * larger than IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF.
>> */
>> static unsigned short bfq_weight_to_ioprio(int weight)
>> {
>> return max_t(int, 0,
>> - IOPRIO_BE_NR * BFQ_WEIGHT_CONVERSION_COEFF - weight);
>> + IOPRIO_NR_LEVELS * BFQ_WEIGHT_CONVERSION_COEFF - weight);
>> }
>>
>> static void bfq_get_entity(struct bfq_entity *entity)
>> diff --git a/block/ioprio.c b/block/ioprio.c
>> index bee628f9f1b2..ca6b136c5586 100644
>> --- a/block/ioprio.c
>> +++ b/block/ioprio.c
>> @@ -74,9 +74,8 @@ int ioprio_check_cap(int ioprio)
>> fallthrough;
>> /* rt has prio field too */
>> case IOPRIO_CLASS_BE:
>> - if (data >= IOPRIO_BE_NR || data < 0)
>> + if (data >= IOPRIO_NR_LEVELS || data < 0)
>> return -EINVAL;
>> -
>> break;
>> case IOPRIO_CLASS_IDLE:
>> break;
>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>> index 6642246206bd..daad532a4e2b 100644
>> --- a/fs/f2fs/sysfs.c
>> +++ b/fs/f2fs/sysfs.c
>> @@ -378,7 +378,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>> ret = kstrtol(name, 10, &data);
>> if (ret)
>> return ret;
>> - if (data >= IOPRIO_BE_NR || data < 0)
>> + if (data >= IOPRIO_NR_LEVELS || data < 0)
>> return -EINVAL;
>>
>> cprc->ckpt_thread_ioprio = IOPRIO_PRIO_VALUE(class, data);
>> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
>> index abc40965aa96..b9d48744dacb 100644
>> --- a/include/uapi/linux/ioprio.h
>> +++ b/include/uapi/linux/ioprio.h
>> @@ -31,9 +31,9 @@ enum {
>> };
>>
>> /*
>> - * 8 best effort priority levels are supported
>> + * The RT an BE priority classes support up to 8 priority levels.
>
> This sentence no verb :-)
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
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] block: fix ioprio interface
2021-08-06 6:35 ` Hannes Reinecke
@ 2021-08-06 6:57 ` Damien Le Moal
2021-08-06 8:38 ` Hannes Reinecke
0 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2021-08-06 6:57 UTC (permalink / raw)
To: Hannes Reinecke, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
On 2021/08/06 15:35, Hannes Reinecke wrote:
> On 8/6/21 7:11 AM, Damien Le Moal wrote:
>> 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 <damien.lemoal@wdc.com>
>> ---
>> include/linux/ioprio.h | 15 ++++++++++++---
>> include/uapi/linux/ioprio.h | 19 +++++++++++--------
>> 2 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
>> index ef9ad4fb245f..9b3a6d8172b4 100644
>> --- a/include/linux/ioprio.h
>> +++ b/include/linux/ioprio.h
>> @@ -8,6 +8,16 @@
>>
>> #include <uapi/linux/ioprio.h>
>>
>> +/*
>> + * Check that a priority value has a valid class.
>> + */
>> +static inline bool ioprio_valid(unsigned short ioprio)
>
> 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 ?
>
>> +{
>> + 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;
>> }
>>
>> /*
>> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
>> index 77b17e08b0da..abc40965aa96 100644
>> --- a/include/uapi/linux/ioprio.h
>> +++ b/include/uapi/linux/ioprio.h
>> @@ -5,12 +5,15 @@
>> /*
>> * Gives us 8 prio classes with 13-bits of data for each class
>> */
>> -#define IOPRIO_CLASS_SHIFT (13)
>> +#define IOPRIO_CLASS_SHIFT 13
>> +#define IOPRIO_CLASS_MASK 0x07
>> #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1)
>>
>> -#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT)
>> -#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK)
>> -#define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data)
>> +#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_SHIFT) | ((data) & IOPRIO_PRIO_MASK))
>>
>> /*
>> * These are the io priority groups as implemented by CFQ. RT is the realtime
>> @@ -23,14 +26,14 @@ enum {
>> IOPRIO_CLASS_RT,
>> IOPRIO_CLASS_BE,
>> IOPRIO_CLASS_IDLE,
>> -};
>>
>> -#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
>> + IOPRIO_CLASS_MAX,
>> +};
>>
>> /*
>> * 8 best effort priority levels are supported
>> */
>> -#define IOPRIO_BE_NR (8)
>> +#define IOPRIO_BE_NR 8
>>
>> enum {
>> IOPRIO_WHO_PROCESS = 1,
>> @@ -41,6 +44,6 @@ enum {
>> /*
>> * Fallback BE prioritye@su
>> */
>> -#define IOPRIO_NORM (4)
>> +#define IOPRIO_NORM 4
>>
>> #endif /* _UAPI_LINUX_IOPRIO_H */
>>
> Other than that:
>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
>
> Cheers,
>
> Hannes
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] block: fix ioprio interface
2021-08-06 6:57 ` Damien Le Moal
@ 2021-08-06 8:38 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2021-08-06 8:38 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe, linux-block, Paolo Valente,
linux-f2fs-devel, Jaegeuk Kim, Chao Yu
On 8/6/21 8:57 AM, Damien Le Moal wrote:
> On 2021/08/06 15:35, Hannes Reinecke wrote:
>> On 8/6/21 7:11 AM, Damien Le Moal wrote:
>>> 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 <damien.lemoal@wdc.com>
>>> ---
>>> include/linux/ioprio.h | 15 ++++++++++++---
>>> include/uapi/linux/ioprio.h | 19 +++++++++++--------
>>> 2 files changed, 23 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
>>> index ef9ad4fb245f..9b3a6d8172b4 100644
>>> --- a/include/linux/ioprio.h
>>> +++ b/include/linux/ioprio.h
>>> @@ -8,6 +8,16 @@
>>>
>>> #include <uapi/linux/ioprio.h>
>>>
>>> +/*
>>> + * Check that a priority value has a valid class.
>>> + */
>>> +static inline bool ioprio_valid(unsigned short ioprio)
>>
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-06 8:38 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 5:11 [PATCH v2 0/4] IO priority fixes and improvements Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 1/4] block: bfq: fix bfq_set_next_ioprio_data() Damien Le Moal
2021-08-06 6:33 ` Hannes Reinecke
2021-08-06 5:11 ` [PATCH v2 2/4] block: fix ioprio interface Damien Le Moal
2021-08-06 6:35 ` Hannes Reinecke
2021-08-06 6:57 ` Damien Le Moal
2021-08-06 8:38 ` Hannes Reinecke
2021-08-06 5:11 ` [PATCH v2 3/4] block: rename IOPRIO_BE_NR Damien Le Moal
2021-08-06 6:37 ` Hannes Reinecke
2021-08-06 6:52 ` Damien Le Moal
2021-08-06 5:11 ` [PATCH v2 4/4] block: fix default IO priority handling Damien Le Moal
2021-08-06 6:39 ` Hannes Reinecke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).