diff mbox series

[v7,01/19] ioprio: cleanup interface definition

Message ID 20230511011356.227789-2-nks@flawful.org (mailing list archive)
State New, archived
Headers show
Series Add Command Duration Limits support | expand

Commit Message

Niklas Cassel May 11, 2023, 1:13 a.m. UTC 
From: Damien Le Moal <dlemoal@kernel.org>

The IO priority user interface defines the 16-bits ioprio values as
the combination of the upper 3-bits for an IO priority class and the
lower 13-bits as priority data. However, the kernel only uses the
lower 3-bits of the priority data to define priority levels for the RT
and BE priority classes. The data part of an ioprio value is completely
ignored for the IDLE and NONE classes. This is enforced by checks done
in ioprio_check_cap(), which is called for all paths that allow defining
an IO priority for IOs: the per-context ioprio_set() system call, aio
interface and io-uring interface.

Clarify this fact in the uapi ioprio.h header file and introduce the
IOPRIO_PRIO_LEVEL_MASK and IOPRIO_PRIO_LEVEL() macros for users to
define and get priority levels in an ioprio value. The coarser macro
IOPRIO_PRIO_DATA() is retained for backward compatibility with old
applications already using it. There is no functional change introduced
with this.

In-kernel users of the IOPRIO_PRIO_DATA() macro which are explicitly
handling IO priority data as a priority level are modified to use the
new IOPRIO_PRIO_LEVEL() macro without any functional change. Since f2fs
is the only user of this macro not explicitly using that value as a
priority level, it is left unchanged.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 block/bfq-iosched.c         |  8 ++++----
 block/ioprio.c              |  6 +++---
 include/uapi/linux/ioprio.h | 19 ++++++++++++++-----
 3 files changed, 21 insertions(+), 12 deletions(-)

Comments

Alexander Gordeev June 7, 2023, 1:10 p.m. UTC | #1 
On Thu, May 11, 2023 at 03:13:34AM +0200, Damien Le Moal wrote:
...
Hi Damien et al,

