Re: [PATCH 7/8] io_uring: support readv/writev with fixed buffers

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

On 12/11/2020 23:00, Bijan Mottahedeh wrote:
> Support readv/writev with fixed buffers, and introduce IOSQE_FIXED_BUFFER,
> consistent with fixed files.

I don't like it at all, see issues below. The actual implementation would
be much uglier.

I propose you to split the series and push separately. Your first 6 patches
first, I don't have conceptual objections to them. Then registration sharing
(I still need to look it up). And then we can return to this, if you're not
yet convinced.

> 
> Signed-off-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
> ---
>  fs/io_uring.c                 | 59 ++++++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/io_uring.h |  3 +++
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6020fd2..12c4144 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -625,6 +625,7 @@ enum {
>  	REQ_F_HARDLINK_BIT	= IOSQE_IO_HARDLINK_BIT,
>  	REQ_F_FORCE_ASYNC_BIT	= IOSQE_ASYNC_BIT,
>  	REQ_F_BUFFER_SELECT_BIT	= IOSQE_BUFFER_SELECT_BIT,
> +	REQ_F_FIXED_BUFFER_BIT	= IOSQE_FIXED_BUFFER_BIT,
>  
>  	REQ_F_FAIL_LINK_BIT,
>  	REQ_F_INFLIGHT_BIT,
> @@ -681,8 +682,12 @@ enum {
>  	REQ_F_WORK_INITIALIZED	= BIT(REQ_F_WORK_INITIALIZED_BIT),
>  	/* linked timeout is active, i.e. prepared by link's head */
>  	REQ_F_LTIMEOUT_ACTIVE	= BIT(REQ_F_LTIMEOUT_ACTIVE_BIT),
> +	/* ctx owns buffer */
> +	REQ_F_FIXED_BUFFER	= BIT(REQ_F_FIXED_BUFFER_BIT),
>  };
>  
> +#define REQ_F_FIXED_RSRC	(REQ_F_FIXED_FILE | REQ_F_FIXED_BUFFER)
> +
>  struct async_poll {
>  	struct io_poll_iocb	poll;
>  	struct io_poll_iocb	*double_poll;
> @@ -3191,6 +3196,46 @@ static ssize_t io_iov_buffer_select(struct io_kiocb *req, struct iovec *iov,
>  	return __io_iov_buffer_select(req, iov, needs_lock);
>  }
>  
> +static ssize_t io_import_iovec_fixed(int rw, struct io_kiocb *req, void *buf,
> +				     unsigned segs, unsigned fast_segs,
> +				     struct iovec **iovec,
> +				     struct iov_iter *iter)
> +{
> +	struct io_ring_ctx *ctx = req->ctx;
> +	struct io_mapped_ubuf *imu;
> +	struct iovec *iov;
> +	u16 index, buf_index;
> +	ssize_t ret;
> +	unsigned long seg;
> +
> +	if (unlikely(!ctx->buf_data))
> +		return -EFAULT;
> +
> +	ret = import_iovec(rw, buf, segs, fast_segs, iovec, iter);

Did you test it? import_iovec() does access_ok() against each iov_base,
which in your case are an index.

> +	if (ret < 0)
> +		return ret;
> +
> +	iov = (struct iovec *)iter->iov;
> +
> +	for (seg = 0; seg < iter->nr_segs; seg++) {
> +		buf_index = *(u16 *)(&iov[seg].iov_base);

That's ugly, and also not consistent with rw_fixed, because iov_base is
used to calculate offset.

> +		if (unlikely(buf_index < 0 || buf_index >= ctx->nr_user_bufs))
> +			return -EFAULT;
> +
> +		index = array_index_nospec(buf_index, ctx->nr_user_bufs);
> +		imu = io_buf_from_index(ctx, index);
> +		if (!imu->ubuf || !imu->len)
> +			return -EFAULT;
> +		if (iov[seg].iov_len > imu->len)
> +			return -EFAULT;
> +
> +		iov[seg].iov_base = (void *)imu->ubuf;

Nope, that's not different from non registered version.
What import_fixed actually do is setting up the iter argument to point
to a bvec (a vector of struct page *).

So it either would need to keep a vector of bvecs, that's a vector of vectors,
that's not supported by iter, etc., so you'll also need to iterate over them
in io_read/write and so on. Or flat 2D structure into 1D, but that's still ugly.



> +		ret += iov[seg].iov_len;
> +	}
> +
> +	return ret;
> +}
> +
>  static ssize_t io_import_iovec(int rw, struct io_kiocb *req,
>  				 struct iovec **iovec, struct iov_iter *iter,
>  				 bool needs_lock)
> @@ -3201,6 +3246,12 @@ static ssize_t io_import_iovec(int rw, struct io_kiocb *req,
>  	u8 opcode;
>  
>  	opcode = req->opcode;
> +
> +	if ((opcode == IORING_OP_READV || opcode == IORING_OP_WRITEV) &&
> +	    req->flags & REQ_F_FIXED_BUFFER)
> +		return (io_import_iovec_fixed(rw, req, buf, sqe_len,
> +					      UIO_FASTIOV, iovec, iter));
> +
>  	if (opcode == IORING_OP_READ_FIXED || opcode == IORING_OP_WRITE_FIXED) {
>  		*iovec = NULL;
>  		return io_import_fixed(req, rw, iter);
> @@ -5692,7 +5743,7 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
>  {
>  	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
>  		return -EINVAL;
> -	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
> +	if (unlikely(req->flags & (REQ_F_FIXED_RSRC | REQ_F_BUFFER_SELECT)))

Why it's here? 

#define REQ_F_FIXED_RSRC	(REQ_F_FIXED_FILE | REQ_F_FIXED_BUFFER)
So, why do you | with REQ_F_BUFFER_SELECT again here?


>  		return -EINVAL;
>  	if (sqe->ioprio || sqe->buf_index || sqe->len || sqe->timeout_flags)
>  		return -EINVAL;
> @@ -5867,7 +5918,7 @@ static int io_async_cancel_prep(struct io_kiocb *req,
>  {
>  	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
>  		return -EINVAL;
> -	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
> +	if (unlikely(req->flags & (REQ_F_FIXED_RSRC | REQ_F_BUFFER_SELECT)))
>  		return -EINVAL;
>  	if (sqe->ioprio || sqe->off || sqe->len || sqe->cancel_flags)
>  		return -EINVAL;
> @@ -5889,7 +5940,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req,
>  {
>  	if (unlikely(req->ctx->flags & IORING_SETUP_SQPOLL))
>  		return -EINVAL;
> -	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
> +	if (unlikely(req->flags & (REQ_F_FIXED_RSRC | REQ_F_BUFFER_SELECT)))
>  		return -EINVAL;
>  	if (sqe->ioprio || sqe->rw_flags)
>  		return -EINVAL;
> @@ -6740,7 +6791,7 @@ static inline bool io_check_restriction(struct io_ring_ctx *ctx,
>  
>  #define SQE_VALID_FLAGS	(IOSQE_FIXED_FILE|IOSQE_IO_DRAIN|IOSQE_IO_LINK|	\
>  				IOSQE_IO_HARDLINK | IOSQE_ASYNC | \
> -				IOSQE_BUFFER_SELECT)
> +				IOSQE_BUFFER_SELECT | IOSQE_FIXED_BUFFER)
>  
>  static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>  		       const struct io_uring_sqe *sqe,
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 17682b5..41da59c 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -70,6 +70,7 @@ enum {
>  	IOSQE_IO_HARDLINK_BIT,
>  	IOSQE_ASYNC_BIT,
>  	IOSQE_BUFFER_SELECT_BIT,
> +	IOSQE_FIXED_BUFFER_BIT,
>  };
>  
>  /*
> @@ -87,6 +88,8 @@ enum {
>  #define IOSQE_ASYNC		(1U << IOSQE_ASYNC_BIT)
>  /* select buffer from sqe->buf_group */
>  #define IOSQE_BUFFER_SELECT	(1U << IOSQE_BUFFER_SELECT_BIT)
> +/* use fixed buffer set */
> +#define IOSQE_FIXED_BUFFER	(1U << IOSQE_FIXED_BUFFER_BIT)

Unfortenatuly, we're almost out of flags bits -- it's a 1 byte
field and 6 bits are already taken. Let's not use it.

-- 
Pavel Begunkov