[PATCH 0/3] IO priority fixes and improvements

[PATCH 0/3] IO priority fixes and improvements

[Damien Le Moal]

This series fix problems with IO priority values handling and renames
one macro to clarify the intended usage.

Damien Le Moal (3):
  block: bfq: fix bfq_set_next_ioprio_data()
  block: fix ioprio interface
  block: rename IOPRIO_BE_NR

 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/linux/ioprio.h      |  5 ++---
 include/uapi/linux/ioprio.h | 19 ++++++++++++++-----
 7 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH 1/3] block: bfq: fix bfq_set_next_ioprio_data()

[Damien Le Moal]

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

[PATCH 2/3] block: fix ioprio interface

[Damien Le Moal]

An iocb aio_reqprio field is 16 bits only, but often handled as an int
in the block layer. E.g. ioprio_check_cap() takes an int as argument.
However, with such int casting function calls, the upper 16 bits of the
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:
* Update the mention of CFQ in the comment describing priority classes
  and mention BFQ and mq-deadline.
* Change the argument name of the IOPRIO_PRIO_CLASS() and
  IOPRIO_PRIO_DATA() macros from "mask" to "val" 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 as defined by
  the IOPRIO_CLASS_MAX enum value.
* 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      |  5 ++---
 include/uapi/linux/ioprio.h | 15 ++++++++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index ef9ad4fb245f..997641211cca 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -25,10 +25,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..27dc7fb0ba12 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -6,10 +6,12 @@
  * 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_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_CLASS(val)	\
+	(((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK)
+#define IOPRIO_PRIO_DATA(val)	((val) & IOPRIO_PRIO_MASK)
 #define IOPRIO_PRIO_VALUE(class, data)	(((class) << IOPRIO_CLASS_SHIFT) | data)
 
 /*
@@ -23,9 +25,16 @@ enum {
 	IOPRIO_CLASS_RT,
 	IOPRIO_CLASS_BE,
 	IOPRIO_CLASS_IDLE,
+
+	IOPRIO_CLASS_MAX,
 };
 
-#define ioprio_valid(mask)	(IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
+static inline bool ioprio_valid(unsigned short ioprio)
+{
+	unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
+
+	return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX;
+}
 
 /*
  * 8 best effort priority levels are supported
-- 
2.31.1

[PATCH 3/3] block: rename IOPRIO_BE_NR

[Damien Le Moal]

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

Rename IOPRIO_BE_NR to 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 27dc7fb0ba12..fe47b58c4274 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -37,9 +37,9 @@ static inline bool ioprio_valid(unsigned short ioprio)
 }
 
 /*
- * 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

Re: [PATCH 3/3] block: rename IOPRIO_BE_NR

[Bart Van Assche]

On 8/2/21 2:21 AM, Damien Le Moal wrote:
>   /*
> - * 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)

Is this kind of change acceptable in a UAPI header? Can this change 
break the build of user space applications?

If this change is acceptable, how about the name IOPRIO_NR_BE_LEVELS? 
Additionally, please leave out the parentheses since these are not 
necessary.

Thanks,

Bart.

Re: [PATCH 2/3] block: fix ioprio interface

[Bart Van Assche]

On 8/2/21 2:21 AM, Damien Le Moal wrote:
> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> index 77b17e08b0da..27dc7fb0ba12 100644
> --- a/include/uapi/linux/ioprio.h
> +++ b/include/uapi/linux/ioprio.h
> @@ -6,10 +6,12 @@
>    * 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_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_CLASS(val)	\
> +	(((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK)
> +#define IOPRIO_PRIO_DATA(val)	((val) & IOPRIO_PRIO_MASK)
>   #define IOPRIO_PRIO_VALUE(class, data)	(((class) << IOPRIO_CLASS_SHIFT) | data)
>   
>   /*
> @@ -23,9 +25,16 @@ enum {
>   	IOPRIO_CLASS_RT,
>   	IOPRIO_CLASS_BE,
>   	IOPRIO_CLASS_IDLE,
> +
> +	IOPRIO_CLASS_MAX,
>   };
>   
> -#define ioprio_valid(mask)	(IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
> +static inline bool ioprio_valid(unsigned short ioprio)
> +{
> +	unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
> +
> +	return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX;
> +}
>   
>   /*
>    * 8 best effort priority levels are supported

Are there any plans to use ioprio_valid() in user space applications? If 
not, should this function perhaps be defined in a kernel-only header?

Thanks,

Bart.

Re: [PATCH 3/3] block: rename IOPRIO_BE_NR

[Damien Le Moal]

On 2021/08/03 0:56, Bart Van Assche wrote:
> On 8/2/21 2:21 AM, Damien Le Moal wrote:
>>   /*
>> - * 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)
> 
> Is this kind of change acceptable in a UAPI header? Can this change 
> break the build of user space applications?

These definitions moving to a uapi header is new in 5.15. So they are not
currently in an uapi header. This is our chance to clean things up.

> 
> If this change is acceptable, how about the name IOPRIO_NR_BE_LEVELS? 
> Additionally, please leave out the parentheses since these are not 
> necessary.

As the commit message mentions, the 8 possible priority levels are used for the
RT class too. So it is not just about the BE class. That is why I would prefer
removing "BE" from the macro name. Or, we need 2 macro: IOPRIO_NR_BE_LEVELS and
IOPRIO_NR_RT_LEVELS. But that will only force adding duplicated checks in
functions like ioprio_check_cap().

> 
> Thanks,
> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] block: fix ioprio interface

[Damien Le Moal]

On 2021/08/03 0:58, Bart Van Assche wrote:
> On 8/2/21 2:21 AM, Damien Le Moal wrote:
>> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
>> index 77b17e08b0da..27dc7fb0ba12 100644
>> --- a/include/uapi/linux/ioprio.h
>> +++ b/include/uapi/linux/ioprio.h
>> @@ -6,10 +6,12 @@
>>    * 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_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_CLASS(val)	\
>> +	(((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK)
>> +#define IOPRIO_PRIO_DATA(val)	((val) & IOPRIO_PRIO_MASK)
>>   #define IOPRIO_PRIO_VALUE(class, data)	(((class) << IOPRIO_CLASS_SHIFT) | data)
>>   
>>   /*
>> @@ -23,9 +25,16 @@ enum {
>>   	IOPRIO_CLASS_RT,
>>   	IOPRIO_CLASS_BE,
>>   	IOPRIO_CLASS_IDLE,
>> +
>> +	IOPRIO_CLASS_MAX,
>>   };
>>   
>> -#define ioprio_valid(mask)	(IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
>> +static inline bool ioprio_valid(unsigned short ioprio)
>> +{
>> +	unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
>> +
>> +	return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX;
>> +}
>>   
>>   /*
>>    * 8 best effort priority levels are supported
> 
> Are there any plans to use ioprio_valid() in user space applications? If 
> not, should this function perhaps be defined in a kernel-only header?

Good point. I wondered the same. I think it may be better to leave that one in
include/linux/ioprio.h instead of moving it to the uapi header.

Jens,

Thoughts ?

> 
> Thanks,
> 
> Bart.
> 
> 
> 


-- 
Damien Le Moal
Western Digital Research