This patch aka commit eca2040972b4 ("scsi: block: ioprio:
Clean up interface definition") in -next breaks LTP test
(at least on s390):

# ./ioprio_set03
tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
ioprio_set03.c:39: TFAIL: ioprio_set IOPRIO_CLASS_BE prio 8 should not work
ioprio_set03.c:47: TINFO: tested illegal priority with class NONE
ioprio_set03.c:50: TPASS: returned correct error for wrong prio: EINVAL (22)

Summary:
passed   1
failed   1
broken   0
skipped  0
warnings 0

Thanks!
Niklas Cassel June 7, 2023, 2:52 p.m. UTC | #2 
On Wed, Jun 07, 2023 at 03:10:59PM +0200, Alexander Gordeev wrote:
> On Thu, May 11, 2023 at 03:13:34AM +0200, Damien Le Moal wrote:
> ...
> Hi Damien et al,
> 
> This patch aka commit eca2040972b4 ("scsi: block: ioprio:
> Clean up interface definition") in -next breaks LTP test
> (at least on s390):
> 
> # ./ioprio_set03
> tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
> ioprio_set03.c:39: TFAIL: ioprio_set IOPRIO_CLASS_BE prio 8 should not work
> ioprio_set03.c:47: TINFO: tested illegal priority with class NONE
> ioprio_set03.c:50: TPASS: returned correct error for wrong prio: EINVAL (22)
> 
> Summary:
> passed   1
> failed   1
> broken   0
> skipped  0
> warnings 0
> 
> Thanks!

Hello Alex,

The LTP failure requires the following patches to LTP:
https://lore.kernel.org/ltp/CACRpkdYdtgcLSqovV-HwZ9PvSXFBZv5wdU3KzasMR1wHgah4kg@mail.gmail.com/T/#t

Unfortunately, the LTP patches seem to not be available on the archive.


However, considering that the LTP patches decided to keep the test case that
sets a priority level out of range, they also require this block layer patch:
https://lore.kernel.org/linux-block/20230530061307.525644-1-dlemoal@kernel.org/T/

The patch would have to go via Martin's tree, since that is where commit
eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") is queued.


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 3164e3177965..3067b75f3fd0 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5524,16 +5524,16 @@  bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
 		bfqq->new_ioprio_class = task_nice_ioclass(tsk);
 		break;
 	case IOPRIO_CLASS_RT:
-		bfqq->new_ioprio = IOPRIO_PRIO_DATA(bic->ioprio);
+		bfqq->new_ioprio = IOPRIO_PRIO_LEVEL(bic->ioprio);
 		bfqq->new_ioprio_class = IOPRIO_CLASS_RT;
 		break;
 	case IOPRIO_CLASS_BE:
-		bfqq->new_ioprio = IOPRIO_PRIO_DATA(bic->ioprio);
+		bfqq->new_ioprio = IOPRIO_PRIO_LEVEL(bic->ioprio);
 		bfqq->new_ioprio_class = IOPRIO_CLASS_BE;
 		break;
 	case IOPRIO_CLASS_IDLE:
 		bfqq->new_ioprio_class = IOPRIO_CLASS_IDLE;
-		bfqq->new_ioprio = 7;
+		bfqq->new_ioprio = IOPRIO_NR_LEVELS - 1;
 		break;
 	}
 
@@ -5830,7 +5830,7 @@  static struct bfq_queue *bfq_get_queue(struct bfq_data *bfqd,
 				       struct bfq_io_cq *bic,
 				       bool respawn)
 {
-	const int ioprio = IOPRIO_PRIO_DATA(bic->ioprio);
+	const int ioprio = IOPRIO_PRIO_LEVEL(bic->ioprio);
 	const int ioprio_class = IOPRIO_PRIO_CLASS(bic->ioprio);
 	struct bfq_queue **async_bfqq = NULL;
 	struct bfq_queue *bfqq;
diff --git a/block/ioprio.c b/block/ioprio.c
index 32a456b45804..f0d9e818abc5 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -33,7 +33,7 @@ 
 int ioprio_check_cap(int ioprio)
 {
 	int class = IOPRIO_PRIO_CLASS(ioprio);
-	int data = IOPRIO_PRIO_DATA(ioprio);
+	int level = IOPRIO_PRIO_LEVEL(ioprio);
 
 	switch (class) {
 		case IOPRIO_CLASS_RT:
@@ -49,13 +49,13 @@  int ioprio_check_cap(int ioprio)
 			fallthrough;
 			/* rt has prio field too */
 		case IOPRIO_CLASS_BE:
-			if (data >= IOPRIO_NR_LEVELS || data < 0)
+			if (level >= IOPRIO_NR_LEVELS)
 				return -EINVAL;
 			break;
 		case IOPRIO_CLASS_IDLE:
 			break;
 		case IOPRIO_CLASS_NONE:
-			if (data)
+			if (level)
 				return -EINVAL;
 			break;
 		default:
diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
index f70f2596a6bf..4444b4e4fdad 100644
--- a/include/uapi/linux/ioprio.h
+++ b/include/uapi/linux/ioprio.h
@@ -17,7 +17,7 @@ 
 	 ((data) & IOPRIO_PRIO_MASK))
 
 /*
- * These are the io priority groups as implemented by the BFQ and mq-deadline
+ * These are the io priority classes as implemented by the BFQ and mq-deadline
  * schedulers. RT is the realtime class, it always gets premium service. For
  * ATA disks supporting NCQ IO priority, RT class IOs will be processed using
  * high priority NCQ commands. BE is the best-effort scheduling class, the
@@ -32,11 +32,20 @@  enum {
 };
 
 /*
- * The RT and BE priority classes both support up to 8 priority levels.
+ * The RT and BE priority classes both support up to 8 priority levels that
+ * can be specified using the lower 3-bits of the priority data.
  */
-#define IOPRIO_NR_LEVELS	8
-#define IOPRIO_BE_NR		IOPRIO_NR_LEVELS
+#define IOPRIO_LEVEL_NR_BITS		3
+#define IOPRIO_NR_LEVELS		(1 << IOPRIO_LEVEL_NR_BITS)
+#define IOPRIO_LEVEL_MASK		(IOPRIO_NR_LEVELS - 1)
+#define IOPRIO_PRIO_LEVEL(ioprio)	((ioprio) & IOPRIO_LEVEL_MASK)
 
+#define IOPRIO_BE_NR			IOPRIO_NR_LEVELS
+
+/*
+ * Possible values for the "which" argument of the ioprio_get() and
+ * ioprio_set() system calls (see "man ioprio_set").
+ */
 enum {
 	IOPRIO_WHO_PROCESS = 1,
 	IOPRIO_WHO_PGRP,
@@ -44,7 +53,7 @@  enum {
 };
 
 /*
- * Fallback BE priority level.
+ * Fallback BE class priority level.
  */
 #define IOPRIO_NORM	4
 #define IOPRIO_BE_NORM	IOPRIO_NORM