Re: [PATCH 2/2] t/io_uring: add support for async-passthru

[Jens Axboe <axboe@kernel.dk>]

On 8/24/22 3:31 AM, Anuj Gupta wrote:
> @@ -457,12 +509,13 @@ static unsigned file_depth(struct submitter *s)
>  
>  static void init_io(struct submitter *s, unsigned index)
>  {
> -	struct io_uring_sqe *sqe = &s->sqes[index];
> +	struct io_uring_sqe *sqe = NULL;
>  	unsigned long offset;
>  	struct file *f;
>  	long r;
>  
>  	if (do_nop) {
> +		sqe = &s->sqes[index];
>  		sqe->opcode = IORING_OP_NOP;
>  		return;

I'd just leave the initial assignment rather than have two manual ones
for the same thing, and then just have the 'pt' path override it.

> @@ -490,6 +543,41 @@ static void init_io(struct submitter *s, unsigned index)
>  			f->cur_off = 0;
>  	}
>  
> +	if (pt) {
> +		struct nvme_uring_cmd *cmd;
> +		unsigned long long slba;
> +		unsigned long long nlb;
> +
> +		sqe = &s->sqes[index<<1];

		sqe = &s->sqes[index << 1];

please.

> +		if (register_files) {
> +			sqe->fd = f->fixed_fd;
> +			sqe->flags = IOSQE_FIXED_FILE;
> +		} else {
> +			sqe->fd = f->real_fd;
> +			sqe->flags = 0;
> +		}
> +		sqe->opcode = IORING_OP_URING_CMD;
> +		sqe->user_data = (unsigned long) f->fileno;
> +		if (stats)
> +			sqe->user_data |= ((unsigned long)s->clock_index << 32);
> +		sqe->cmd_op = NVME_URING_CMD_IO;
> +		slba = offset / lbs;
> +		nlb = bs/lbs - 1;

You have two expensive integer divisions here. Would make sense to turn
lbs into a shift instead, those are a lot cheaper to do. And then you
can put the expensive part in the setup part rather than have it in the
fast path.

Another thing to consider is to make the pt path separate rather than
need a bunch of 'if (pt) ... else ...' in here?

> @@ -587,11 +690,14 @@ static int reap_events_uring(struct submitter *s)
>  
>  			f = &s->files[fileno];
>  			f->pending_ios--;
> -			if (cqe->res != bs) {
> +			if (!pt && cqe->res != bs) {
>  				printf("io: unexpected ret=%d\n", cqe->res);
>  				if (polled && cqe->res == -EOPNOTSUPP)
>  					printf("Your filesystem/driver/kernel doesn't support polled IO\n");
>  				return -1;
> +			} else if (pt && cqe->res != 0) {
> +				printf("io: unexpected ret=%d\n", cqe->res);
> +				return -1;
>  			}
>  		}
>  		if (stats) {

These two would be examples of things that would be cleaner with a
separate path too.

> @@ -1306,11 +1428,12 @@ static void usage(char *argv, int status)
>  		" -a <bool> : Use legacy aio, default %d\n"
>  		" -S <bool> : Use sync IO (preadv2), default %d\n"
>  		" -X <bool> : Use registered ring %d\n"
> -		" -P <bool> : Automatically place on device home node %d\n",
> +		" -P <bool> : Automatically place on device home node %d\n"
> +		" -u <bool> : Use nvme-passthrough I/O, default %d\n",
>  		argv, DEPTH, BATCH_SUBMIT, BATCH_COMPLETE, BS, polled,
>  		fixedbufs, dma_map, register_files, nthreads, !buffered, do_nop,
>  		stats, runtime == 0 ? "unlimited" : runtime_str, random_io, aio,
> -		use_sync, register_ring, numa_placement);
> +		use_sync, register_ring, numa_placement, pt);
>  	exit(status);

Reminds me, should the nsid and lbs be in the submitter struct?

Thanks for doing this work!!

-- 
Jens Axboe