[PATCH 0/3] fdp fixes, random placment id and xnvme support

[PATCH 0/3] fdp fixes, random placment id and xnvme support

[Ankit Kumar]

This series now enables us to either roundrobin or select a random
placement ID from the available ones. In future a non-unifrom
placement id selection such as zipf, pareto etc can be added.

The current boundary check for input placment ids was not correct.
Added a fix for that.

This series also adds FDP support to the xnvme I/O engine.

Ankit Kumar (3):
  fdp: use macro for max ruhs and fix placement id check
  fdp: support random placement id selection
  engines/xnvme: add support for fdp

 HOWTO.rst              | 19 ++++++++--
 configure              |  2 +-
 engines/io_uring.c     |  2 +-
 engines/xnvme.c        | 78 +++++++++++++++++++++++++++++++++++++++++-
 examples/xnvme-fdp.fio | 36 +++++++++++++++++++
 fdp.c                  | 20 +++++++----
 fdp.h                  | 12 +++++++
 fio.1                  | 20 +++++++++--
 fio.h                  |  2 ++
 init.c                 |  2 ++
 options.c              | 20 +++++++++++
 thread_options.h       |  2 ++
 12 files changed, 201 insertions(+), 14 deletions(-)
 create mode 100644 examples/xnvme-fdp.fio

-- 
2.25.1

[PATCH 1/3] fdp: use macro for max ruhs and fix placement id check

[Ankit Kumar]

Use macro for max ruhs.
Number of reclaim unit handle descriptors are 1 based, whereas the
input placement id index / indices are 0 based. Add the correct check
for that.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 engines/io_uring.c | 2 +-
 fdp.c              | 8 ++++----
 fdp.h              | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 5021239e..407d65ce 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -1310,7 +1310,7 @@ static int fio_ioring_cmd_fetch_ruhs(struct thread_data *td, struct fio_file *f,
 	struct nvme_fdp_ruh_status *ruhs;
 	int bytes, ret, i;
 
-	bytes = sizeof(*ruhs) + 128 * sizeof(struct nvme_fdp_ruh_status_desc);
+	bytes = sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(struct nvme_fdp_ruh_status_desc);
 	ruhs = scalloc(1, bytes);
 	if (!ruhs)
 		return -ENOMEM;
diff --git a/fdp.c b/fdp.c
index d92dbc67..72afca01 100644
--- a/fdp.c
+++ b/fdp.c
@@ -45,7 +45,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
 	struct fio_ruhs_info *ruhs, *tmp;
 	int i, ret;
 
-	ruhs = scalloc(1, sizeof(*ruhs) + 128 * sizeof(*ruhs->plis));
+	ruhs = scalloc(1, sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(*ruhs->plis));
 	if (!ruhs)
 		return -ENOMEM;
 
@@ -56,8 +56,8 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
 		goto out;
 	}
 
