On 2/9/23 07:20, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> NVMe TP4146 creates a new feature called Flexible Data Placement. This
> feature allows a host to tell the device how to group write data through
> the use of "Placement Identifiers" in write commands.
>
> Add support for using placement identifiers in write commands. The user
> can enabled this with the new "fdp=1" parameter for fio's io_uring_cmd
> ioengine. By default, the fio jobs will cycle through all the namespace's
> available placement identifiers for write commands. The user can limit
> which placement identifiers can be used with additional parameter,
> "fdp_plis=<list,>", which can be used to separate write intensive jobs
> from less intenstive ones.
>
> Setting up your namespace for FDP is outside the scope of 'fio', so this
> assumes the namespace is already properly configured for the mode.
>
> Based-on-a-patch-by: Ankit Kumar <ankit.kumar@xxxxxxxxxxx>
> Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
[...]
> +static int fio_ioring_cmd_fetch_ruhs(struct thread_data *td, struct fio_file *f,
> + struct fio_ruhs_info *fruhs_info)
> +{
> + struct nvme_fdp_ruh_status *ruhs;
> + int bytes, ret, i;
> +
> + bytes = sizeof(*ruhs) + 1024 * sizeof(struct nvme_fdp_ruh_status_desc);
> + ruhs = malloc(bytes);
if malloc() fails here (rare case), I expect that fio_nvme_iomgmt_ruhs()
will fail but the above memset() will crash the process. So maybe add a
"if (ruhs)" before it to gracefully fail ?
> + memset(ruhs, 0, bytes);
> +
> + ret = fio_nvme_iomgmt_ruhs(td, f, ruhs, bytes);
> + if (ret)
> + goto free;
> +
> + fruhs_info->nr_ruhs = le16_to_cpu(ruhs->nruhsd);
> + for (i = 0; i < fruhs_info->nr_ruhs; i++)
> + fruhs_info->plis[i] = le16_to_cpu(ruhs->ruhss[i].pid);
> +free:
> + free(ruhs);
> + return ret;
> +}
[...]
> diff --git a/file.h b/file.h
> index da1b8947..deb36e02 100644
> --- a/file.h
> +++ b/file.h
> @@ -12,6 +12,7 @@
>
> /* Forward declarations */
> struct zoned_block_device_info;
> +struct fdp_ruh_info;
>
> /*
> * The type of object we are working on
> @@ -101,6 +102,8 @@ struct fio_file {
> uint64_t file_offset;
> uint64_t io_size;
>
> + struct fio_ruhs_info *ruhs_info;
white line before declaration can be removed I think.
[...]
> diff --git a/io_u.h b/io_u.h
> index 206e24fe..13b26d37 100644
> --- a/io_u.h
> +++ b/io_u.h
> @@ -117,6 +117,9 @@ struct io_u {
> */
> int (*end_io)(struct thread_data *, struct io_u **);
>
> + uint32_t dtype;
> + uint32_t dspec;
Nit: may be call these fdp_dtype and fdp_dspec to clarify these are for fdp ?
> diff --git a/ioengines.h b/ioengines.h
> index 2cb9743e..4a9284c0 100644
> --- a/ioengines.h
> +++ b/ioengines.h
> @@ -7,6 +7,7 @@
> #include "flist.h"
> #include "io_u.h"
> #include "zbd_types.h"
> +#include "fdp.h"
>
> #define FIO_IOOPS_VERSION 31
Shouldn't this be increased given that you are changing the ioengine_ops
structure ?
> @@ -63,6 +64,9 @@ struct ioengine_ops {
> unsigned int *);
> int (*finish_zone)(struct thread_data *, struct fio_file *,
> uint64_t, uint64_t);
> + int (*fdp_support)(struct thread_data *, struct fio_file *, bool *);
> + int (*fetch_ruhs)(struct thread_data *, struct fio_file *,
> + struct fio_ruhs_info *);
> int option_struct_size;
> struct fio_option *options;
> };
[...]
> diff --git a/thread_options.h b/thread_options.h
> index 74e7ea45..34eb4d3f 100644
> --- a/thread_options.h
> +++ b/thread_options.h
> @@ -386,6 +386,12 @@ struct thread_options {
> fio_fp64_t zrt;
> fio_fp64_t zrf;
>
> + unsigned int fdp;
> +
> +#define FIO_MAX_PLIS 16
> + unsigned int plis[FIO_MAX_PLIS];
> + unsigned int nrpli;
> +
Nit: FDP_/fdp_ prefix for these too to clarify the usage ?
> unsigned int log_entries;
> unsigned int log_prio;
> };
> @@ -698,6 +704,8 @@ struct thread_options_pack {
> uint32_t log_entries;
> uint32_t log_prio;
>
> + uint32_t fdp;
> +
> /*
> * verify_pattern followed by buffer_pattern from the unpacked struct
> */
--
Damien Le Moal
Western Digital Research
