Re: [PATCH] io_uring/rw: transform single vector readv/writev into ubuf

[Ming Lei <ming.lei@redhat.com>]

On Fri, Mar 24, 2023 at 05:06:00PM -0600, Jens Axboe wrote:
> On 3/24/23 4:41?PM, Ming Lei wrote:
> > On Fri, Mar 24, 2023 at 08:35:38AM -0600, Jens Axboe wrote:
> >> It's very common to have applications that use vectored reads or writes,
> >> even if they only pass in a single segment. Obviously they should be
> >> using read/write at that point, but...
> > 
> > Yeah, it is like fixing application issue in kernel side, :-)
> 
> It totally is, the same thing happens all of the time for readv as well.
> No amount of informing or documenting will ever fix that...
> 
> Also see:
> 
> https://lore.kernel.org/linux-fsdevel/20230324204443.45950-1-axboe@kernel.dk/
> 
> with which I think I'll change this one to just be:
> 
> 	if (iter->iter_type == ITER_UBUF) {
> 		rw->addr = iter->ubuf;
> 		rw->len = iter->count;
> 	/* readv -> read distance is the same as writev -> write */
> 	BUILD_BUG_ON((IORING_OP_READ - IORING_OP_READV) !=
> 			(IORING_OP_WRITE - IORING_OP_WRITEV));
> 		req->opcode += (IORING_OP_READ - IORING_OP_READV);
> 	}
> 
> instead.
> 
> We could also just skip it completely and just have liburing do the
> right thing if io_uring_prep_readv/writev is called with nr_segs == 1.
> Just turn it into a READ/WRITE at that point. If we do that, and with
> the above generic change, it's probably Good Enough. If you use
> READV/WRITEV and you're using the raw interface, then you're on your
> own.
> 
> >> +	rw->addr = (unsigned long) iter->iov[0].iov_base;
> >> +	rw->len = iter->iov[0].iov_len;
> >> +	iov_iter_ubuf(iter, ddir, iter->iov[0].iov_base, rw->len);
> >> +	/* readv -> read distance is the same as writev -> write */
> >> +	BUILD_BUG_ON((IORING_OP_READ - IORING_OP_READV) !=
> >> +			(IORING_OP_WRITE - IORING_OP_WRITEV));
> >> +	req->opcode += (IORING_OP_READ - IORING_OP_READV);
> > 
> > It is a bit fragile to change ->opcode, which may need matched
> > callbacks for the two OPs, also cause inconsistent opcode in traces.
> > 
> > I am wondering why not play the magic in io_prep_rw() from beginning?
> 
> It has to be done when importing the vec, we cannot really do it in
> prep... Well we could, but that'd be adding a bunch more code and
> duplicating part of the vec import.

I meant something like the following(un-tested), which at least
guarantees that op_code, rw->addr/len are finalized since ->prep().

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 0c292ef9a40f..4bf4c3effdac 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -120,6 +120,25 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 			return ret;
 	}
 
+	if (req->opcode == IORING_OP_READV && req->opcode == IORING_OP_WRITEV &&
+			rw->len == 1) {
+		struct iovec iov;
+		struct iovec *iovp;
+
+		iovp = iovec_from_user(u64_to_user_ptr(rw->addr), 1, 1, &iov,
+					req->ctx->compat);
+		if (IS_ERR(iovp))
+			return PTR_ERR(iovp);
+
+		rw->addr = (unsigned long) iovp->iov_base;
+		rw->len = iovp->iov_len;
+
+		/* readv -> read distance is the same as writev -> write */
+		BUILD_BUG_ON((IORING_OP_READ - IORING_OP_READV) !=
+				(IORING_OP_WRITE - IORING_OP_WRITEV));
+		req->opcode += (IORING_OP_READ - IORING_OP_READV);
+	}
+
 	return 0;
 }
 


Thanks,
Ming