[PATCH v2 0/8] cmdprio cleanup series

[PATCH v2 0/8] cmdprio cleanup series

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hello Jens,

Here is a cleanup series that improves the cmdprio_percentage and
cmdprio_bssplit options in libaio and io_uring ioengines.

I'm currently working on adding support for multiple priority class +
priority level combinations, however that series is not quite ready yet.

Getting this cleanup series merged would make it easier to get multiple
priority class + priority level combinations added.
(Because AFAICT we wouldn't need any further changes to engines/io_uring.c
or engines/libaio.c after this. Most changes will be to cmdprio.c and
stat.c.)


Changes since v1:
-Incorporated review comments by Damien.


Kind regards,
Niklas

Niklas Cassel (8):
  docs: update cmdprio_percentage documentation
  cmdprio: move cmdprio function definitions to a new cmdprio.c file
  cmdprio: do not allocate memory for unused data direction
  io_uring: set async IO priority to td->ioprio in fio_ioring_prep()
  libaio,io_uring: rename prio_prep() to include cmdprio in the name
  libaio,io_uring: move common cmdprio_prep() code to cmdprio
  cmdprio: add mode to make the logic easier to reason about
  libaio,io_uring: make it possible to cleanup cmdprio malloced data

 HOWTO              |   5 +-
 Makefile           |   6 ++
 engines/cmdprio.c  | 236 +++++++++++++++++++++++++++++++++++++++++++++
 engines/cmdprio.h  | 150 +++++-----------------------
 engines/io_uring.c | 100 +++++++------------
 engines/libaio.c   |  72 +++++---------
 fio.1              |   3 +-
 7 files changed, 325 insertions(+), 247 deletions(-)
 create mode 100644 engines/cmdprio.c

-- 
2.33.1

