Re: [PATCH 1/2] engines/io_uring: support r/w with metadata

Jens Axboe
2025-07-23T17:23:35Z

On 7/23/25 11:04 AM, Vincent Fu wrote:
> @@ -436,6 +502,27 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
> sqe->len = 1;
> }
> }
> + if (o->md_per_io_size) {
> + struct io_uring_attr_pi *pi_attr = io_u->pi_attr;
> + struct nvme_data *data = FILE_ENG_DATA(io_u->file);
> +
> + sqe->attr_type_mask = IORING_RW_ATTR_FLAG_PI;
> + sqe->attr_ptr = (__u64)(uintptr_t)pi_attr;
> + pi_attr->addr = (__u64)(uintptr_t)io_u->mmap_data;
> + pi_attr->len = o->md_per_io_size;
> + pi_attr->app_tag = o->apptag;
> + pi_attr->flags = 0;
> + if (strstr(o->pi_chk, "GUARD") != NULL)
> + pi_attr->flags |= IO_INTEGRITY_CHK_GUARD;
> + if (strstr(o->pi_chk, "REFTAG") != NULL) {
> + __u64 slba = get_slba(data, io_u->offset);
> +
> + pi_attr->flags |= IO_INTEGRITY_CHK_REFTAG;
> + pi_attr->seed = (__u32)slba;
> + }
> + if (strstr(o->pi_chk, "APPTAG") != NULL)
> + pi_attr->flags |= IO_INTEGRITY_CHK_APPTAG;
> + }

I'd move these out-of-line whenever possible. It both improves
readability, but also conveys that this isn't necessarily the normal/hot
path.

> @@ -590,6 +678,19 @@ static struct io_u *fio_ioring_event(struct thread_data *td, int event)
> io_u->error = -cqe->res;
> else
> io_u->resid = io_u->xfer_buflen - cqe->res;
> + return io_u;
> + }
> +
> + if (o->md_per_io_size) {
> + struct nvme_data *data;
> + int ret;
> +
> + data = FILE_ENG_DATA(io_u->file);
> + if (data->pi_type && (io_u->ddir == DDIR_READ) && !o->pi_act) {
> + ret = fio_nvme_pi_verify(data, io_u);
> + if (ret)
> + io_u->error = ret;
> + }
> }

Ditto

> + if (!strcmp(td->io_ops->name, "io_uring") && o->md_per_io_size) {
> + struct nvme_data *data = FILE_ENG_DATA(io_u->file);
> + struct nvme_cmd_ext_io_opts ext_opts = {0};
> +
> + if (data->pi_type) {
> + if (o->pi_act)
> + ext_opts.io_flags |= NVME_IO_PRINFO_PRACT;
> +
> + ext_opts.io_flags |= o->prchk;
> + ext_opts.apptag = o->apptag;
> + ext_opts.apptag_mask = o->apptag_mask;
> + }
> + fio_nvme_generate_guard(io_u, &ext_opts);
> + }

Ehh a strcmp() in the hot path?! First of all, that's a big no-no.
Secondly, if this really was required, you'd add something to put that
strcmp() in the slow path and flag it. Lastly, thankfully this should be
much better as:

if (td->io_ops == &ioengine_uring ...)

instead.

> @@ -1347,12 +1464,20 @@ static int fio_ioring_init(struct thread_data *td)
> /* io_u index */
> ld->io_u_index = calloc(td->o.iodepth, sizeof(struct io_u *));
>
> + if (!strcmp(td->io_ops->name, "io_uring") && o->md_per_io_size) {
> + if (o->apptag_mask != 0xffff) {
> + log_err("fio: io_uring with metadata requires an apptag_mask of 0xffff\n");
> + return 1;
> + }
> + }

Same, though this is at least slow path. But people copy-paste wonky
code like that all the time...

> * metadata buffer for nvme command.
> * We are only supporting iomem=malloc / mem=malloc as of now.
> */
> - if (!strcmp(td->io_ops->name, "io_uring_cmd") &&
> - (o->cmd_type == FIO_URING_CMD_NVME) && o->md_per_io_size) {
> + if ((!strcmp(td->io_ops->name, "io_uring_cmd") &&
> + (o->cmd_type == FIO_URING_CMD_NVME) && o->md_per_io_size) ||
> + (!strcmp(td->io_ops->name, "io_uring") && o->md_per_io_size)) {
> md_size = (unsigned long long) o->md_per_io_size
> * (unsigned long long) td->o.iodepth;
> md_size += page_mask + td->o.mem_align;

And I'm starting to see where this is coming from. If you are so
inclined, please do a cleanup patch first getting rid of these strcmp()
calls.

> static int fio_ioring_open_file(struct thread_data *td, struct fio_file *f)
> {
> struct ioring_data *ld = td->io_ops_data;
> struct ioring_options *o = td->eo;
>
> + if (o->md_per_io_size) {
> + struct nvme_data *data = NULL;
> + __u64 nlba = 0;
> + int ret;
> +
> + data = FILE_ENG_DATA(f);
> + if (data == NULL) {
> + data = calloc(1, sizeof(struct nvme_data));
> + ret = fio_get_pi_info(f, &nlba, o->pi_act, data);
> + if (ret) {
> + free(data);
> + return ret;
> + }
> +
> + FILE_SET_ENG_DATA(f, data);
> + }
> + }

Out of line please...

> diff --git a/engines/nvme.c b/engines/nvme.c
> index 37a31e2f..4b3d3860 100644
> --- a/engines/nvme.c
> +++ b/engines/nvme.c
> @@ -8,22 +8,6 @@
> #include "../crc/crc-t10dif.h"
> #include "../crc/crc64.h"
>
> -static inline __u64 get_slba(struct nvme_data *data, __u64 offset)
> -{
> - if (data->lba_ext)
> - return offset / data->lba_ext;
> -
> - return offset >> data->lba_shift;
> -}
> -
> -static inline __u32 get_nlb(struct nvme_data *data, __u64 len)
> -{
> - if (data->lba_ext)
> - return len / data->lba_ext - 1;
> -
> - return (len >> data->lba_shift) - 1;
> -}
> -

Various bits like this, please do a prep patch first moving things
around.

--
Jens Axboe