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

[Pavel Begunkov <asml.silence@gmail.com>]

On 3/25/23 00:24, Ming Lei wrote:
> 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.

I like this option but sendmsg and recvmsg probably do need the same
fix up, which is more justified as they can't get converted to
send/recv as this.

Another option is to internally detangle opcodes from iter types.

import() {
     if (req->op == READV)
         import_iovec();
     else
         import_buf();
}

would get replaced with:

prep() {
     if (req->op == READV)
         req->flags = REQ_F_IOVEC;
}

import() {
     if (req->flags & REQ_F_IOVEC)
         import_iovec();
     else
         import_buf();
}


>>>> +	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().

It sounds like a better approach. With opcode machinations it's easy
to forget about some kind of state that could be fatal.

Take IOSQE_ASYNC for example. The core code will allocate
async_data and do io_readv_prep_async() -> import_iovec(), which
inside changes the opcode. It'll be a problem if io_readv_prep_async()
forgets that it might a different opcode with a slightly different req
layout, or even non-vectored read would do sth weird with ->async_data
or mishandle REQ_F_NEED_CLEANUP.

fwiw, needs compat handling, i.e. leave as iovec if compat

> 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;
>   }

-- 
Pavel Begunkov