[PATCH v2 1/8] docs: update cmdprio_percentage documentation

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Commit 1437d6357429 ("libaio,io_uring: relax cmdprio_percentage
constraints") relaxed the cmdprio_percentage constraints such that
cmdprio_percentage and prioclass/prio could be used together.

However, it forgot to remove the mention of this constraint from
the docs. Update the docs to reflect the new behavior.

Fixes: 1437d6357429 ("libaio,io_uring: relax cmdprio_percentage constraints")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 HOWTO | 5 ++---
 fio.1 | 3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/HOWTO b/HOWTO
index 297a0485..196bca6c 100644
--- a/HOWTO
+++ b/HOWTO
@@ -2167,9 +2167,8 @@ with the caveat that when used on the command line, they must come after the
 
     Set the percentage of I/O that will be issued with the highest priority.
     Default: 0. A single value applies to reads and writes. Comma-separated
-    values may be specified for reads and writes. This option cannot be used
-    with the :option:`prio` or :option:`prioclass` options. For this option
-    to be effective, NCQ priority must be supported and enabled, and `direct=1'
+    values may be specified for reads and writes. For this option to be
+    effective, NCQ priority must be supported and enabled, and `direct=1'
     option must be used. fio must also be run as the root user.
 
 .. option:: cmdprio_class=int[,int] : [io_uring] [libaio]
diff --git a/fio.1 b/fio.1
index 78988c9e..e3c3feae 100644
--- a/fio.1
+++ b/fio.1
@@ -1965,8 +1965,7 @@ with the caveat that when used on the command line, they must come after the
 .BI (io_uring,libaio)cmdprio_percentage \fR=\fPint[,int]
 Set the percentage of I/O that will be issued with the highest priority.
 Default: 0. A single value applies to reads and writes. Comma-separated
-values may be specified for reads and writes. This option cannot be used
-with the `prio` or `prioclass` options. For this option to be effective,
+values may be specified for reads and writes. For this option to be effective,
 NCQ priority must be supported and enabled, and `direct=1' option must be
 used. fio must also be run as the root user.
 .TP
-- 
2.33.1

[PATCH v2 2/8] cmdprio: move cmdprio function definitions to a new cmdprio.c file

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Move cmdprio function definitions from the cmdprio.h header file to a new
cmdprio.c file, such that we can add new static functions to cmdprio.c.

A follow up patch will add new cmdprio functions which do not need to be
directly accessible by ioengines.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 Makefile          |   6 +++
 engines/cmdprio.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++
 engines/cmdprio.h | 127 ++------------------------------------------
 3 files changed, 141 insertions(+), 122 deletions(-)
 create mode 100644 engines/cmdprio.c

diff --git a/Makefile b/Makefile
index 4ae5a371..e9028dce 100644
--- a/Makefile
+++ b/Makefile
@@ -98,6 +98,7 @@ else ifdef CONFIG_32BIT
 endif
 ifdef CONFIG_LIBAIO
   libaio_SRCS = engines/libaio.c
+  cmdprio_SRCS = engines/cmdprio.c
   libaio_LIBS = -laio
   ENGINES += libaio
 endif
@@ -225,6 +226,7 @@ endif
 ifeq ($(CONFIG_TARGET_OS), Linux)
   SOURCE += diskutil.c fifo.c blktrace.c cgroup.c trim.c engines/sg.c \
 		oslib/linux-dev-lookup.c engines/io_uring.c
+  cmdprio_SRCS = engines/cmdprio.c
 ifdef CONFIG_HAS_BLKZONED
   SOURCE += oslib/linux-blkzoned.c
 endif
@@ -281,6 +283,10 @@ ifneq (,$(findstring CYGWIN,$(CONFIG_TARGET_OS)))
   FIO_CFLAGS += -DPSAPI_VERSION=1 -Ios/windows/posix/include -Wno-format
 endif
 
+ifdef cmdprio_SRCS
+  SOURCE += $(cmdprio_SRCS)
+endif
+
 ifdef CONFIG_DYNAMIC_ENGINES
  DYNAMIC_ENGS := $(ENGINES)
 define engine_template =
diff --git a/engines/cmdprio.c b/engines/cmdprio.c
new file mode 100644
index 00000000..f5b7342f
--- /dev/null
+++ b/engines/cmdprio.c
@@ -0,0 +1,130 @@
+/*
+ * IO priority handling helper functions common to the libaio and io_uring
+ * engines.
+ */
+
+#include "cmdprio.h"
+
+static int fio_cmdprio_bssplit_ddir(struct thread_options *to, void *cb_arg,
+				    enum fio_ddir ddir, char *str, bool data)
+{
+	struct cmdprio *cmdprio = cb_arg;
+	struct split split;
+	unsigned int i;
+
+	if (ddir == DDIR_TRIM)
+		return 0;
+
+	memset(&split, 0, sizeof(split));
+
+	if (split_parse_ddir(to, &split, str, data, BSSPLIT_MAX))
+		return 1;
+	if (!split.nr)
+		return 0;
+
+	cmdprio->bssplit_nr[ddir] = split.nr;
+	cmdprio->bssplit[ddir] = malloc(split.nr * sizeof(struct bssplit));
+	if (!cmdprio->bssplit[ddir])
+		return 1;
+
+	for (i = 0; i < split.nr; i++) {
+		cmdprio->bssplit[ddir][i].bs = split.val1[i];
+		if (split.val2[i] == -1U) {
+			cmdprio->bssplit[ddir][i].perc = 0;
+		} else {
+			if (split.val2[i] > 100)
+				cmdprio->bssplit[ddir][i].perc = 100;
+			else
+				cmdprio->bssplit[ddir][i].perc = split.val2[i];
+		}
+	}
+
+	return 0;
+}
+
+int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
+			      struct cmdprio *cmdprio)
+{
+	char *str, *p;
+	int i, ret = 0;
+
+	p = str = strdup(input);
+
+	strip_blank_front(&str);
+	strip_blank_end(str);
+
+	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
+
+	if (parse_dryrun()) {
+		for (i = 0; i < DDIR_RWDIR_CNT; i++) {
+			free(cmdprio->bssplit[i]);
+			cmdprio->bssplit[i] = NULL;
+			cmdprio->bssplit_nr[i] = 0;
+		}
+	}
+
+	free(p);
+	return ret;
+}
+
+int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
+{
+	enum fio_ddir ddir = io_u->ddir;
+	unsigned int p = cmdprio->percentage[ddir];
+	int i;
+
+	/*
+	 * If cmdprio_percentage option was specified, then use that
+	 * percentage. Otherwise, use cmdprio_bssplit percentages depending
+	 * on the IO size.
+	 */
+	if (p)
+		return p;
+
+	for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
+		if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
+			return cmdprio->bssplit[ddir][i].perc;
+	}
+
+	return 0;
+}
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+		     bool *has_cmdprio)
+{
+	struct thread_options *to = &td->o;
+	bool has_cmdprio_percentage = false;
+	bool has_cmdprio_bssplit = false;
+	int i;
+
+	/*
+	 * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
+	 * is not set, default to RT priority class.
+	 */
+	for (i = 0; i < DDIR_RWDIR_CNT; i++) {
+		if (cmdprio->percentage[i]) {
+			if (!cmdprio->class[i])
+				cmdprio->class[i] = IOPRIO_CLASS_RT;
+			has_cmdprio_percentage = true;
+		}
+		if (cmdprio->bssplit_nr[i]) {
+			if (!cmdprio->class[i])
+				cmdprio->class[i] = IOPRIO_CLASS_RT;
+			has_cmdprio_bssplit = true;
+		}
+	}
+
+	/*
+	 * Check for option conflicts
+	 */
+	if (has_cmdprio_percentage && has_cmdprio_bssplit) {
+		log_err("%s: cmdprio_percentage and cmdprio_bssplit options "
+			"are mutually exclusive\n",
+			to->name);
+		return 1;
+	}
+
+	*has_cmdprio = has_cmdprio_percentage || has_cmdprio_bssplit;
+
+	return 0;
+}
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 0edc4365..33a8f5b9 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -16,129 +16,12 @@ struct cmdprio {
 	struct bssplit *bssplit[DDIR_RWDIR_CNT];
 };
 
-static int fio_cmdprio_bssplit_ddir(struct thread_options *to, void *cb_arg,
-				    enum fio_ddir ddir, char *str, bool data)
-{
-	struct cmdprio *cmdprio = cb_arg;
-	struct split split;
-	unsigned int i;
+int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
+			      struct cmdprio *cmdprio);
 
-	if (ddir == DDIR_TRIM)
-		return 0;
+int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u);
 
-	memset(&split, 0, sizeof(split));
-
-	if (split_parse_ddir(to, &split, str, data, BSSPLIT_MAX))
-		return 1;
-	if (!split.nr)
-		return 0;
-
-	cmdprio->bssplit_nr[ddir] = split.nr;
-	cmdprio->bssplit[ddir] = malloc(split.nr * sizeof(struct bssplit));
-	if (!cmdprio->bssplit[ddir])
-		return 1;
-
-	for (i = 0; i < split.nr; i++) {
-		cmdprio->bssplit[ddir][i].bs = split.val1[i];
-		if (split.val2[i] == -1U) {
-			cmdprio->bssplit[ddir][i].perc = 0;
-		} else {
-			if (split.val2[i] > 100)
-				cmdprio->bssplit[ddir][i].perc = 100;
-			else
-				cmdprio->bssplit[ddir][i].perc = split.val2[i];
-		}
-	}
-
-	return 0;
-}
-
-static int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
-				     struct cmdprio *cmdprio)
-{
-	char *str, *p;
-	int i, ret = 0;
-
-	p = str = strdup(input);
-
-	strip_blank_front(&str);
-	strip_blank_end(str);
-
-	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
-
-	if (parse_dryrun()) {
-		for (i = 0; i < DDIR_RWDIR_CNT; i++) {
-			free(cmdprio->bssplit[i]);
-			cmdprio->bssplit[i] = NULL;
-			cmdprio->bssplit_nr[i] = 0;
-		}
-	}
-
-	free(p);
-	return ret;
-}
-
-static inline int fio_cmdprio_percentage(struct cmdprio *cmdprio,
-					 struct io_u *io_u)
-{
-	enum fio_ddir ddir = io_u->ddir;
-	unsigned int p = cmdprio->percentage[ddir];
-	int i;
-
-	/*
-	 * If cmdprio_percentage option was specified, then use that
-	 * percentage. Otherwise, use cmdprio_bssplit percentages depending
-	 * on the IO size.
-	 */
-	if (p)
-		return p;
-
-	for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
-		if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
-			return cmdprio->bssplit[ddir][i].perc;
-	}
-
-	return 0;
-}
-
-static int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
-			    bool *has_cmdprio)
-{
-	struct thread_options *to = &td->o;
-	bool has_cmdprio_percentage = false;
-	bool has_cmdprio_bssplit = false;
-	int i;
-
-	/*
-	 * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
-	 * is not set, default to RT priority class.
-	 */
-	for (i = 0; i < DDIR_RWDIR_CNT; i++) {
-		if (cmdprio->percentage[i]) {
-			if (!cmdprio->class[i])
-				cmdprio->class[i] = IOPRIO_CLASS_RT;
-			has_cmdprio_percentage = true;
-		}
-		if (cmdprio->bssplit_nr[i]) {
-			if (!cmdprio->class[i])
-				cmdprio->class[i] = IOPRIO_CLASS_RT;
-			has_cmdprio_bssplit = true;
-		}
-	}
-
-	/*
-	 * Check for option conflicts
-	 */
-	if (has_cmdprio_percentage && has_cmdprio_bssplit) {
-		log_err("%s: cmdprio_percentage and cmdprio_bssplit options "
-			"are mutually exclusive\n",
-			to->name);
-		return 1;
-	}
-
-	*has_cmdprio = has_cmdprio_percentage || has_cmdprio_bssplit;
-
-	return 0;
-}
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+		     bool *has_cmdprio);
 
 #endif
-- 
2.33.1

[PATCH v2 3/8] cmdprio: do not allocate memory for unused data direction

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

All cmdprio options only support data directions read and write.
However, each cmdprio option allocates memory for ddir trim as well,
even though nothing is ever written to this memory.

Change this so that we don't allocate memory for something which is
never used.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 engines/cmdprio.c |  4 ++--
 engines/cmdprio.h | 13 ++++++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index f5b7342f..2c764e49 100644
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -56,7 +56,7 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
 
 	if (parse_dryrun()) {
-		for (i = 0; i < DDIR_RWDIR_CNT; i++) {
+		for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
 			free(cmdprio->bssplit[i]);
 			cmdprio->bssplit[i] = NULL;
 			cmdprio->bssplit_nr[i] = 0;
@@ -101,7 +101,7 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
 	 * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
 	 * is not set, default to RT priority class.
 	 */
-	for (i = 0; i < DDIR_RWDIR_CNT; i++) {
+	for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
 		if (cmdprio->percentage[i]) {
 			if (!cmdprio->class[i])
 				cmdprio->class[i] = IOPRIO_CLASS_RT;
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 33a8f5b9..d3265741 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -8,12 +8,15 @@
 
 #include "../fio.h"
 
+/* read and writes only, no trim */
+#define CMDPRIO_RWDIR_CNT 2
+
 struct cmdprio {
-	unsigned int percentage[DDIR_RWDIR_CNT];
-	unsigned int class[DDIR_RWDIR_CNT];
-	unsigned int level[DDIR_RWDIR_CNT];
-	unsigned int bssplit_nr[DDIR_RWDIR_CNT];
-	struct bssplit *bssplit[DDIR_RWDIR_CNT];
+	unsigned int percentage[CMDPRIO_RWDIR_CNT];
+	unsigned int class[CMDPRIO_RWDIR_CNT];
+	unsigned int level[CMDPRIO_RWDIR_CNT];
+	unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
+	struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
 };
 
 int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
-- 
2.33.1

[PATCH v2 4/8] io_uring: set async IO priority to td->ioprio in fio_ioring_prep()

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The default priority (which is either 0 or the value set by "prio" and
"prioclass" options) is now saved in td->ioprio.

The simplest thing is therefore to unconditionally set the async IO
priority to td->ioprio in fio_ioring_prep(), and let fio_ioring_prio_prep()
only handle the case where cmdprio_percentage/cmdprio_bssplit is enabled.

Therefore, fio_ioring_prio_prep() doesn't need to care if prio/prioclass
was enabled or not, we can simply think that fio_ioring_prio_prep()
might "override" the default priority, whatever the default priority may
be.

Doing it this way also has the advantage that the prio_prep() function
in io_uring will now look identical to the prio_prep() function in
libaio.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/io_uring.c | 51 ++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 27a4a678..0e66398a 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -338,6 +338,18 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 			sqe->rw_flags |= RWF_UNCACHED;
 		if (o->nowait)
 			sqe->rw_flags |= RWF_NOWAIT;
+
+		/*
+		 * Since io_uring can have a submission context (sqthread_poll)
+		 * that is different from the process context, we cannot rely on
+		 * the IO priority set by ioprio_set() (option prio/prioclass)
+		 * to be inherited.
+		 * td->ioprio will have the value of the "default prio", so set
+		 * this unconditionally. This value might get overriden by
+		 * fio_ioring_prio_prep() if the option cmdprio_percentage or
+		 * cmdprio_bssplit is used.
+		 */
+		sqe->ioprio = td->ioprio;
 		sqe->off = io_u->offset;
 	} else if (ddir_sync(io_u->ddir)) {
 		sqe->ioprio = 0;
@@ -456,29 +468,25 @@ static void fio_ioring_prio_prep(struct thread_data *td, struct io_u *io_u)
 		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
 
 	if (p && rand_between(&td->prio_state, 0, 99) < p) {
+		io_u->ioprio = cmdprio_value;
 		sqe->ioprio = cmdprio_value;
 		if (!td->ioprio || cmdprio_value < td->ioprio) {
 			/*
 			 * The async IO priority is higher (has a lower value)
-			 * than the priority set by "prio" and "prioclass"
-			 * options.
-			 */
-			io_u->flags |= IO_U_F_HIGH_PRIO;
-		}
-	} else {
-		sqe->ioprio = td->ioprio;
-		if (cmdprio_value && td->ioprio && td->ioprio < cmdprio_value) {
-			/*
-			 * The IO will be executed with the priority set by
-			 * "prio" and "prioclass" options, and this priority
-			 * is higher (has a lower value) than the async IO
-			 * priority.
+			 * than the default priority (which is either 0 or the
+			 * value set by "prio" and "prioclass" options).
 			 */
 			io_u->flags |= IO_U_F_HIGH_PRIO;
 		}
+	} else if (td->ioprio && td->ioprio < cmdprio_value) {
+		/*
+		 * The IO will be executed with the default priority (which is
+		 * either 0 or the value set by "prio" and "prioclass options),
+		 * and this priority is higher (has a lower value) than the
+		 * async IO priority.
+		 */
+		io_u->flags |= IO_U_F_HIGH_PRIO;
 	}
-
-	io_u->ioprio = sqe->ioprio;
 }
 
 static enum fio_q_status fio_ioring_queue(struct thread_data *td,
@@ -820,7 +828,6 @@ static int fio_ioring_init(struct thread_data *td)
 	struct ioring_options *o = td->eo;
 	struct ioring_data *ld;
 	struct cmdprio *cmdprio = &o->cmdprio;
-	bool has_cmdprio = false;
 	int ret;
 
 	/* sqthread submission requires registered files */
@@ -845,22 +852,12 @@ static int fio_ioring_init(struct thread_data *td)
 
 	td->io_ops_data = ld;
 
-	ret = fio_cmdprio_init(td, cmdprio, &has_cmdprio);
+	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
 	if (ret) {
 		td_verror(td, EINVAL, "fio_ioring_init");
 		return 1;
 	}
 
-	/*
-	 * Since io_uring can have a submission context (sqthread_poll) that is
-	 * different from the process context, we cannot rely on the the IO
-	 * priority set by ioprio_set() (option prio/prioclass) to be inherited.
-	 * Therefore, we set the sqe->ioprio field when prio/prioclass is used.
-	 */
-	ld->use_cmdprio = has_cmdprio ||
-		fio_option_is_set(&td->o, ioprio_class) ||
-		fio_option_is_set(&td->o, ioprio);
-
 	return 0;
 }
 
-- 
2.33.1

Re: [PATCH v2 4/8] io_uring: set async IO priority to td->ioprio in fio_ioring_prep()

[Damien Le Moal]

On 2021/11/10 8:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The default priority (which is either 0 or the value set by "prio" and
> "prioclass" options) is now saved in td->ioprio.
> 
> The simplest thing is therefore to unconditionally set the async IO
> priority to td->ioprio in fio_ioring_prep(), and let fio_ioring_prio_prep()
> only handle the case where cmdprio_percentage/cmdprio_bssplit is enabled.
> 
> Therefore, fio_ioring_prio_prep() doesn't need to care if prio/prioclass
> was enabled or not, we can simply think that fio_ioring_prio_prep()
> might "override" the default priority, whatever the default priority may
> be.
> 
> Doing it this way also has the advantage that the prio_prep() function
> in io_uring will now look identical to the prio_prep() function in
> libaio.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/io_uring.c | 51 ++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 27a4a678..0e66398a 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -338,6 +338,18 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
>  			sqe->rw_flags |= RWF_UNCACHED;
>  		if (o->nowait)
>  			sqe->rw_flags |= RWF_NOWAIT;
> +
> +		/*
> +		 * Since io_uring can have a submission context (sqthread_poll)
> +		 * that is different from the process context, we cannot rely on
> +		 * the IO priority set by ioprio_set() (option prio/prioclass)
> +		 * to be inherited.
> +		 * td->ioprio will have the value of the "default prio", so set
> +		 * this unconditionally. This value might get overriden by

Nit: s/overriden/overridden

With this fixed, looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>


> +		 * fio_ioring_prio_prep() if the option cmdprio_percentage or
> +		 * cmdprio_bssplit is used.
> +		 */
> +		sqe->ioprio = td->ioprio;
>  		sqe->off = io_u->offset;
>  	} else if (ddir_sync(io_u->ddir)) {
>  		sqe->ioprio = 0;
> @@ -456,29 +468,25 @@ static void fio_ioring_prio_prep(struct thread_data *td, struct io_u *io_u)
>  		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
>  
>  	if (p && rand_between(&td->prio_state, 0, 99) < p) {
> +		io_u->ioprio = cmdprio_value;
>  		sqe->ioprio = cmdprio_value;
>  		if (!td->ioprio || cmdprio_value < td->ioprio) {
>  			/*
>  			 * The async IO priority is higher (has a lower value)
> -			 * than the priority set by "prio" and "prioclass"
> -			 * options.
> -			 */
> -			io_u->flags |= IO_U_F_HIGH_PRIO;
> -		}
> -	} else {
> -		sqe->ioprio = td->ioprio;
> -		if (cmdprio_value && td->ioprio && td->ioprio < cmdprio_value) {
> -			/*
> -			 * The IO will be executed with the priority set by
> -			 * "prio" and "prioclass" options, and this priority
> -			 * is higher (has a lower value) than the async IO
> -			 * priority.
> +			 * than the default priority (which is either 0 or the
> +			 * value set by "prio" and "prioclass" options).
>  			 */
>  			io_u->flags |= IO_U_F_HIGH_PRIO;
>  		}
> +	} else if (td->ioprio && td->ioprio < cmdprio_value) {
> +		/*
> +		 * The IO will be executed with the default priority (which is
> +		 * either 0 or the value set by "prio" and "prioclass options),
> +		 * and this priority is higher (has a lower value) than the
> +		 * async IO priority.
> +		 */
> +		io_u->flags |= IO_U_F_HIGH_PRIO;
>  	}
> -
> -	io_u->ioprio = sqe->ioprio;
>  }
>  
>  static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> @@ -820,7 +828,6 @@ static int fio_ioring_init(struct thread_data *td)
>  	struct ioring_options *o = td->eo;
>  	struct ioring_data *ld;
>  	struct cmdprio *cmdprio = &o->cmdprio;
> -	bool has_cmdprio = false;
>  	int ret;
>  
>  	/* sqthread submission requires registered files */
> @@ -845,22 +852,12 @@ static int fio_ioring_init(struct thread_data *td)
>  
>  	td->io_ops_data = ld;
>  
> -	ret = fio_cmdprio_init(td, cmdprio, &has_cmdprio);
> +	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
>  	if (ret) {
>  		td_verror(td, EINVAL, "fio_ioring_init");
>  		return 1;
>  	}
>  
> -	/*
> -	 * Since io_uring can have a submission context (sqthread_poll) that is
> -	 * different from the process context, we cannot rely on the the IO
> -	 * priority set by ioprio_set() (option prio/prioclass) to be inherited.
> -	 * Therefore, we set the sqe->ioprio field when prio/prioclass is used.
> -	 */
> -	ld->use_cmdprio = has_cmdprio ||
> -		fio_option_is_set(&td->o, ioprio_class) ||
> -		fio_option_is_set(&td->o, ioprio);
> -
>  	return 0;
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research

[PATCH v2 5/8] libaio,io_uring: rename prio_prep() to include cmdprio in the name

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The default priority (which is either 0 or the value set by "prio" and
"prioclass" options, will now be used regardless if prio_prep() is
called or not. This is true for both libaio and io_uring.

The way to think about it is that prio_prep() is only called if
cmdprio_percentage/cmdprio_bssplit is used.

prio_prep() might then override the default priority, if the random
value happens to say that this I/O should use the cmdprio_value,
rather than the default priority.

Rename the prio_prep() functions to highlight that these functions
are now only called if cmdprio is used. (If only option
"prio"/"prioclass" is used, that is handled elsewhere.)

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 engines/io_uring.c | 7 ++++---
 engines/libaio.c   | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 0e66398a..6f90e0a7 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -346,7 +346,7 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 		 * to be inherited.
 		 * td->ioprio will have the value of the "default prio", so set
 		 * this unconditionally. This value might get overriden by
-		 * fio_ioring_prio_prep() if the option cmdprio_percentage or
+		 * fio_ioring_cmdprio_prep() if the option cmdprio_percentage or
 		 * cmdprio_bssplit is used.
 		 */
 		sqe->ioprio = td->ioprio;
@@ -456,7 +456,7 @@ static int fio_ioring_getevents(struct thread_data *td, unsigned int min,
 	return r < 0 ? r : events;
 }
 
-static void fio_ioring_prio_prep(struct thread_data *td, struct io_u *io_u)
+static void fio_ioring_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
 {
 	struct ioring_options *o = td->eo;
 	struct ioring_data *ld = td->io_ops_data;
@@ -517,7 +517,8 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 		return FIO_Q_BUSY;
 
 	if (ld->use_cmdprio)
-		fio_ioring_prio_prep(td, io_u);
+		fio_ioring_cmdprio_prep(td, io_u);
+
 	ring->array[tail & ld->sq_ring_mask] = io_u->index;
 	atomic_store_release(ring->tail, next_tail);
 
diff --git a/engines/libaio.c b/engines/libaio.c
index dd655355..9c14dd88 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -205,7 +205,7 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
 	return 0;
 }
 
-static void fio_libaio_prio_prep(struct thread_data *td, struct io_u *io_u)
+static void fio_libaio_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
 {
 	struct libaio_options *o = td->eo;
 	struct cmdprio *cmdprio = &o->cmdprio;
@@ -369,7 +369,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
 	}
 
 	if (ld->use_cmdprio)
-		fio_libaio_prio_prep(td, io_u);
+		fio_libaio_cmdprio_prep(td, io_u);
 
 	ld->iocbs[ld->head] = &io_u->iocb;
 	ld->io_us[ld->head] = io_u;
-- 
2.33.1

[PATCH v2 7/8] cmdprio: add mode to make the logic easier to reason about

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Add a new field "mode", in order to know if we are determining IO
priorities according to cmdprio_percentage or to cmdprio_bssplit.

This makes the logic easier to reason about, and allows us to
remove the "use_cmdprio" variable from the ioengines themselves.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/cmdprio.c  | 45 +++++++++++++++++++++++++++++----------------
 engines/cmdprio.h  | 10 ++++++++--
 engines/io_uring.c |  7 +++----
 engines/libaio.c   |  7 +++----
 4 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index 9cad76b5..174bed7c 100644
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -70,20 +70,19 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
 {
 	enum fio_ddir ddir = io_u->ddir;
-	unsigned int p = cmdprio->percentage[ddir];
 	int i;
 
-	/*
-	 * If cmdprio_percentage option was specified, then use that
-	 * percentage. Otherwise, use cmdprio_bssplit percentages depending
-	 * on the IO size.
-	 */
-	if (p)
-		return p;
-
-	for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
-		if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
-			return cmdprio->bssplit[ddir][i].perc;
+	switch (cmdprio->mode) {
+	case CMDPRIO_MODE_PERC:
+		return cmdprio->percentage[ddir];
+	case CMDPRIO_MODE_BSSPLIT:
+		for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
+			if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
+				return cmdprio->bssplit[ddir][i].perc;
+		}
+		break;
+	default:
+		assert(0);
 	}
 
 	return 0;
@@ -99,10 +98,20 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 			    struct io_u *io_u)
 {
 	enum fio_ddir ddir = io_u->ddir;
-	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
+	unsigned int p;
 	unsigned int cmdprio_value =
 		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
 
+	if (cmdprio->mode == CMDPRIO_MODE_NONE) {
+		/*
+		 * An I/O engine should never call this function if cmdprio
+		 * is not is use.
+		 */
+		assert(0);
+		return false;
+	}
+
+	p = fio_cmdprio_percentage(cmdprio, io_u);
 	if (p && rand_between(&td->prio_state, 0, 99) < p) {
 		io_u->ioprio = cmdprio_value;
 		if (!td->ioprio || cmdprio_value < td->ioprio) {
@@ -128,8 +137,7 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 	return false;
 }
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
-		     bool *has_cmdprio)
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
 {
 	struct thread_options *to = &td->o;
 	bool has_cmdprio_percentage = false;
@@ -163,7 +171,12 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
 		return 1;
 	}
 
-	*has_cmdprio = has_cmdprio_percentage || has_cmdprio_bssplit;
+	if (has_cmdprio_bssplit)
+		cmdprio->mode = CMDPRIO_MODE_BSSPLIT;
+	else if (has_cmdprio_percentage)
+		cmdprio->mode = CMDPRIO_MODE_PERC;
+	else
+		cmdprio->mode = CMDPRIO_MODE_NONE;
 
 	return 0;
 }
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 732b087e..7e4fcf6c 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -11,12 +11,19 @@
 /* read and writes only, no trim */
 #define CMDPRIO_RWDIR_CNT 2
 
+enum {
+	CMDPRIO_MODE_NONE,
+	CMDPRIO_MODE_PERC,
+	CMDPRIO_MODE_BSSPLIT,
+};
+
 struct cmdprio {
 	unsigned int percentage[CMDPRIO_RWDIR_CNT];
 	unsigned int class[CMDPRIO_RWDIR_CNT];
 	unsigned int level[CMDPRIO_RWDIR_CNT];
 	unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
 	struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
+	unsigned int mode;
 };
 
 int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
@@ -25,7 +32,6 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 			    struct io_u *io_u);
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
-		     bool *has_cmdprio);
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
 
 #endif
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 230f08a5..979d07d4 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -68,8 +68,6 @@ struct ioring_data {
 	int prepped;
 
 	struct ioring_mmap mmap[3];
-
-	bool use_cmdprio;
 };
 
 struct ioring_options {
@@ -472,6 +470,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 {
 	struct ioring_data *ld = td->io_ops_data;
 	struct io_sq_ring *ring = &ld->sq_ring;
+	struct ioring_options *o = td->eo;
 	unsigned tail, next_tail;
 
 	fio_ro_check(td, io_u);
@@ -494,7 +493,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 	if (next_tail == atomic_load_acquire(ring->head))
 		return FIO_Q_BUSY;
 
-	if (ld->use_cmdprio)
+	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
 		fio_ioring_cmdprio_prep(td, io_u);
 
 	ring->array[tail & ld->sq_ring_mask] = io_u->index;
@@ -831,7 +830,7 @@ static int fio_ioring_init(struct thread_data *td)
 
 	td->io_ops_data = ld;
 
-	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
+	ret = fio_cmdprio_init(td, cmdprio);
 	if (ret) {
 		td_verror(td, EINVAL, "fio_ioring_init");
 		return 1;
diff --git a/engines/libaio.c b/engines/libaio.c
index dc6ad08e..53fe7428 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -51,8 +51,6 @@ struct libaio_data {
 	unsigned int queued;
 	unsigned int head;
 	unsigned int tail;
-
-	bool use_cmdprio;
 };
 
 struct libaio_options {
@@ -320,6 +318,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
 					  struct io_u *io_u)
 {
 	struct libaio_data *ld = td->io_ops_data;
+	struct libaio_options *o = td->eo;
 
 	fio_ro_check(td, io_u);
 
@@ -350,7 +349,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
 		return FIO_Q_COMPLETED;
 	}
 
-	if (ld->use_cmdprio)
+	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
 		fio_libaio_cmdprio_prep(td, io_u);
 
 	ld->iocbs[ld->head] = &io_u->iocb;
@@ -507,7 +506,7 @@ static int fio_libaio_init(struct thread_data *td)
 
 	td->io_ops_data = ld;
 
-	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
+	ret = fio_cmdprio_init(td, cmdprio);
 	if (ret) {
 		td_verror(td, EINVAL, "fio_libaio_init");
 		return 1;
-- 
2.33.1

Re: [PATCH v2 7/8] cmdprio: add mode to make the logic easier to reason about

[Damien Le Moal]

On 2021/11/10 8:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Add a new field "mode", in order to know if we are determining IO
> priorities according to cmdprio_percentage or to cmdprio_bssplit.
> 
> This makes the logic easier to reason about, and allows us to
> remove the "use_cmdprio" variable from the ioengines themselves.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/cmdprio.c  | 45 +++++++++++++++++++++++++++++----------------
>  engines/cmdprio.h  | 10 ++++++++--
>  engines/io_uring.c |  7 +++----
>  engines/libaio.c   |  7 +++----
>  4 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 9cad76b5..174bed7c 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -70,20 +70,19 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
>  {
>  	enum fio_ddir ddir = io_u->ddir;
> -	unsigned int p = cmdprio->percentage[ddir];
>  	int i;
>  
> -	/*
> -	 * If cmdprio_percentage option was specified, then use that
> -	 * percentage. Otherwise, use cmdprio_bssplit percentages depending
> -	 * on the IO size.
> -	 */
> -	if (p)
> -		return p;
> -
> -	for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
> -		if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> -			return cmdprio->bssplit[ddir][i].perc;
> +	switch (cmdprio->mode) {
> +	case CMDPRIO_MODE_PERC:
> +		return cmdprio->percentage[ddir];
> +	case CMDPRIO_MODE_BSSPLIT:
> +		for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
> +			if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> +				return cmdprio->bssplit[ddir][i].perc;
> +		}
> +		break;
> +	default:
> +		assert(0);
>  	}
>  
>  	return 0;
> @@ -99,10 +98,20 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  			    struct io_u *io_u)
>  {
>  	enum fio_ddir ddir = io_u->ddir;
> -	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> +	unsigned int p;
>  	unsigned int cmdprio_value =
>  		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
>  
> +	if (cmdprio->mode == CMDPRIO_MODE_NONE) {
> +		/*
> +		 * An I/O engine should never call this function if cmdprio
> +		 * is not is use.
> +		 */
> +		assert(0);
> +		return false;
> +	}

This hunk is not useful as the call to fio_cmdprio_percentage() will cover this
buggy case. So let's remove it. No need to clutter the code with it. You may
want to move the above comment under the if to the default case in
fio_cmdprio_percentage() switch.

> +
> +	p = fio_cmdprio_percentage(cmdprio, io_u);
>  	if (p && rand_between(&td->prio_state, 0, 99) < p) {
>  		io_u->ioprio = cmdprio_value;
>  		if (!td->ioprio || cmdprio_value < td->ioprio) {
> @@ -128,8 +137,7 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  	return false;
>  }
>  
> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> -		     bool *has_cmdprio)
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
>  {
>  	struct thread_options *to = &td->o;
>  	bool has_cmdprio_percentage = false;
> @@ -163,7 +171,12 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
>  		return 1;
>  	}
>  
> -	*has_cmdprio = has_cmdprio_percentage || has_cmdprio_bssplit;
> +	if (has_cmdprio_bssplit)
> +		cmdprio->mode = CMDPRIO_MODE_BSSPLIT;
> +	else if (has_cmdprio_percentage)
> +		cmdprio->mode = CMDPRIO_MODE_PERC;
> +	else
> +		cmdprio->mode = CMDPRIO_MODE_NONE;
>  
>  	return 0;
>  }
> diff --git a/engines/cmdprio.h b/engines/cmdprio.h
> index 732b087e..7e4fcf6c 100644
> --- a/engines/cmdprio.h
> +++ b/engines/cmdprio.h
> @@ -11,12 +11,19 @@
>  /* read and writes only, no trim */
>  #define CMDPRIO_RWDIR_CNT 2
>  
> +enum {
> +	CMDPRIO_MODE_NONE,
> +	CMDPRIO_MODE_PERC,
> +	CMDPRIO_MODE_BSSPLIT,
> +};
> +
>  struct cmdprio {
>  	unsigned int percentage[CMDPRIO_RWDIR_CNT];
>  	unsigned int class[CMDPRIO_RWDIR_CNT];
>  	unsigned int level[CMDPRIO_RWDIR_CNT];
>  	unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
>  	struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
> +	unsigned int mode;
>  };
>  
>  int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> @@ -25,7 +32,6 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  			    struct io_u *io_u);
>  
> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> -		     bool *has_cmdprio);
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
>  
>  #endif
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 230f08a5..979d07d4 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -68,8 +68,6 @@ struct ioring_data {
>  	int prepped;
>  
>  	struct ioring_mmap mmap[3];
> -
> -	bool use_cmdprio;
>  };
>  
>  struct ioring_options {
> @@ -472,6 +470,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
>  {
>  	struct ioring_data *ld = td->io_ops_data;
>  	struct io_sq_ring *ring = &ld->sq_ring;
> +	struct ioring_options *o = td->eo;
>  	unsigned tail, next_tail;
>  
>  	fio_ro_check(td, io_u);
> @@ -494,7 +493,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
>  	if (next_tail == atomic_load_acquire(ring->head))
>  		return FIO_Q_BUSY;
>  
> -	if (ld->use_cmdprio)
> +	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
>  		fio_ioring_cmdprio_prep(td, io_u);
>  
>  	ring->array[tail & ld->sq_ring_mask] = io_u->index;
> @@ -831,7 +830,7 @@ static int fio_ioring_init(struct thread_data *td)
>  
>  	td->io_ops_data = ld;
>  
> -	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
> +	ret = fio_cmdprio_init(td, cmdprio);
>  	if (ret) {
>  		td_verror(td, EINVAL, "fio_ioring_init");
>  		return 1;
> diff --git a/engines/libaio.c b/engines/libaio.c
> index dc6ad08e..53fe7428 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -51,8 +51,6 @@ struct libaio_data {
>  	unsigned int queued;
>  	unsigned int head;
>  	unsigned int tail;
> -
> -	bool use_cmdprio;
>  };
>  
>  struct libaio_options {
> @@ -320,6 +318,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
>  					  struct io_u *io_u)
>  {
>  	struct libaio_data *ld = td->io_ops_data;
> +	struct libaio_options *o = td->eo;
>  
>  	fio_ro_check(td, io_u);
>  
> @@ -350,7 +349,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
>  		return FIO_Q_COMPLETED;
>  	}
>  
> -	if (ld->use_cmdprio)
> +	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
>  		fio_libaio_cmdprio_prep(td, io_u);
>  
>  	ld->iocbs[ld->head] = &io_u->iocb;
> @@ -507,7 +506,7 @@ static int fio_libaio_init(struct thread_data *td)
>  
>  	td->io_ops_data = ld;
>  
> -	ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
> +	ret = fio_cmdprio_init(td, cmdprio);
>  	if (ret) {
>  		td_verror(td, EINVAL, "fio_libaio_init");
>  		return 1;
> 

With the above fix, this is a nice cleanup.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

[PATCH v2 6/8] libaio,io_uring: move common cmdprio_prep() code to cmdprio

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Move common cmdprio_prep() code to cmdprio.c to avoid code duplication.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/cmdprio.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
 engines/cmdprio.h  |  3 ++-
 engines/io_uring.c | 34 ++++++----------------------------
 engines/libaio.c   | 28 +++++-----------------------
 4 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index 2c764e49..9cad76b5 100644
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -67,7 +67,7 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 	return ret;
 }
 
-int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
+static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
 {
 	enum fio_ddir ddir = io_u->ddir;
 	unsigned int p = cmdprio->percentage[ddir];
@@ -89,6 +89,45 @@ int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
 	return 0;
 }
 
+/**
+ * fio_cmdprio_set_ioprio - Generates a random value. If the random value is
+ * within the user specified percentage of I/Os that should use a cmdprio
+ * priority value (rather than the default priority), then this function
+ * updates the io_u with a cmdprio priority value.
+ */
+bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
+			    struct io_u *io_u)
+{
+	enum fio_ddir ddir = io_u->ddir;
+	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
+	unsigned int cmdprio_value =
+		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
+
+	if (p && rand_between(&td->prio_state, 0, 99) < p) {
+		io_u->ioprio = cmdprio_value;
+		if (!td->ioprio || cmdprio_value < td->ioprio) {
+			/*
+			 * The async IO priority is higher (has a lower value)
+			 * than the default priority (which is either 0 or the
+			 * value set by "prio" and "prioclass" options).
+			 */
+			io_u->flags |= IO_U_F_HIGH_PRIO;
+		}
+		return true;
+	}
+	if (td->ioprio && td->ioprio < cmdprio_value) {
+		/*
+		 * The IO will be executed with the default priority (which is
+		 * either 0 or the value set by "prio" and "prioclass options),
+		 * and this priority is higher (has a lower value) than the
+		 * async IO priority.
+		 */
+		io_u->flags |= IO_U_F_HIGH_PRIO;
+	}
+
+	return false;
+}
+
 int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
 		     bool *has_cmdprio)
 {
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index d3265741..732b087e 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -22,7 +22,8 @@ struct cmdprio {
 int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 			      struct cmdprio *cmdprio);
 
-int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u);
+bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
+			    struct io_u *io_u);
 
 int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
 		     bool *has_cmdprio);
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 6f90e0a7..230f08a5 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -456,37 +456,15 @@ static int fio_ioring_getevents(struct thread_data *td, unsigned int min,
 	return r < 0 ? r : events;
 }
 
-static void fio_ioring_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
+static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
+					   struct io_u *io_u)
 {
-	struct ioring_options *o = td->eo;
 	struct ioring_data *ld = td->io_ops_data;
-	struct io_uring_sqe *sqe = &ld->sqes[io_u->index];
+	struct ioring_options *o = td->eo;
 	struct cmdprio *cmdprio = &o->cmdprio;
-	enum fio_ddir ddir = io_u->ddir;
-	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
-	unsigned int cmdprio_value =
-		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
-
-	if (p && rand_between(&td->prio_state, 0, 99) < p) {
-		io_u->ioprio = cmdprio_value;
-		sqe->ioprio = cmdprio_value;
-		if (!td->ioprio || cmdprio_value < td->ioprio) {
-			/*
-			 * The async IO priority is higher (has a lower value)
-			 * than the default priority (which is either 0 or the
-			 * value set by "prio" and "prioclass" options).
-			 */
-			io_u->flags |= IO_U_F_HIGH_PRIO;
-		}
-	} else if (td->ioprio && td->ioprio < cmdprio_value) {
-		/*
-		 * The IO will be executed with the default priority (which is
-		 * either 0 or the value set by "prio" and "prioclass options),
-		 * and this priority is higher (has a lower value) than the
-		 * async IO priority.
-		 */
-		io_u->flags |= IO_U_F_HIGH_PRIO;
-	}
+
+	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
+		ld->sqes[io_u->index].ioprio = io_u->ioprio;
 }
 
 static enum fio_q_status fio_ioring_queue(struct thread_data *td,
diff --git a/engines/libaio.c b/engines/libaio.c
index 9c14dd88..dc6ad08e 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -205,33 +205,15 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
 	return 0;
 }
 
-static void fio_libaio_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
+static inline void fio_libaio_cmdprio_prep(struct thread_data *td,
+					   struct io_u *io_u)
 {
 	struct libaio_options *o = td->eo;
 	struct cmdprio *cmdprio = &o->cmdprio;
-	enum fio_ddir ddir = io_u->ddir;
-	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
-	unsigned int cmdprio_value =
-		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
-
-	if (p && rand_between(&td->prio_state, 0, 99) < p) {
-		io_u->ioprio = cmdprio_value;
-		io_u->iocb.aio_reqprio = cmdprio_value;
+
+	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
+		io_u->iocb.aio_reqprio = io_u->ioprio;
 		io_u->iocb.u.c.flags |= IOCB_FLAG_IOPRIO;
-		if (!td->ioprio || cmdprio_value < td->ioprio) {
-			/*
-			 * The async IO priority is higher (has a lower value)
-			 * than the default context priority.
-			 */
-			io_u->flags |= IO_U_F_HIGH_PRIO;
-		}
-	} else if (td->ioprio && td->ioprio < cmdprio_value) {
-		/*
-		 * The IO will be executed with the default context priority,
-		 * and this priority is higher (has a lower value) than the
-		 * async IO priority.
-		 */
-		io_u->flags |= IO_U_F_HIGH_PRIO;
 	}
 }
 
-- 
2.33.1

Re: [PATCH v2 6/8] libaio,io_uring: move common cmdprio_prep() code to cmdprio

[Damien Le Moal]

On 2021/11/10 8:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Move common cmdprio_prep() code to cmdprio.c to avoid code duplication.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/cmdprio.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  engines/cmdprio.h  |  3 ++-
>  engines/io_uring.c | 34 ++++++----------------------------
>  engines/libaio.c   | 28 +++++-----------------------
>  4 files changed, 53 insertions(+), 53 deletions(-)
> 
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 2c764e49..9cad76b5 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -67,7 +67,7 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  	return ret;
>  }
>  
> -int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
> +static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
>  {
>  	enum fio_ddir ddir = io_u->ddir;
>  	unsigned int p = cmdprio->percentage[ddir];
> @@ -89,6 +89,45 @@ int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
>  	return 0;
>  }
>  
> +/**
> + * fio_cmdprio_set_ioprio - Generates a random value. If the random value is
> + * within the user specified percentage of I/Os that should use a cmdprio
> + * priority value (rather than the default priority), then this function
> + * updates the io_u with a cmdprio priority value.
> + */

This comment is not very precise in its description. What about this:

/**
 * fio_cmdprio_set_ioprio - Set an io_u ioprio according to cmdprio options
 *
 * Generates a random percentage value to determine if an io_u ioprio needs
 * to be set. If the random percentage value is within the user specified
 * percentage of I/Os that should use a cmdprio priority value (rather than
 * the default priority), then this function updates the io_u with an ioprio
 * value as defined by the cmdprio/cdmprio_class or cmdprio_bssplit options.
 *
 * Return true if the io_u ioprio was changed and false otherwise.
 */

> +bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
> +			    struct io_u *io_u)
> +{
> +	enum fio_ddir ddir = io_u->ddir;
> +	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> +	unsigned int cmdprio_value =
> +		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> +
> +	if (p && rand_between(&td->prio_state, 0, 99) < p) {
> +		io_u->ioprio = cmdprio_value;
> +		if (!td->ioprio || cmdprio_value < td->ioprio) {
> +			/*
> +			 * The async IO priority is higher (has a lower value)
> +			 * than the default priority (which is either 0 or the
> +			 * value set by "prio" and "prioclass" options).
> +			 */
> +			io_u->flags |= IO_U_F_HIGH_PRIO;
> +		}
> +		return true;
> +	}

Nit: a blank line here would be nice, for readability.

> +	if (td->ioprio && td->ioprio < cmdprio_value) {
> +		/*
> +		 * The IO will be executed with the default priority (which is
> +		 * either 0 or the value set by "prio" and "prioclass options),
> +		 * and this priority is higher (has a lower value) than the
> +		 * async IO priority.
> +		 */
> +		io_u->flags |= IO_U_F_HIGH_PRIO;
> +	}
> +
> +	return false;
> +}
> +
>  int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
>  		     bool *has_cmdprio)
>  {
> diff --git a/engines/cmdprio.h b/engines/cmdprio.h
> index d3265741..732b087e 100644
> --- a/engines/cmdprio.h
> +++ b/engines/cmdprio.h
> @@ -22,7 +22,8 @@ struct cmdprio {
>  int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  			      struct cmdprio *cmdprio);
>  
> -int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u);
> +bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
> +			    struct io_u *io_u);
>  
>  int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
>  		     bool *has_cmdprio);
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 6f90e0a7..230f08a5 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -456,37 +456,15 @@ static int fio_ioring_getevents(struct thread_data *td, unsigned int min,
>  	return r < 0 ? r : events;
>  }
>  
> -static void fio_ioring_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> +static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
> +					   struct io_u *io_u)
>  {
> -	struct ioring_options *o = td->eo;
>  	struct ioring_data *ld = td->io_ops_data;
> -	struct io_uring_sqe *sqe = &ld->sqes[io_u->index];
> +	struct ioring_options *o = td->eo;
>  	struct cmdprio *cmdprio = &o->cmdprio;
> -	enum fio_ddir ddir = io_u->ddir;
> -	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> -	unsigned int cmdprio_value =
> -		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> -
> -	if (p && rand_between(&td->prio_state, 0, 99) < p) {
> -		io_u->ioprio = cmdprio_value;
> -		sqe->ioprio = cmdprio_value;
> -		if (!td->ioprio || cmdprio_value < td->ioprio) {
> -			/*
> -			 * The async IO priority is higher (has a lower value)
> -			 * than the default priority (which is either 0 or the
> -			 * value set by "prio" and "prioclass" options).
> -			 */
> -			io_u->flags |= IO_U_F_HIGH_PRIO;
> -		}
> -	} else if (td->ioprio && td->ioprio < cmdprio_value) {
> -		/*
> -		 * The IO will be executed with the default priority (which is
> -		 * either 0 or the value set by "prio" and "prioclass options),
> -		 * and this priority is higher (has a lower value) than the
> -		 * async IO priority.
> -		 */
> -		io_u->flags |= IO_U_F_HIGH_PRIO;
> -	}
> +
> +	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
> +		ld->sqes[io_u->index].ioprio = io_u->ioprio;
>  }
>  
>  static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> diff --git a/engines/libaio.c b/engines/libaio.c
> index 9c14dd88..dc6ad08e 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -205,33 +205,15 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
>  	return 0;
>  }
>  
> -static void fio_libaio_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> +static inline void fio_libaio_cmdprio_prep(struct thread_data *td,
> +					   struct io_u *io_u)
>  {
>  	struct libaio_options *o = td->eo;
>  	struct cmdprio *cmdprio = &o->cmdprio;
> -	enum fio_ddir ddir = io_u->ddir;
> -	unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> -	unsigned int cmdprio_value =
> -		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> -
> -	if (p && rand_between(&td->prio_state, 0, 99) < p) {
> -		io_u->ioprio = cmdprio_value;
> -		io_u->iocb.aio_reqprio = cmdprio_value;
> +
> +	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
> +		io_u->iocb.aio_reqprio = io_u->ioprio;
>  		io_u->iocb.u.c.flags |= IOCB_FLAG_IOPRIO;
> -		if (!td->ioprio || cmdprio_value < td->ioprio) {
> -			/*
> -			 * The async IO priority is higher (has a lower value)
> -			 * than the default context priority.
> -			 */
> -			io_u->flags |= IO_U_F_HIGH_PRIO;
> -		}
> -	} else if (td->ioprio && td->ioprio < cmdprio_value) {
> -		/*
> -		 * The IO will be executed with the default context priority,
> -		 * and this priority is higher (has a lower value) than the
> -		 * async IO priority.
> -		 */
> -		io_u->flags |= IO_U_F_HIGH_PRIO;
>  	}
>  }
>  
> 

With the above comment fix, looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>


-- 
Damien Le Moal
Western Digital Research

[PATCH v2 8/8] libaio,io_uring: make it possible to cleanup cmdprio malloced data

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The way that fio currently handles engine options:
options_free() will only every call free() on an option that has type
FIO_OPT_STR_STORE. This means that any option that has a pointer in
either td->o or td->eo, which is not of type FIO_OPT_STR_STORE will
leak memory. This is true even for numjobs == 1.

When running with numjobs > 1, fio_options_mem_dupe() will memcpy
td->eo into the new td. Since off1 of the pointers in the first td
has already been set, the pointers in the new td will point to the
same data. (Regardless, options_free() will never try to free the
memory, for neither td.) Neither can we manually free the memory in
cleanup(), since the other td will still point to the same memory,
so this would lead to a double free.

These memory leaks are reported by e.g. valgrind.

The most obvious way to solve this is to put dynamically allocated
memory in {ioring,libaio}_data instead of {ioring,libaio}_options.

This solves the problem since {ioring,libaio}_data is dynamically
allocated by each td during the ioengine init callback, and is freed
when the ioengine cleanup callback for that td is called.

The downside of this is that the parsing has to be done in
fio_cmdprio_init() instead of in the option .cb callback, since the
.cb callback is called before {ioring,libaio}_data is available.

This patch keeps the static cmdprio options in
{ioring,libaio}_options, but moves the dynamically allocated memory
needed by cmdprio to {ioring,libaio}_data.

No cmdprio related memory leaks are reported after this patch.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/cmdprio.c  | 106 ++++++++++++++++++++++++++++++++++-----------
 engines/cmdprio.h  |  15 ++++---
 engines/io_uring.c |  41 +++++++-----------
 engines/libaio.c   |  43 ++++++++----------
 4 files changed, 124 insertions(+), 81 deletions(-)

diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index 174bed7c..7ff39d93 100644
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -46,22 +46,15 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 			      struct cmdprio *cmdprio)
 {
 	char *str, *p;
-	int i, ret = 0;
+	int ret = 0;
 
 	p = str = strdup(input);
 
 	strip_blank_front(&str);
 	strip_blank_end(str);
 
-	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
-
-	if (parse_dryrun()) {
-		for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
-			free(cmdprio->bssplit[i]);
-			cmdprio->bssplit[i] = NULL;
-			cmdprio->bssplit_nr[i] = 0;
-		}
-	}
+	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio,
+			      false);
 
 	free(p);
 	return ret;
@@ -70,11 +63,12 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
 {
 	enum fio_ddir ddir = io_u->ddir;
+	struct cmdprio_options *options = cmdprio->options;
 	int i;
 
 	switch (cmdprio->mode) {
 	case CMDPRIO_MODE_PERC:
-		return cmdprio->percentage[ddir];
+		return options->percentage[ddir];
 	case CMDPRIO_MODE_BSSPLIT:
 		for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
 			if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
@@ -98,9 +92,10 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 			    struct io_u *io_u)
 {
 	enum fio_ddir ddir = io_u->ddir;
+	struct cmdprio_options *options = cmdprio->options;
 	unsigned int p;
 	unsigned int cmdprio_value =
-		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
+		ioprio_value(options->class[ddir], options->level[ddir]);
 
 	if (cmdprio->mode == CMDPRIO_MODE_NONE) {
 		/*
@@ -137,30 +132,85 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 	return false;
 }
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
+static int fio_cmdprio_parse_and_gen_bssplit(struct thread_data *td,
+					     struct cmdprio *cmdprio)
 {
-	struct thread_options *to = &td->o;
-	bool has_cmdprio_percentage = false;
-	bool has_cmdprio_bssplit = false;
+	struct cmdprio_options *options = cmdprio->options;
+	int ret;
+
+	ret = fio_cmdprio_bssplit_parse(td, options->bssplit_str, cmdprio);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	fio_cmdprio_cleanup(cmdprio);
+
+	return ret;
+}
+
+static int fio_cmdprio_gen_data_structs(struct thread_data *td,
+					struct cmdprio *cmdprio)
+{
+	struct cmdprio_options *options = cmdprio->options;
+	int ret = 1;
 	int i;
 
+	if (cmdprio->mode == CMDPRIO_MODE_BSSPLIT)
+		ret = fio_cmdprio_parse_and_gen_bssplit(td, cmdprio);
+	else if (cmdprio->mode == CMDPRIO_MODE_PERC)
+		ret = 0;
+
 	/*
 	 * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
 	 * is not set, default to RT priority class.
 	 */
 	for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
-		if (cmdprio->percentage[i]) {
-			if (!cmdprio->class[i])
-				cmdprio->class[i] = IOPRIO_CLASS_RT;
-			has_cmdprio_percentage = true;
-		}
-		if (cmdprio->bssplit_nr[i]) {
-			if (!cmdprio->class[i])
-				cmdprio->class[i] = IOPRIO_CLASS_RT;
-			has_cmdprio_bssplit = true;
+		if (options->percentage[i] || cmdprio->bssplit_nr[i]) {
+			if (!options->class[i])
+				options->class[i] = IOPRIO_CLASS_RT;
 		}
 	}
 
+	return ret;
+}
+
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio)
+{
+	int ddir;
+
+	for (ddir = 0; ddir < CMDPRIO_RWDIR_CNT; ddir++) {
+		free(cmdprio->bssplit[ddir]);
+		cmdprio->bssplit[ddir] = NULL;
+		cmdprio->bssplit_nr[ddir] = 0;
+	}
+
+	/*
+	 * options points to a cmdprio_options struct that is part of td->eo.
+	 * td->eo itself will be freed by free_ioengine().
+	 */
+	cmdprio->options = NULL;
+}
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+		     struct cmdprio_options *options)
+{
+	struct thread_options *to = &td->o;
+	bool has_cmdprio_percentage = false;
+	bool has_cmdprio_bssplit = false;
+	int i;
+
+	cmdprio->options = options;
+
+	if (options->bssplit_str && strlen(options->bssplit_str))
+		has_cmdprio_bssplit = true;
+
+	for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
+		if (options->percentage[i])
+			has_cmdprio_percentage = true;
+	}
+
 	/*
 	 * Check for option conflicts
 	 */
@@ -178,5 +228,9 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
 	else
 		cmdprio->mode = CMDPRIO_MODE_NONE;
 
-	return 0;
+	/* Nothing left to do if cmdprio is not used */
+	if (cmdprio->mode == CMDPRIO_MODE_NONE)
+		return 0;
+
+	return fio_cmdprio_gen_data_structs(td, cmdprio);
 }
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 7e4fcf6c..0c7bd6cf 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -17,21 +17,26 @@ enum {
 	CMDPRIO_MODE_BSSPLIT,
 };
 
-struct cmdprio {
+struct cmdprio_options {
 	unsigned int percentage[CMDPRIO_RWDIR_CNT];
 	unsigned int class[CMDPRIO_RWDIR_CNT];
 	unsigned int level[CMDPRIO_RWDIR_CNT];
+	char *bssplit_str;
+};
+
+struct cmdprio {
+	struct cmdprio_options *options;
 	unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
 	struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
 	unsigned int mode;
 };
 
-int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
-			      struct cmdprio *cmdprio);
-
 bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
 			    struct io_u *io_u);
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio);
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+		     struct cmdprio_options *options);
 
 #endif
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 979d07d4..b2308b94 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -68,12 +68,14 @@ struct ioring_data {
 	int prepped;
 
 	struct ioring_mmap mmap[3];
+
+	struct cmdprio cmdprio;
 };
 
 struct ioring_options {
 	struct thread_data *td;
 	unsigned int hipri;
-	struct cmdprio cmdprio;
+	struct cmdprio_options cmdprio_options;
 	unsigned int fixedbufs;
 	unsigned int registerfiles;
 	unsigned int sqpoll_thread;
@@ -104,15 +106,6 @@ static int fio_ioring_sqpoll_cb(void *data, unsigned long long *val)
 	return 0;
 }
 
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
-	struct ioring_options *o = data;
-	struct thread_data *td = o->td;
-	struct cmdprio *cmdprio = &o->cmdprio;
-
-	return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
 static struct fio_option options[] = {
 	{
 		.name	= "hipri",
@@ -129,9 +122,9 @@ static struct fio_option options[] = {
 		.lname	= "high priority percentage",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct ioring_options,
-				   cmdprio.percentage[DDIR_READ]),
+				   cmdprio_options.percentage[DDIR_READ]),
 		.off2	= offsetof(struct ioring_options,
-				   cmdprio.percentage[DDIR_WRITE]),
+				   cmdprio_options.percentage[DDIR_WRITE]),
 		.minval	= 0,
 		.maxval	= 100,
 		.help	= "Send high priority I/O this percentage of the time",
@@ -143,9 +136,9 @@ static struct fio_option options[] = {
 		.lname	= "Asynchronous I/O priority class",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct ioring_options,
-				   cmdprio.class[DDIR_READ]),
+				   cmdprio_options.class[DDIR_READ]),
 		.off2	= offsetof(struct ioring_options,
-				   cmdprio.class[DDIR_WRITE]),
+				   cmdprio_options.class[DDIR_WRITE]),
 		.help	= "Set asynchronous IO priority class",
 		.minval	= IOPRIO_MIN_PRIO_CLASS + 1,
 		.maxval	= IOPRIO_MAX_PRIO_CLASS,
@@ -158,9 +151,9 @@ static struct fio_option options[] = {
 		.lname	= "Asynchronous I/O priority level",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct ioring_options,
-				   cmdprio.level[DDIR_READ]),
+				   cmdprio_options.level[DDIR_READ]),
 		.off2	= offsetof(struct ioring_options,
-				   cmdprio.level[DDIR_WRITE]),
+				   cmdprio_options.level[DDIR_WRITE]),
 		.help	= "Set asynchronous IO priority level",
 		.minval	= IOPRIO_MIN_PRIO,
 		.maxval	= IOPRIO_MAX_PRIO,
@@ -171,9 +164,9 @@ static struct fio_option options[] = {
 	{
 		.name   = "cmdprio_bssplit",
 		.lname  = "Priority percentage block size split",
-		.type   = FIO_OPT_STR_ULL,
-		.cb     = str_cmdprio_bssplit_cb,
-		.off1   = offsetof(struct ioring_options, cmdprio.bssplit),
+		.type   = FIO_OPT_STR_STORE,
+		.off1   = offsetof(struct ioring_options,
+				   cmdprio_options.bssplit_str),
 		.help   = "Set priority percentages for different block sizes",
 		.category = FIO_OPT_C_ENGINE,
 		.group	= FIO_OPT_G_IOURING,
@@ -458,8 +451,7 @@ static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
 					   struct io_u *io_u)
 {
 	struct ioring_data *ld = td->io_ops_data;
-	struct ioring_options *o = td->eo;
-	struct cmdprio *cmdprio = &o->cmdprio;
+	struct cmdprio *cmdprio = &ld->cmdprio;
 
 	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
 		ld->sqes[io_u->index].ioprio = io_u->ioprio;
@@ -470,7 +462,6 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 {
 	struct ioring_data *ld = td->io_ops_data;
 	struct io_sq_ring *ring = &ld->sq_ring;
-	struct ioring_options *o = td->eo;
 	unsigned tail, next_tail;
 
 	fio_ro_check(td, io_u);
@@ -493,7 +484,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 	if (next_tail == atomic_load_acquire(ring->head))
 		return FIO_Q_BUSY;
 
-	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+	if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
 		fio_ioring_cmdprio_prep(td, io_u);
 
 	ring->array[tail & ld->sq_ring_mask] = io_u->index;
@@ -599,6 +590,7 @@ static void fio_ioring_cleanup(struct thread_data *td)
 		if (!(td->flags & TD_F_CHILD))
 			fio_ioring_unmap(ld);
 
+		fio_cmdprio_cleanup(&ld->cmdprio);
 		free(ld->io_u_index);
 		free(ld->iovecs);
 		free(ld->fds);
@@ -805,7 +797,6 @@ static int fio_ioring_init(struct thread_data *td)
 {
 	struct ioring_options *o = td->eo;
 	struct ioring_data *ld;
-	struct cmdprio *cmdprio = &o->cmdprio;
 	int ret;
 
 	/* sqthread submission requires registered files */
@@ -830,7 +821,7 @@ static int fio_ioring_init(struct thread_data *td)
 
 	td->io_ops_data = ld;
 
-	ret = fio_cmdprio_init(td, cmdprio);
+	ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
 	if (ret) {
 		td_verror(td, EINVAL, "fio_ioring_init");
 		return 1;
diff --git a/engines/libaio.c b/engines/libaio.c
index 53fe7428..9c278d06 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -51,24 +51,17 @@ struct libaio_data {
 	unsigned int queued;
 	unsigned int head;
 	unsigned int tail;
+
+	struct cmdprio cmdprio;
 };
 
 struct libaio_options {
 	struct thread_data *td;
 	unsigned int userspace_reap;
-	struct cmdprio cmdprio;
+	struct cmdprio_options cmdprio_options;
 	unsigned int nowait;
 };
 
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
-	struct libaio_options *o = data;
-	struct thread_data *td = o->td;
-	struct cmdprio *cmdprio = &o->cmdprio;
-
-	return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
 static struct fio_option options[] = {
 	{
 		.name	= "userspace_reap",
@@ -85,9 +78,9 @@ static struct fio_option options[] = {
 		.lname	= "high priority percentage",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct libaio_options,
-				   cmdprio.percentage[DDIR_READ]),
+				   cmdprio_options.percentage[DDIR_READ]),
 		.off2	= offsetof(struct libaio_options,
-				   cmdprio.percentage[DDIR_WRITE]),
+				   cmdprio_options.percentage[DDIR_WRITE]),
 		.minval	= 0,
 		.maxval	= 100,
 		.help	= "Send high priority I/O this percentage of the time",
@@ -99,9 +92,9 @@ static struct fio_option options[] = {
 		.lname	= "Asynchronous I/O priority class",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct libaio_options,
-				   cmdprio.class[DDIR_READ]),
+				   cmdprio_options.class[DDIR_READ]),
 		.off2	= offsetof(struct libaio_options,
-				   cmdprio.class[DDIR_WRITE]),
+				   cmdprio_options.class[DDIR_WRITE]),
 		.help	= "Set asynchronous IO priority class",
 		.minval	= IOPRIO_MIN_PRIO_CLASS + 1,
 		.maxval	= IOPRIO_MAX_PRIO_CLASS,
@@ -114,9 +107,9 @@ static struct fio_option options[] = {
 		.lname	= "Asynchronous I/O priority level",
 		.type	= FIO_OPT_INT,
 		.off1	= offsetof(struct libaio_options,
-				   cmdprio.level[DDIR_READ]),
+				   cmdprio_options.level[DDIR_READ]),
 		.off2	= offsetof(struct libaio_options,
-				   cmdprio.level[DDIR_WRITE]),
+				   cmdprio_options.level[DDIR_WRITE]),
 		.help	= "Set asynchronous IO priority level",
 		.minval	= IOPRIO_MIN_PRIO,
 		.maxval	= IOPRIO_MAX_PRIO,
@@ -127,9 +120,9 @@ static struct fio_option options[] = {
 	{
 		.name   = "cmdprio_bssplit",
 		.lname  = "Priority percentage block size split",
-		.type   = FIO_OPT_STR_ULL,
-		.cb     = str_cmdprio_bssplit_cb,
-		.off1   = offsetof(struct libaio_options, cmdprio.bssplit),
+		.type   = FIO_OPT_STR_STORE,
+		.off1   = offsetof(struct libaio_options,
+				   cmdprio_options.bssplit_str),
 		.help   = "Set priority percentages for different block sizes",
 		.category = FIO_OPT_C_ENGINE,
 		.group	= FIO_OPT_G_LIBAIO,
@@ -206,8 +199,8 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
 static inline void fio_libaio_cmdprio_prep(struct thread_data *td,
 					   struct io_u *io_u)
 {
-	struct libaio_options *o = td->eo;
-	struct cmdprio *cmdprio = &o->cmdprio;
+	struct libaio_data *ld = td->io_ops_data;
+	struct cmdprio *cmdprio = &ld->cmdprio;
 
 	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
 		io_u->iocb.aio_reqprio = io_u->ioprio;
@@ -318,7 +311,6 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
 					  struct io_u *io_u)
 {
 	struct libaio_data *ld = td->io_ops_data;
-	struct libaio_options *o = td->eo;
 
 	fio_ro_check(td, io_u);
 
@@ -349,7 +341,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
 		return FIO_Q_COMPLETED;
 	}
 
-	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+	if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
 		fio_libaio_cmdprio_prep(td, io_u);
 
 	ld->iocbs[ld->head] = &io_u->iocb;
@@ -468,6 +460,8 @@ static void fio_libaio_cleanup(struct thread_data *td)
 		 */
 		if (!(td->flags & TD_F_CHILD))
 			io_destroy(ld->aio_ctx);
+
+		fio_cmdprio_cleanup(&ld->cmdprio);
 		free(ld->aio_events);
 		free(ld->iocbs);
 		free(ld->io_us);
@@ -493,7 +487,6 @@ static int fio_libaio_init(struct thread_data *td)
 {
 	struct libaio_data *ld;
 	struct libaio_options *o = td->eo;
-	struct cmdprio *cmdprio = &o->cmdprio;
 	int ret;
 
 	ld = calloc(1, sizeof(*ld));
@@ -506,7 +499,7 @@ static int fio_libaio_init(struct thread_data *td)
 
 	td->io_ops_data = ld;
 
-	ret = fio_cmdprio_init(td, cmdprio);
+	ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
 	if (ret) {
 		td_verror(td, EINVAL, "fio_libaio_init");
 		return 1;
-- 
2.33.1

Re: [PATCH v2 8/8] libaio,io_uring: make it possible to cleanup cmdprio malloced data

[Damien Le Moal]

On 2021/11/10 8:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The way that fio currently handles engine options:
> options_free() will only every call free() on an option that has type

...will call free() only for options that have the type...

> FIO_OPT_STR_STORE. This means that any option that has a pointer in
> either td->o or td->eo, which is not of type FIO_OPT_STR_STORE will
> leak memory. This is true even for numjobs == 1.
> 
> When running with numjobs > 1, fio_options_mem_dupe() will memcpy
> td->eo into the new td. Since off1 of the pointers in the first td
> has already been set, the pointers in the new td will point to the
> same data. (Regardless, options_free() will never try to free the
> memory, for neither td.) Neither can we manually free the memory in
> cleanup(), since the other td will still point to the same memory,
> so this would lead to a double free.
> 
> These memory leaks are reported by e.g. valgrind.
> 
> The most obvious way to solve this is to put dynamically allocated
> memory in {ioring,libaio}_data instead of {ioring,libaio}_options.
> 
> This solves the problem since {ioring,libaio}_data is dynamically
> allocated by each td during the ioengine init callback, and is freed
> when the ioengine cleanup callback for that td is called.
> 
> The downside of this is that the parsing has to be done in
> fio_cmdprio_init() instead of in the option .cb callback, since the
> .cb callback is called before {ioring,libaio}_data is available.
> 
> This patch keeps the static cmdprio options in
> {ioring,libaio}_options, but moves the dynamically allocated memory
> needed by cmdprio to {ioring,libaio}_data.
> 
> No cmdprio related memory leaks are reported after this patch.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/cmdprio.c  | 106 ++++++++++++++++++++++++++++++++++-----------
>  engines/cmdprio.h  |  15 ++++---
>  engines/io_uring.c |  41 +++++++-----------
>  engines/libaio.c   |  43 ++++++++----------
>  4 files changed, 124 insertions(+), 81 deletions(-)
> 
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 174bed7c..7ff39d93 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -46,22 +46,15 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  			      struct cmdprio *cmdprio)
>  {
>  	char *str, *p;
> -	int i, ret = 0;
> +	int ret = 0;
>  
>  	p = str = strdup(input);
>  
>  	strip_blank_front(&str);
>  	strip_blank_end(str);
>  
> -	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
> -
> -	if (parse_dryrun()) {
> -		for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
> -			free(cmdprio->bssplit[i]);
> -			cmdprio->bssplit[i] = NULL;
> -			cmdprio->bssplit_nr[i] = 0;
> -		}
> -	}
> +	ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio,
> +			      false);
>  
>  	free(p);
>  	return ret;
> @@ -70,11 +63,12 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
>  static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
>  {
>  	enum fio_ddir ddir = io_u->ddir;
> +	struct cmdprio_options *options = cmdprio->options;
>  	int i;
>  
>  	switch (cmdprio->mode) {
>  	case CMDPRIO_MODE_PERC:
> -		return cmdprio->percentage[ddir];
> +		return options->percentage[ddir];
>  	case CMDPRIO_MODE_BSSPLIT:
>  		for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
>  			if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> @@ -98,9 +92,10 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  			    struct io_u *io_u)
>  {
>  	enum fio_ddir ddir = io_u->ddir;
> +	struct cmdprio_options *options = cmdprio->options;
>  	unsigned int p;
>  	unsigned int cmdprio_value =
> -		ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> +		ioprio_value(options->class[ddir], options->level[ddir]);
>  
>  	if (cmdprio->mode == CMDPRIO_MODE_NONE) {
>  		/*
> @@ -137,30 +132,85 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  	return false;
>  }
>  
> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
> +static int fio_cmdprio_parse_and_gen_bssplit(struct thread_data *td,
> +					     struct cmdprio *cmdprio)
>  {
> -	struct thread_options *to = &td->o;
> -	bool has_cmdprio_percentage = false;
> -	bool has_cmdprio_bssplit = false;
> +	struct cmdprio_options *options = cmdprio->options;
> +	int ret;
> +
> +	ret = fio_cmdprio_bssplit_parse(td, options->bssplit_str, cmdprio);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	fio_cmdprio_cleanup(cmdprio);
> +
> +	return ret;
> +}
> +
> +static int fio_cmdprio_gen_data_structs(struct thread_data *td,
> +					struct cmdprio *cmdprio)

You could call this fio_cmdprio_parse_and_gen(), or simply
fio_cmdprio_parse() ?

> +{
> +	struct cmdprio_options *options = cmdprio->options;
> +	int ret = 1;
>  	int i;
>  
> +	if (cmdprio->mode == CMDPRIO_MODE_BSSPLIT)
> +		ret = fio_cmdprio_parse_and_gen_bssplit(td, cmdprio);
> +	else if (cmdprio->mode == CMDPRIO_MODE_PERC)
> +		ret = 0;

What about:

	int ret;

	if (cmdprio->mode == CMDPRIO_MODE_NONE)
		return 1;

	if (cmdprio->mode == CMDPRIO_MODE_BSSPLIT) {
		ret = fio_cmdprio_parse_and_gen_bssplit(td, cmdprio);
		if (ret)
			return ret;
	}

And then "return 0;" at the end.

That is easier to follow this way, no ?

> +
>  	/*
>  	 * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
>  	 * is not set, default to RT priority class.
>  	 */
>  	for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
> -		if (cmdprio->percentage[i]) {
> -			if (!cmdprio->class[i])
> -				cmdprio->class[i] = IOPRIO_CLASS_RT;
> -			has_cmdprio_percentage = true;
> -		}
> -		if (cmdprio->bssplit_nr[i]) {
> -			if (!cmdprio->class[i])
> -				cmdprio->class[i] = IOPRIO_CLASS_RT;
> -			has_cmdprio_bssplit = true;
> +		if (options->percentage[i] || cmdprio->bssplit_nr[i]) {
> +			if (!options->class[i])
> +				options->class[i] = IOPRIO_CLASS_RT;
>  		}
>  	}
>  
> +	return ret;
> +}
> +
> +void fio_cmdprio_cleanup(struct cmdprio *cmdprio)
> +{
> +	int ddir;
> +
> +	for (ddir = 0; ddir < CMDPRIO_RWDIR_CNT; ddir++) {
> +		free(cmdprio->bssplit[ddir]);
> +		cmdprio->bssplit[ddir] = NULL;
> +		cmdprio->bssplit_nr[ddir] = 0;
> +	}
> +
> +	/*
> +	 * options points to a cmdprio_options struct that is part of td->eo.
> +	 * td->eo itself will be freed by free_ioengine().
> +	 */
> +	cmdprio->options = NULL;
> +}
> +
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> +		     struct cmdprio_options *options)
> +{
> +	struct thread_options *to = &td->o;
> +	bool has_cmdprio_percentage = false;
> +	bool has_cmdprio_bssplit = false;
> +	int i;
> +
> +	cmdprio->options = options;
> +
> +	if (options->bssplit_str && strlen(options->bssplit_str))
> +		has_cmdprio_bssplit = true;
> +
> +	for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
> +		if (options->percentage[i])
> +			has_cmdprio_percentage = true;
> +	}
> +
>  	/*
>  	 * Check for option conflicts
>  	 */
> @@ -178,5 +228,9 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
>  	else
>  		cmdprio->mode = CMDPRIO_MODE_NONE;
>  
> -	return 0;
> +	/* Nothing left to do if cmdprio is not used */
> +	if (cmdprio->mode == CMDPRIO_MODE_NONE)
> +		return 0;
> +
> +	return fio_cmdprio_gen_data_structs(td, cmdprio);
>  }
> diff --git a/engines/cmdprio.h b/engines/cmdprio.h
> index 7e4fcf6c..0c7bd6cf 100644
> --- a/engines/cmdprio.h
> +++ b/engines/cmdprio.h
> @@ -17,21 +17,26 @@ enum {
>  	CMDPRIO_MODE_BSSPLIT,
>  };
>  
> -struct cmdprio {
> +struct cmdprio_options {
>  	unsigned int percentage[CMDPRIO_RWDIR_CNT];
>  	unsigned int class[CMDPRIO_RWDIR_CNT];
>  	unsigned int level[CMDPRIO_RWDIR_CNT];
> +	char *bssplit_str;
> +};
> +
> +struct cmdprio {
> +	struct cmdprio_options *options;
>  	unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
>  	struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
>  	unsigned int mode;
>  };
>  
> -int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> -			      struct cmdprio *cmdprio);
> -
>  bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
>  			    struct io_u *io_u);
>  
> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
> +void fio_cmdprio_cleanup(struct cmdprio *cmdprio);
> +
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> +		     struct cmdprio_options *options);
>  
>  #endif
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 979d07d4..b2308b94 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -68,12 +68,14 @@ struct ioring_data {
>  	int prepped;
>  
>  	struct ioring_mmap mmap[3];
> +
> +	struct cmdprio cmdprio;
>  };
>  
>  struct ioring_options {
>  	struct thread_data *td;
>  	unsigned int hipri;
> -	struct cmdprio cmdprio;
> +	struct cmdprio_options cmdprio_options;
>  	unsigned int fixedbufs;
>  	unsigned int registerfiles;
>  	unsigned int sqpoll_thread;
> @@ -104,15 +106,6 @@ static int fio_ioring_sqpoll_cb(void *data, unsigned long long *val)
>  	return 0;
>  }
>  
> -static int str_cmdprio_bssplit_cb(void *data, const char *input)
> -{
> -	struct ioring_options *o = data;
> -	struct thread_data *td = o->td;
> -	struct cmdprio *cmdprio = &o->cmdprio;
> -
> -	return fio_cmdprio_bssplit_parse(td, input, cmdprio);
> -}
> -
>  static struct fio_option options[] = {
>  	{
>  		.name	= "hipri",
> @@ -129,9 +122,9 @@ static struct fio_option options[] = {
>  		.lname	= "high priority percentage",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct ioring_options,
> -				   cmdprio.percentage[DDIR_READ]),
> +				   cmdprio_options.percentage[DDIR_READ]),
>  		.off2	= offsetof(struct ioring_options,
> -				   cmdprio.percentage[DDIR_WRITE]),
> +				   cmdprio_options.percentage[DDIR_WRITE]),
>  		.minval	= 0,
>  		.maxval	= 100,
>  		.help	= "Send high priority I/O this percentage of the time",
> @@ -143,9 +136,9 @@ static struct fio_option options[] = {
>  		.lname	= "Asynchronous I/O priority class",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct ioring_options,
> -				   cmdprio.class[DDIR_READ]),
> +				   cmdprio_options.class[DDIR_READ]),
>  		.off2	= offsetof(struct ioring_options,
> -				   cmdprio.class[DDIR_WRITE]),
> +				   cmdprio_options.class[DDIR_WRITE]),
>  		.help	= "Set asynchronous IO priority class",
>  		.minval	= IOPRIO_MIN_PRIO_CLASS + 1,
>  		.maxval	= IOPRIO_MAX_PRIO_CLASS,
> @@ -158,9 +151,9 @@ static struct fio_option options[] = {
>  		.lname	= "Asynchronous I/O priority level",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct ioring_options,
> -				   cmdprio.level[DDIR_READ]),
> +				   cmdprio_options.level[DDIR_READ]),
>  		.off2	= offsetof(struct ioring_options,
> -				   cmdprio.level[DDIR_WRITE]),
> +				   cmdprio_options.level[DDIR_WRITE]),
>  		.help	= "Set asynchronous IO priority level",
>  		.minval	= IOPRIO_MIN_PRIO,
>  		.maxval	= IOPRIO_MAX_PRIO,
> @@ -171,9 +164,9 @@ static struct fio_option options[] = {
>  	{
>  		.name   = "cmdprio_bssplit",
>  		.lname  = "Priority percentage block size split",
> -		.type   = FIO_OPT_STR_ULL,
> -		.cb     = str_cmdprio_bssplit_cb,
> -		.off1   = offsetof(struct ioring_options, cmdprio.bssplit),
> +		.type   = FIO_OPT_STR_STORE,
> +		.off1   = offsetof(struct ioring_options,
> +				   cmdprio_options.bssplit_str),
>  		.help   = "Set priority percentages for different block sizes",
>  		.category = FIO_OPT_C_ENGINE,
>  		.group	= FIO_OPT_G_IOURING,
> @@ -458,8 +451,7 @@ static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
>  					   struct io_u *io_u)
>  {
>  	struct ioring_data *ld = td->io_ops_data;
> -	struct ioring_options *o = td->eo;
> -	struct cmdprio *cmdprio = &o->cmdprio;
> +	struct cmdprio *cmdprio = &ld->cmdprio;
>  
>  	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
>  		ld->sqes[io_u->index].ioprio = io_u->ioprio;
> @@ -470,7 +462,6 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
>  {
>  	struct ioring_data *ld = td->io_ops_data;
>  	struct io_sq_ring *ring = &ld->sq_ring;
> -	struct ioring_options *o = td->eo;
>  	unsigned tail, next_tail;
>  
>  	fio_ro_check(td, io_u);
> @@ -493,7 +484,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
>  	if (next_tail == atomic_load_acquire(ring->head))
>  		return FIO_Q_BUSY;
>  
> -	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
> +	if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
>  		fio_ioring_cmdprio_prep(td, io_u);
>  
>  	ring->array[tail & ld->sq_ring_mask] = io_u->index;
> @@ -599,6 +590,7 @@ static void fio_ioring_cleanup(struct thread_data *td)
>  		if (!(td->flags & TD_F_CHILD))
>  			fio_ioring_unmap(ld);
>  
> +		fio_cmdprio_cleanup(&ld->cmdprio);
>  		free(ld->io_u_index);
>  		free(ld->iovecs);
>  		free(ld->fds);
> @@ -805,7 +797,6 @@ static int fio_ioring_init(struct thread_data *td)
>  {
>  	struct ioring_options *o = td->eo;
>  	struct ioring_data *ld;
> -	struct cmdprio *cmdprio = &o->cmdprio;
>  	int ret;
>  
>  	/* sqthread submission requires registered files */
> @@ -830,7 +821,7 @@ static int fio_ioring_init(struct thread_data *td)
>  
>  	td->io_ops_data = ld;
>  
> -	ret = fio_cmdprio_init(td, cmdprio);
> +	ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
>  	if (ret) {
>  		td_verror(td, EINVAL, "fio_ioring_init");
>  		return 1;
> diff --git a/engines/libaio.c b/engines/libaio.c
> index 53fe7428..9c278d06 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -51,24 +51,17 @@ struct libaio_data {
>  	unsigned int queued;
>  	unsigned int head;
>  	unsigned int tail;
> +
> +	struct cmdprio cmdprio;
>  };
>  
>  struct libaio_options {
>  	struct thread_data *td;
>  	unsigned int userspace_reap;
> -	struct cmdprio cmdprio;
> +	struct cmdprio_options cmdprio_options;
>  	unsigned int nowait;
>  };
>  
> -static int str_cmdprio_bssplit_cb(void *data, const char *input)
> -{
> -	struct libaio_options *o = data;
> -	struct thread_data *td = o->td;
> -	struct cmdprio *cmdprio = &o->cmdprio;
> -
> -	return fio_cmdprio_bssplit_parse(td, input, cmdprio);
> -}
> -
>  static struct fio_option options[] = {
>  	{
>  		.name	= "userspace_reap",
> @@ -85,9 +78,9 @@ static struct fio_option options[] = {
>  		.lname	= "high priority percentage",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct libaio_options,
> -				   cmdprio.percentage[DDIR_READ]),
> +				   cmdprio_options.percentage[DDIR_READ]),
>  		.off2	= offsetof(struct libaio_options,
> -				   cmdprio.percentage[DDIR_WRITE]),
> +				   cmdprio_options.percentage[DDIR_WRITE]),
>  		.minval	= 0,
>  		.maxval	= 100,
>  		.help	= "Send high priority I/O this percentage of the time",
> @@ -99,9 +92,9 @@ static struct fio_option options[] = {
>  		.lname	= "Asynchronous I/O priority class",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct libaio_options,
> -				   cmdprio.class[DDIR_READ]),
> +				   cmdprio_options.class[DDIR_READ]),
>  		.off2	= offsetof(struct libaio_options,
> -				   cmdprio.class[DDIR_WRITE]),
> +				   cmdprio_options.class[DDIR_WRITE]),
>  		.help	= "Set asynchronous IO priority class",
>  		.minval	= IOPRIO_MIN_PRIO_CLASS + 1,
>  		.maxval	= IOPRIO_MAX_PRIO_CLASS,
> @@ -114,9 +107,9 @@ static struct fio_option options[] = {
>  		.lname	= "Asynchronous I/O priority level",
>  		.type	= FIO_OPT_INT,
>  		.off1	= offsetof(struct libaio_options,
> -				   cmdprio.level[DDIR_READ]),
> +				   cmdprio_options.level[DDIR_READ]),
>  		.off2	= offsetof(struct libaio_options,
> -				   cmdprio.level[DDIR_WRITE]),
> +				   cmdprio_options.level[DDIR_WRITE]),
>  		.help	= "Set asynchronous IO priority level",
>  		.minval	= IOPRIO_MIN_PRIO,
>  		.maxval	= IOPRIO_MAX_PRIO,
> @@ -127,9 +120,9 @@ static struct fio_option options[] = {
>  	{
>  		.name   = "cmdprio_bssplit",
>  		.lname  = "Priority percentage block size split",
> -		.type   = FIO_OPT_STR_ULL,
> -		.cb     = str_cmdprio_bssplit_cb,
> -		.off1   = offsetof(struct libaio_options, cmdprio.bssplit),
> +		.type   = FIO_OPT_STR_STORE,
> +		.off1   = offsetof(struct libaio_options,
> +				   cmdprio_options.bssplit_str),
>  		.help   = "Set priority percentages for different block sizes",
>  		.category = FIO_OPT_C_ENGINE,
>  		.group	= FIO_OPT_G_LIBAIO,
> @@ -206,8 +199,8 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
>  static inline void fio_libaio_cmdprio_prep(struct thread_data *td,
>  					   struct io_u *io_u)
>  {
> -	struct libaio_options *o = td->eo;
> -	struct cmdprio *cmdprio = &o->cmdprio;
> +	struct libaio_data *ld = td->io_ops_data;
> +	struct cmdprio *cmdprio = &ld->cmdprio;
>  
>  	if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
>  		io_u->iocb.aio_reqprio = io_u->ioprio;
> @@ -318,7 +311,6 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
>  					  struct io_u *io_u)
>  {
>  	struct libaio_data *ld = td->io_ops_data;
> -	struct libaio_options *o = td->eo;
>  
>  	fio_ro_check(td, io_u);
>  
> @@ -349,7 +341,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
>  		return FIO_Q_COMPLETED;
>  	}
>  
> -	if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
> +	if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
>  		fio_libaio_cmdprio_prep(td, io_u);
>  
>  	ld->iocbs[ld->head] = &io_u->iocb;
> @@ -468,6 +460,8 @@ static void fio_libaio_cleanup(struct thread_data *td)
>  		 */
>  		if (!(td->flags & TD_F_CHILD))
>  			io_destroy(ld->aio_ctx);
> +
> +		fio_cmdprio_cleanup(&ld->cmdprio);
>  		free(ld->aio_events);
>  		free(ld->iocbs);
>  		free(ld->io_us);
> @@ -493,7 +487,6 @@ static int fio_libaio_init(struct thread_data *td)
>  {
>  	struct libaio_data *ld;
>  	struct libaio_options *o = td->eo;
> -	struct cmdprio *cmdprio = &o->cmdprio;
>  	int ret;
>  
>  	ld = calloc(1, sizeof(*ld));
> @@ -506,7 +499,7 @@ static int fio_libaio_init(struct thread_data *td)
>  
>  	td->io_ops_data = ld;
>  
> -	ret = fio_cmdprio_init(td, cmdprio);
> +	ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
>  	if (ret) {
>  		td_verror(td, EINVAL, "fio_libaio_init");
>  		return 1;
> 


-- 
Damien Le Moal
Western Digital Research