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

[Jens Axboe <axboe@kernel.dk>]

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