-	if (ruhs->nr_ruhs > 128)
-		ruhs->nr_ruhs = 128;
+	if (ruhs->nr_ruhs > FDP_MAX_RUHS)
+		ruhs->nr_ruhs = FDP_MAX_RUHS;
 
 	if (td->o.fdp_nrpli == 0) {
 		f->ruhs_info = ruhs;
@@ -65,7 +65,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
 	}
 
 	for (i = 0; i < td->o.fdp_nrpli; i++) {
-		if (td->o.fdp_plis[i] > ruhs->nr_ruhs) {
+		if (td->o.fdp_plis[i] >= ruhs->nr_ruhs) {
 			ret = -EINVAL;
 			goto out;
 		}
diff --git a/fdp.h b/fdp.h
index 81691f62..4a5e0e54 100644
--- a/fdp.h
+++ b/fdp.h
@@ -3,6 +3,8 @@
 
 #include "io_u.h"
 
+#define FDP_MAX_RUHS	128
+
 struct fio_ruhs_info {
 	uint32_t nr_ruhs;
 	uint32_t pli_loc;
-- 
2.25.1

[PATCH 2/3] fdp: support random placement id selection

[Ankit Kumar]

Allow user to either roundrobin or select random placement ID from
the available placement IDs.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 HOWTO.rst        | 15 +++++++++++++++
 fdp.c            | 12 +++++++++---
 fdp.h            | 10 ++++++++++
 fio.1            | 16 ++++++++++++++++
 fio.h            |  2 ++
 init.c           |  2 ++
 options.c        | 20 ++++++++++++++++++++
 thread_options.h |  2 ++
 8 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/HOWTO.rst b/HOWTO.rst
index 2e1e55c2..24789f41 100644
--- a/HOWTO.rst
+++ b/HOWTO.rst
@@ -2435,6 +2435,21 @@ with the caveat that when used on the command line, they must come after the
 
 	Enable Flexible Data Placement mode for write commands.
 
+.. option:: fdp_pli_select=str : [io_uring_cmd]
+
+	Defines how fio decides which placement ID to use next. The following
+	types are defined:
+
+		**random**
+			Choose a placement ID at random (uniform).
+
+		**roundrobin**
+			Round robin over available placement IDs. This is the
+			default.
+
+	The available placement ID index/indices is defined by the option
+	:option:`fdp_pli`.
+
 .. option:: fdp_pli=str : [io_uring_cmd]
 
 	Select which Placement ID Index/Indicies this job is allowed to use for
diff --git a/fdp.c b/fdp.c
index 72afca01..7ac0ab55 100644
--- a/fdp.c
+++ b/fdp.c
@@ -119,10 +119,16 @@ void fdp_fill_dspec_data(struct thread_data *td, struct io_u *io_u)
 		return;
 	}
 
-	if (ruhs->pli_loc >= ruhs->nr_ruhs)
-		ruhs->pli_loc = 0;
+	if (td->o.fdp_pli_select == FIO_FDP_RR) {
+		if (ruhs->pli_loc >= ruhs->nr_ruhs)
+			ruhs->pli_loc = 0;
+
+		dspec = ruhs->plis[ruhs->pli_loc++];
+	} else {
+		ruhs->pli_loc = rand_between(&td->fdp_state, 0, ruhs->nr_ruhs - 1);
+		dspec = ruhs->plis[ruhs->pli_loc];
+	}
 
-	dspec = ruhs->plis[ruhs->pli_loc++];
 	io_u->dtype = 2;
 	io_u->dspec = dspec;
 }
diff --git a/fdp.h b/fdp.h
index 4a5e0e54..86d8d9e6 100644
--- a/fdp.h
+++ b/fdp.h
@@ -5,6 +5,16 @@
 
 #define FDP_MAX_RUHS	128
 
+/*
+ * How fio chooses what placement identifier to use next. Choice of
+ * uniformly random, or roundrobin.
+ */
+
+enum {
+	FIO_FDP_RANDOM	= 0x1,
+	FIO_FDP_RR	= 0x2,
+};
+
 struct fio_ruhs_info {
 	uint32_t nr_ruhs;
 	uint32_t pli_loc;
diff --git a/fio.1 b/fio.1
index 73b7e8c9..0257513b 100644
--- a/fio.1
+++ b/fio.1
@@ -2195,6 +2195,22 @@ file blocks are fully allocated and the disk request could be issued immediately
 .BI (io_uring_cmd)fdp \fR=\fPbool
 Enable Flexible Data Placement mode for write commands.
 .TP
+.BI (io_uring_cmd)fdp_pli_select \fR=\fPstr
+Defines how fio decides which placement ID to use next. The following types
+are defined:
+.RS
+.RS
+.TP
+.B random
+Choose a placement ID at random (uniform).
+.TP
+.B roundrobin
+Round robin over available placement IDs. This is the default.
+.RE
+.P
+The available placement ID index/indices is defined by \fBfdp_pli\fR option.
+.RE
+.TP
 .BI (io_uring_cmd)fdp_pli \fR=\fPstr
 Select which Placement ID Index/Indicies this job is allowed to use for writes.
 By default, the job will cycle through all available Placement IDs, so use this
diff --git a/fio.h b/fio.h
index c5453d13..a54f57c9 100644
--- a/fio.h
+++ b/fio.h
@@ -144,6 +144,7 @@ enum {
 	FIO_RAND_POISSON3_OFF,
 	FIO_RAND_PRIO_CMDS,
 	FIO_RAND_DEDUPE_WORKING_SET_IX,
+	FIO_RAND_FDP_OFF,
 	FIO_RAND_NR_OFFS,
 };
 
@@ -262,6 +263,7 @@ struct thread_data {
 	struct frand_state verify_state_last_do_io;
 	struct frand_state trim_state;
 	struct frand_state delay_state;
+	struct frand_state fdp_state;
 
 	struct frand_state buf_state;
 	struct frand_state buf_state_prev;
diff --git a/init.c b/init.c
index 10e63cca..105339fa 100644
--- a/init.c
+++ b/init.c
@@ -1082,6 +1082,8 @@ void td_fill_rand_seeds(struct thread_data *td)
 
 	init_rand_seed(&td->buf_state, td->rand_seeds[FIO_RAND_BUF_OFF], use64);
 	frand_copy(&td->buf_state_prev, &td->buf_state);
+
+	init_rand_seed(&td->fdp_state, td->rand_seeds[FIO_RAND_FDP_OFF], use64);
 }
 
 static int setup_random_seeds(struct thread_data *td)
diff --git a/options.c b/options.c
index a7c4ef6e..0f739317 100644
--- a/options.c
+++ b/options.c
@@ -3679,6 +3679,26 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.category = FIO_OPT_C_IO,
 		.group  = FIO_OPT_G_INVALID,
 	},
+	{
+		.name	= "fdp_pli_select",
+		.lname	= "FDP Placement ID select",
+		.type	= FIO_OPT_STR,
+		.off1	= offsetof(struct thread_options, fdp_pli_select),
+		.help	= "Select which FDP placement ID to use next",
+		.def	= "roundrobin",
+		.category = FIO_OPT_C_IO,
+		.group	= FIO_OPT_G_INVALID,
+		.posval	= {
+			  { .ival = "random",
+			    .oval = FIO_FDP_RANDOM,
+			    .help = "Choose a Placement ID at random (uniform)",
+			  },
+			  { .ival = "roundrobin",
+			    .oval = FIO_FDP_RR,
+			    .help = "Round robin select Placement IDs",
+			  },
+		},
+	},
 	{
 		.name	= "fdp_pli",
 		.lname	= "FDP Placement ID indicies",
diff --git a/thread_options.h b/thread_options.h
index a24ebee6..1715b36c 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -388,6 +388,7 @@ struct thread_options {
 
 #define FIO_MAX_PLIS 16
 	unsigned int fdp;
+	unsigned int fdp_pli_select;
 	unsigned int fdp_plis[FIO_MAX_PLIS];
 	unsigned int fdp_nrpli;
 
@@ -703,6 +704,7 @@ struct thread_options_pack {
 	uint32_t log_prio;
 
 	uint32_t fdp;
+	uint32_t fdp_pli_select;
 	uint32_t fdp_plis[FIO_MAX_PLIS];
 	uint32_t fdp_nrpli;
 
-- 
2.25.1

[PATCH 3/3] engines/xnvme: add support for fdp

[Ankit Kumar]

Added FDP support to xnvme I/O engine. This support can be used only with
nvme-ns generic character device (/dev/ngXnY). The available backends are
--xnvme_async=io_uring_cmd and --xnvme_sync=nvme.
Added a xnvme-fdp config example file.

Updated the minimum required xnvme version to 0.7.0

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 HOWTO.rst              |  6 ++--
 configure              |  2 +-
 engines/xnvme.c        | 78 +++++++++++++++++++++++++++++++++++++++++-
 examples/xnvme-fdp.fio | 36 +++++++++++++++++++
 fio.1                  |  6 ++--
 5 files changed, 120 insertions(+), 8 deletions(-)
 create mode 100644 examples/xnvme-fdp.fio

diff --git a/HOWTO.rst b/HOWTO.rst
index 24789f41..b047877e 100644
--- a/HOWTO.rst
+++ b/HOWTO.rst
@@ -2431,11 +2431,11 @@ with the caveat that when used on the command line, they must come after the
 	For direct I/O, requests will only succeed if cache invalidation isn't required,
 	file blocks are fully allocated and the disk request could be issued immediately.
 
-.. option:: fdp=bool : [io_uring_cmd]
+.. option:: fdp=bool : [io_uring_cmd] [xnvme]
 
 	Enable Flexible Data Placement mode for write commands.
 
-.. option:: fdp_pli_select=str : [io_uring_cmd]
+.. option:: fdp_pli_select=str : [io_uring_cmd] [xnvme]
 
 	Defines how fio decides which placement ID to use next. The following
 	types are defined:
@@ -2450,7 +2450,7 @@ with the caveat that when used on the command line, they must come after the
 	The available placement ID index/indices is defined by the option
 	:option:`fdp_pli`.
 
-.. option:: fdp_pli=str : [io_uring_cmd]
+.. option:: fdp_pli=str : [io_uring_cmd] [xnvme]
 
 	Select which Placement ID Index/Indicies this job is allowed to use for
 	writes. By default, the job will cycle through all available Placement
diff --git a/configure b/configure
index 74416fd4..6c938251 100755
--- a/configure
+++ b/configure
@@ -2651,7 +2651,7 @@ fi
 ##########################################
 # Check if we have xnvme
 if test "$xnvme" != "no" ; then
-  if check_min_lib_version xnvme 0.2.0; then
+  if check_min_lib_version xnvme 0.7.0; then
     xnvme="yes"
     xnvme_cflags=$(pkg-config --cflags xnvme)
     xnvme_libs=$(pkg-config --libs xnvme)
diff --git a/engines/xnvme.c b/engines/xnvme.c
index bb92a121..ce7b2bdd 100644
--- a/engines/xnvme.c
+++ b/engines/xnvme.c
@@ -16,6 +16,7 @@
 #include <libxnvme_spec_fs.h>
 #include "fio.h"
 #include "zbd_types.h"
+#include "fdp.h"
 #include "optgroup.h"
 
 static pthread_mutex_t g_serialize = PTHREAD_MUTEX_INITIALIZER;
@@ -509,6 +510,7 @@ static enum fio_q_status xnvme_fioe_queue(struct thread_data *td, struct io_u *i
 	uint16_t nlb;
 	int err;
 	bool vectored_io = ((struct xnvme_fioe_options *)td->eo)->xnvme_iovec;
+	uint32_t dir = io_u->dtype;
 
 	fio_ro_check(td, io_u);
 
@@ -524,6 +526,10 @@ static enum fio_q_status xnvme_fioe_queue(struct thread_data *td, struct io_u *i
 	ctx->cmd.common.nsid = nsid;
 	ctx->cmd.nvm.slba = slba;
 	ctx->cmd.nvm.nlb = nlb;
+	if (dir) {
+		ctx->cmd.nvm.dtype = io_u->dtype;
+		ctx->cmd.nvm.cdw13.dspec = io_u->dspec;
+	}
 
 	switch (io_u->ddir) {
 	case DDIR_READ:
@@ -947,6 +953,72 @@ exit:
 	return err;
 }
 
+static int xnvme_fioe_fetch_ruhs(struct thread_data *td, struct fio_file *f,
+				 struct fio_ruhs_info *fruhs_info)
+{
+	struct xnvme_opts opts = xnvme_opts_from_fioe(td);
+	struct xnvme_dev *dev;
+	struct xnvme_spec_ruhs *ruhs;
+	struct xnvme_cmd_ctx ctx;
+	uint32_t ruhs_nbytes;
+	uint32_t nsid;
+	int err = 0, err_lock;
+
+	if (f->filetype != FIO_TYPE_CHAR) {
+		log_err("ioeng->fdp_ruhs(): ignoring filetype: %d\n", f->filetype);
+		return -EINVAL;
+	}
+
+	err = pthread_mutex_lock(&g_serialize);
+	if (err) {
+		log_err("ioeng->fdp_ruhs(): pthread_mutex_lock(), err(%d)\n", err);
+		return -err;
+	}
+
+	dev = xnvme_dev_open(f->file_name, &opts);
+	if (!dev) {
+		log_err("ioeng->fdp_ruhs(): xnvme_dev_open(%s) failed, errno: %d\n",
+			f->file_name, errno);
+		err = -errno;
+		goto exit;
+	}
+
+	ruhs_nbytes = sizeof(*ruhs) + (FDP_MAX_RUHS * sizeof(struct xnvme_spec_ruhs_desc));
+	ruhs = xnvme_buf_alloc(dev, ruhs_nbytes);
+	if (!ruhs) {
+		err = -errno;
+		goto exit;
+	}
+	memset(ruhs, 0, ruhs_nbytes);
+
+	ctx = xnvme_cmd_ctx_from_dev(dev);
+	nsid = xnvme_dev_get_nsid(dev);
+
+	err = xnvme_nvm_mgmt_recv(&ctx, nsid, XNVME_SPEC_IO_MGMT_RECV_RUHS, 0, ruhs, ruhs_nbytes);
+
+	if (err || xnvme_cmd_ctx_cpl_status(&ctx)) {
+		err = err ? err : -EIO;
+		log_err("ioeng->fdp_ruhs(): err(%d), sc(%d)", err, ctx.cpl.status.sc);
+		goto free_buffer;
+	}
+
+	fruhs_info->nr_ruhs = ruhs->nruhsd;
+	for (uint32_t idx = 0; idx < fruhs_info->nr_ruhs; ++idx) {
+		fruhs_info->plis[idx] = le16_to_cpu(ruhs->desc[idx].pi);
+	}
+
+free_buffer:
+	xnvme_buf_free(dev, ruhs);
+exit:
+	xnvme_dev_close(dev);
+
+	err_lock = pthread_mutex_unlock(&g_serialize);
+	if (err_lock)
+		log_err("ioeng->fdp_ruhs(): pthread_mutex_unlock(), err(%d)\n", err_lock);
+
+	return err;
+}
+
 static int xnvme_fioe_get_file_size(struct thread_data *td, struct fio_file *f)
 {
 	struct xnvme_opts opts = xnvme_opts_from_fioe(td);
@@ -971,7 +1043,9 @@ static int xnvme_fioe_get_file_size(struct thread_data *td, struct fio_file *f)
 
 	f->real_file_size = xnvme_dev_get_geo(dev)->tbytes;
 	fio_file_set_size_known(f);
-	f->filetype = FIO_TYPE_BLOCK;
+
+	if (td->o.zone_mode == ZONE_MODE_ZBD)
+		f->filetype = FIO_TYPE_BLOCK;
 
 exit:
 	xnvme_dev_close(dev);
@@ -1011,6 +1085,8 @@ FIO_STATIC struct ioengine_ops ioengine = {
 	.get_zoned_model = xnvme_fioe_get_zoned_model,
 	.report_zones = xnvme_fioe_report_zones,
 	.reset_wp = xnvme_fioe_reset_wp,
+
+	.fdp_fetch_ruhs = xnvme_fioe_fetch_ruhs,
 };
 
 static void fio_init fio_xnvme_register(void)
diff --git a/examples/xnvme-fdp.fio b/examples/xnvme-fdp.fio
new file mode 100644
index 00000000..86fbe0d3
--- /dev/null
+++ b/examples/xnvme-fdp.fio
@@ -0,0 +1,36 @@
+; README
+;
+; This job-file is intended to be used either as:
+;
+; # Use the xNVMe io-engine engine io_uring_cmd async. impl.
+; fio examples/xnvme-fdp.fio \
+;   --section=default \
+;   --ioengine=xnvme \
+;   --xnvme_async=io_uring_cmd \
+;   --filename=/dev/ng0n1
+;
+; # Use the xNVMe io-engine engine with nvme sync. impl.
+; fio examples/xnvme-fdp.fio \
+;   --section=default \
+;   --ioengine=xnvme \
+;   --xnvme_sync=nvme \
+;   --filename=/dev/ng0n1
+;
+; FIO_BS="512" FIO_RW="read" FIO_IODEPTH=16 fio examples/xnvme-fdp.fio \
+;   --section=override --ioengine=xnvme --xnvme_sync=nvme --filename=/dev/ng0n1
+;
+[global]
+rw=randwrite
+size=2M
+iodepth=1
+bs=4K
+thread=1
+fdp=1
+fdp_pli=4,5
+
+[default]
+
+[override]
+rw=${FIO_RW}
+iodepth=${FIO_IODEPTH}
+bs=${FIO_BS}
diff --git a/fio.1 b/fio.1
index 0257513b..86cb2af6 100644
--- a/fio.1
+++ b/fio.1
@@ -2192,10 +2192,10 @@ cached data. Currently the RWF_NOWAIT flag does not supported for cached write.
 For direct I/O, requests will only succeed if cache invalidation isn't required,
 file blocks are fully allocated and the disk request could be issued immediately.
 .TP
-.BI (io_uring_cmd)fdp \fR=\fPbool
+.BI (io_uring_cmd,xnvme)fdp \fR=\fPbool
 Enable Flexible Data Placement mode for write commands.
 .TP
-.BI (io_uring_cmd)fdp_pli_select \fR=\fPstr
+.BI (io_uring_cmd,xnvme)fdp_pli_select \fR=\fPstr
 Defines how fio decides which placement ID to use next. The following types
 are defined:
 .RS
@@ -2211,7 +2211,7 @@ Round robin over available placement IDs. This is the default.
 The available placement ID index/indices is defined by \fBfdp_pli\fR option.
 .RE
 .TP
-.BI (io_uring_cmd)fdp_pli \fR=\fPstr
+.BI (io_uring_cmd,xnvme)fdp_pli \fR=\fPstr
 Select which Placement ID Index/Indicies this job is allowed to use for writes.
 By default, the job will cycle through all available Placement IDs, so use this
 to isolate these identifiers to specific jobs. If you want fio to use placement
-- 
2.25.1

Re: [PATCH 1/3] fdp: use macro for max ruhs and fix placement id check

[Vincent Fu]

On 7/11/23 08:54, Ankit Kumar wrote:
> Use macro for max ruhs.
> Number of reclaim unit handle descriptors are 1 based, whereas the
> input placement id index / indices are 0 based. Add the correct check
> for that.
> 
> Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> ---
>   engines/io_uring.c | 2 +-
>   fdp.c              | 8 ++++----
>   fdp.h              | 2 ++
>   3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 5021239e..407d65ce 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -1310,7 +1310,7 @@ static int fio_ioring_cmd_fetch_ruhs(struct thread_data *td, struct fio_file *f,
>   	struct nvme_fdp_ruh_status *ruhs;
>   	int bytes, ret, i;
>   
> -	bytes = sizeof(*ruhs) + 128 * sizeof(struct nvme_fdp_ruh_status_desc);
> +	bytes = sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(struct nvme_fdp_ruh_status_desc);
>   	ruhs = scalloc(1, bytes);
>   	if (!ruhs)
>   		return -ENOMEM;
> diff --git a/fdp.c b/fdp.c
> index d92dbc67..72afca01 100644
> --- a/fdp.c
> +++ b/fdp.c
> @@ -45,7 +45,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
>   	struct fio_ruhs_info *ruhs, *tmp;
>   	int i, ret;
>   
> -	ruhs = scalloc(1, sizeof(*ruhs) + 128 * sizeof(*ruhs->plis));
> +	ruhs = scalloc(1, sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(*ruhs->plis));
>   	if (!ruhs)
>   		return -ENOMEM;
>   
> @@ -56,8 +56,8 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
>   		goto out;
>   	}
>   
> -	if (ruhs->nr_ruhs > 128)
> -		ruhs->nr_ruhs = 128;
> +	if (ruhs->nr_ruhs > FDP_MAX_RUHS)
> +		ruhs->nr_ruhs = FDP_MAX_RUHS;
>   
>   	if (td->o.fdp_nrpli == 0) {
>   		f->ruhs_info = ruhs;
> @@ -65,7 +65,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
>   	}
>   
>   	for (i = 0; i < td->o.fdp_nrpli; i++) {
> -		if (td->o.fdp_plis[i] > ruhs->nr_ruhs) {
> +		if (td->o.fdp_plis[i] >= ruhs->nr_ruhs) {
>   			ret = -EINVAL;
>   			goto out;
>   		}
> diff --git a/fdp.h b/fdp.h
> index 81691f62..4a5e0e54 100644
> --- a/fdp.h
> +++ b/fdp.h
> @@ -3,6 +3,8 @@
>   
>   #include "io_u.h"
>   
> +#define FDP_MAX_RUHS	128
> +
>   struct fio_ruhs_info {
>   	uint32_t nr_ruhs;
>   	uint32_t pli_loc;


Ankit, the commit history will be cleaner if the refactoring change and 
the bug fix were split into separate commits. Would you mind doing that?

Vincent

Re: [PATCH 1/3] fdp: use macro for max ruhs and fix placement id check

[Ankit Kumar]

Yeah makes sense, I will do that in v2.

On Tue, Jul 11, 2023 at 8:16 PM Vincent Fu <vincentfu@gmail.com> wrote:
>
> On 7/11/23 08:54, Ankit Kumar wrote:
> > Use macro for max ruhs.
> > Number of reclaim unit handle descriptors are 1 based, whereas the
> > input placement id index / indices are 0 based. Add the correct check
> > for that.
> >
> > Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> > ---
> >   engines/io_uring.c | 2 +-
> >   fdp.c              | 8 ++++----
> >   fdp.h              | 2 ++
> >   3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/engines/io_uring.c b/engines/io_uring.c
> > index 5021239e..407d65ce 100644
> > --- a/engines/io_uring.c
> > +++ b/engines/io_uring.c
> > @@ -1310,7 +1310,7 @@ static int fio_ioring_cmd_fetch_ruhs(struct thread_data *td, struct fio_file *f,
> >       struct nvme_fdp_ruh_status *ruhs;
> >       int bytes, ret, i;
> >
> > -     bytes = sizeof(*ruhs) + 128 * sizeof(struct nvme_fdp_ruh_status_desc);
> > +     bytes = sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(struct nvme_fdp_ruh_status_desc);
> >       ruhs = scalloc(1, bytes);
> >       if (!ruhs)
> >               return -ENOMEM;
> > diff --git a/fdp.c b/fdp.c
> > index d92dbc67..72afca01 100644
> > --- a/fdp.c
> > +++ b/fdp.c
> > @@ -45,7 +45,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
> >       struct fio_ruhs_info *ruhs, *tmp;
> >       int i, ret;
> >
> > -     ruhs = scalloc(1, sizeof(*ruhs) + 128 * sizeof(*ruhs->plis));
> > +     ruhs = scalloc(1, sizeof(*ruhs) + FDP_MAX_RUHS * sizeof(*ruhs->plis));
> >       if (!ruhs)
> >               return -ENOMEM;
> >
> > @@ -56,8 +56,8 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
> >               goto out;
> >       }
> >
> > -     if (ruhs->nr_ruhs > 128)
> > -             ruhs->nr_ruhs = 128;
> > +     if (ruhs->nr_ruhs > FDP_MAX_RUHS)
> > +             ruhs->nr_ruhs = FDP_MAX_RUHS;
> >
> >       if (td->o.fdp_nrpli == 0) {
> >               f->ruhs_info = ruhs;
> > @@ -65,7 +65,7 @@ static int init_ruh_info(struct thread_data *td, struct fio_file *f)
> >       }
> >
> >       for (i = 0; i < td->o.fdp_nrpli; i++) {
> > -             if (td->o.fdp_plis[i] > ruhs->nr_ruhs) {
> > +             if (td->o.fdp_plis[i] >= ruhs->nr_ruhs) {
> >                       ret = -EINVAL;
> >                       goto out;
> >               }
> > diff --git a/fdp.h b/fdp.h
> > index 81691f62..4a5e0e54 100644
> > --- a/fdp.h
> > +++ b/fdp.h
> > @@ -3,6 +3,8 @@
> >
> >   #include "io_u.h"
> >
> > +#define FDP_MAX_RUHS 128
> > +
> >   struct fio_ruhs_info {
> >       uint32_t nr_ruhs;
> >       uint32_t pli_loc;
>
>
> Ankit, the commit history will be cleaner if the refactoring change and
> the bug fix were split into separate commits. Would you mind doing that?
>
> Vincent