[PATCH 0/4] clean up rw buffer import

[PATCH 0/4] clean up rw buffer import

[Pavel Begunkov]

Do some minor brushing for read/write prep. It might need more work,
but should be a cleaner base for changes around how we import buffers.

Pavel Begunkov (4):
  io_uring/rw: allocate async data in io_prep_rw()
  io_uring/rw: rename io_import_iovec()
  io_uring/rw: extract helper for iovec import
  io_uring/rw: open code io_prep_rw_setup()

 io_uring/rw.c | 94 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 48 deletions(-)

-- 
2.48.1

[PATCH 1/4] io_uring/rw: allocate async data in io_prep_rw()

[Pavel Begunkov]

io_prep_rw() relies on async_data being allocated in io_prep_rw_setup().
Be a bit more explicit and move the allocation earlier into io_prep_rw()
and don't hide it in a call chain.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 22612a956e75..7efc2337c5a0 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -203,9 +203,6 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import)
 {
 	struct io_async_rw *rw;
 
-	if (io_rw_alloc_async(req))
-		return -ENOMEM;
-
 	if (!do_import || io_do_buffer_select(req))
 		return 0;
 
@@ -262,6 +259,9 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	u64 attr_type_mask;
 	int ret;
 
+	if (io_rw_alloc_async(req))
+		return -ENOMEM;
+
 	rw->kiocb.ki_pos = READ_ONCE(sqe->off);
 	/* used for fixed read/write too - just read unconditionally */
 	req->buf_index = READ_ONCE(sqe->buf_index);
-- 
2.48.1

[PATCH 2/4] io_uring/rw: rename io_import_iovec()

[Pavel Begunkov]

io_import_iovec() is not limited to iovecs but also imports buffers for
normal reads and selected buffers, rename it for clarity.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rw.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 7efc2337c5a0..e636be4850a7 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -76,7 +76,7 @@ static int io_iov_buffer_select_prep(struct io_kiocb *req)
 	return 0;
 }
 
-static int __io_import_iovec(int ddir, struct io_kiocb *req,
+static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
 			     struct io_async_rw *io,
 			     unsigned int issue_flags)
 {
@@ -122,13 +122,13 @@ static int __io_import_iovec(int ddir, struct io_kiocb *req,
 	return 0;
 }
 
-static inline int io_import_iovec(int rw, struct io_kiocb *req,
-				  struct io_async_rw *io,
-				  unsigned int issue_flags)
+static inline int io_import_rw_buffer(int rw, struct io_kiocb *req,
+				      struct io_async_rw *io,
+				      unsigned int issue_flags)
 {
 	int ret;
 
-	ret = __io_import_iovec(rw, req, io, issue_flags);
+	ret = __io_import_rw_buffer(rw, req, io, issue_flags);
 	if (unlikely(ret < 0))
 		return ret;
 
@@ -207,7 +207,7 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import)
 		return 0;
 
 	rw = req->async_data;
-	return io_import_iovec(ddir, req, rw, 0);
+	return io_import_rw_buffer(ddir, req, rw, 0);
 }
 
 static inline void io_meta_save_state(struct io_async_rw *io)
@@ -845,7 +845,7 @@ static int __io_read(struct io_kiocb *req, unsigned int issue_flags)
 	loff_t *ppos;
 
 	if (io_do_buffer_select(req)) {
-		ret = io_import_iovec(ITER_DEST, req, io, issue_flags);
+		ret = io_import_rw_buffer(ITER_DEST, req, io, issue_flags);
 		if (unlikely(ret < 0))
 			return ret;
 	}
-- 
2.48.1

[PATCH 3/4] io_uring/rw: extract helper for iovec import

[Pavel Begunkov]

Split out a helper out of __io_import_rw_buffer() that handles vectored
buffers. I'll need it for registered vectored buffers, but it also looks
cleaner, especially with parameters being properly named.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rw.c | 57 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index e636be4850a7..0e0d2a19f21d 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -76,41 +76,23 @@ static int io_iov_buffer_select_prep(struct io_kiocb *req)
 	return 0;
 }
 
-static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
-			     struct io_async_rw *io,
-			     unsigned int issue_flags)
+static int io_import_vec(int ddir, struct io_kiocb *req,
+			 struct io_async_rw *io, void __user *uvec,
+			 size_t uvec_segs)
 {
-	const struct io_issue_def *def = &io_issue_defs[req->opcode];
-	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
+	int ret, nr_segs;
 	struct iovec *iov;
-	void __user *buf;
-	int nr_segs, ret;
-	size_t sqe_len;
-
-	buf = u64_to_user_ptr(rw->addr);
-	sqe_len = rw->len;
-
-	if (!def->vectored || req->flags & REQ_F_BUFFER_SELECT) {
-		if (io_do_buffer_select(req)) {
-			buf = io_buffer_select(req, &sqe_len, issue_flags);
-			if (!buf)
-				return -ENOBUFS;
-			rw->addr = (unsigned long) buf;
-			rw->len = sqe_len;
-		}
-
-		return import_ubuf(ddir, buf, sqe_len, &io->iter);
-	}
 
 	if (io->free_iovec) {
 		nr_segs = io->free_iov_nr;
 		iov = io->free_iovec;
 	} else {
-		iov = &io->fast_iov;
 		nr_segs = 1;
+		iov = &io->fast_iov;
 	}
-	ret = __import_iovec(ddir, buf, sqe_len, nr_segs, &iov, &io->iter,
-				io_is_compat(req->ctx));
+
+	ret = __import_iovec(ddir, uvec, uvec_segs, nr_segs, &iov, &io->iter,
+			     io_is_compat(req->ctx));
 	if (unlikely(ret < 0))
 		return ret;
 	if (iov) {
@@ -122,6 +104,29 @@ static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
 	return 0;
 }
 
+static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
+			     struct io_async_rw *io,
+			     unsigned int issue_flags)
+{
+	const struct io_issue_def *def = &io_issue_defs[req->opcode];
+	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
+	void __user *buf = u64_to_user_ptr(rw->addr);
+	size_t sqe_len = rw->len;
+
+	if (!def->vectored || req->flags & REQ_F_BUFFER_SELECT) {
+		if (io_do_buffer_select(req)) {
+			buf = io_buffer_select(req, &sqe_len, issue_flags);
+			if (!buf)
+				return -ENOBUFS;
+			rw->addr = (unsigned long) buf;
+			rw->len = sqe_len;
+		}
+		return import_ubuf(ddir, buf, sqe_len, &io->iter);
+	}
+
+	return io_import_vec(ddir, req, io, buf, sqe_len);
+}
+
 static inline int io_import_rw_buffer(int rw, struct io_kiocb *req,
 				      struct io_async_rw *io,
 				      unsigned int issue_flags)
-- 
2.48.1

[PATCH 4/4] io_uring/rw: open code io_prep_rw_setup()

[Pavel Begunkov]

Open code io_prep_rw_setup() into its only caller, it doesn't provide
any meaningful abstraction anymore.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rw.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 0e0d2a19f21d..2e35e7e68ff8 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -204,17 +204,6 @@ static int io_rw_alloc_async(struct io_kiocb *req)
 	return 0;
 }
 
-static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import)
-{
-	struct io_async_rw *rw;
-
-	if (!do_import || io_do_buffer_select(req))
-		return 0;
-
-	rw = req->async_data;
-	return io_import_rw_buffer(ddir, req, rw, 0);
-}
-
 static inline void io_meta_save_state(struct io_async_rw *io)
 {
 	io->meta_state.seed = io->meta.seed;
@@ -287,10 +276,14 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	rw->addr = READ_ONCE(sqe->addr);
 	rw->len = READ_ONCE(sqe->len);
 	rw->flags = READ_ONCE(sqe->rw_flags);
-	ret = io_prep_rw_setup(req, ddir, do_import);
 
-	if (unlikely(ret))
-		return ret;
+	if (do_import && !io_do_buffer_select(req)) {
+		struct io_async_rw *io = req->async_data;
+
+		ret = io_import_rw_buffer(ddir, req, io, 0);
+		if (unlikely(ret))
+			return ret;
+	}
 
 	attr_type_mask = READ_ONCE(sqe->attr_type_mask);
 	if (attr_type_mask) {
-- 
2.48.1

Re: [PATCH 3/4] io_uring/rw: extract helper for iovec import

[Caleb Sander Mateos]

On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> Split out a helper out of __io_import_rw_buffer() that handles vectored
> buffers. I'll need it for registered vectored buffers, but it also looks
> cleaner, especially with parameters being properly named.
>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/rw.c | 57 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index e636be4850a7..0e0d2a19f21d 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -76,41 +76,23 @@ static int io_iov_buffer_select_prep(struct io_kiocb *req)
>         return 0;
>  }
>
> -static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
> -                            struct io_async_rw *io,
> -                            unsigned int issue_flags)
> +static int io_import_vec(int ddir, struct io_kiocb *req,
> +                        struct io_async_rw *io, void __user *uvec,
> +                        size_t uvec_segs)

Could use a more specific type for uvec:  const struct iovec __user *uvec?

>  {
> -       const struct io_issue_def *def = &io_issue_defs[req->opcode];
> -       struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
> +       int ret, nr_segs;
>         struct iovec *iov;
> -       void __user *buf;
> -       int nr_segs, ret;
> -       size_t sqe_len;
> -
> -       buf = u64_to_user_ptr(rw->addr);
> -       sqe_len = rw->len;
> -
> -       if (!def->vectored || req->flags & REQ_F_BUFFER_SELECT) {
> -               if (io_do_buffer_select(req)) {
> -                       buf = io_buffer_select(req, &sqe_len, issue_flags);
> -                       if (!buf)
> -                               return -ENOBUFS;
> -                       rw->addr = (unsigned long) buf;
> -                       rw->len = sqe_len;
> -               }
> -
> -               return import_ubuf(ddir, buf, sqe_len, &io->iter);
> -       }
>
>         if (io->free_iovec) {
>                 nr_segs = io->free_iov_nr;
>                 iov = io->free_iovec;
>         } else {
> -               iov = &io->fast_iov;
>                 nr_segs = 1;
> +               iov = &io->fast_iov;
>         }
> -       ret = __import_iovec(ddir, buf, sqe_len, nr_segs, &iov, &io->iter,
> -                               io_is_compat(req->ctx));
> +
> +       ret = __import_iovec(ddir, uvec, uvec_segs, nr_segs, &iov, &io->iter,
> +                            io_is_compat(req->ctx));
>         if (unlikely(ret < 0))
>                 return ret;
>         if (iov) {
> @@ -122,6 +104,29 @@ static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
>         return 0;
>  }
>
> +static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
> +                            struct io_async_rw *io,
> +                            unsigned int issue_flags)
> +{
> +       const struct io_issue_def *def = &io_issue_defs[req->opcode];
> +       struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
> +       void __user *buf = u64_to_user_ptr(rw->addr);
> +       size_t sqe_len = rw->len;
> +
> +       if (!def->vectored || req->flags & REQ_F_BUFFER_SELECT) {
> +               if (io_do_buffer_select(req)) {
> +                       buf = io_buffer_select(req, &sqe_len, issue_flags);
> +                       if (!buf)
> +                               return -ENOBUFS;
> +                       rw->addr = (unsigned long) buf;
> +                       rw->len = sqe_len;
> +               }
> +               return import_ubuf(ddir, buf, sqe_len, &io->iter);
> +       }
> +
> +       return io_import_vec(ddir, req, io, buf, sqe_len);

nit: an early return for the vectored case could reduce indentation here.

Best,
Caleb

> +}
> +
>  static inline int io_import_rw_buffer(int rw, struct io_kiocb *req,
>                                       struct io_async_rw *io,
>                                       unsigned int issue_flags)
> --
> 2.48.1
>
>

Re: [PATCH 1/4] io_uring/rw: allocate async data in io_prep_rw()

[Caleb Sander Mateos]

On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> io_prep_rw() relies on async_data being allocated in io_prep_rw_setup().
> Be a bit more explicit and move the allocation earlier into io_prep_rw()
> and don't hide it in a call chain.

Hmm, where is async_data currently used in io_prep_rw()? I don't see
any reference to async_data in io_prep_rw() until your patch 4,
"io_uring/rw: open code io_prep_rw_setup()". Would it make sense to
combine the 2 patches?

Best,
Caleb

>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/rw.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 22612a956e75..7efc2337c5a0 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -203,9 +203,6 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import)
>  {
>         struct io_async_rw *rw;
>
> -       if (io_rw_alloc_async(req))
> -               return -ENOMEM;
> -
>         if (!do_import || io_do_buffer_select(req))
>                 return 0;
>
> @@ -262,6 +259,9 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>         u64 attr_type_mask;
>         int ret;
>
> +       if (io_rw_alloc_async(req))
> +               return -ENOMEM;
> +
>         rw->kiocb.ki_pos = READ_ONCE(sqe->off);
>         /* used for fixed read/write too - just read unconditionally */
>         req->buf_index = READ_ONCE(sqe->buf_index);
> --
> 2.48.1
>
>

Re: [PATCH 3/4] io_uring/rw: extract helper for iovec import

[Pavel Begunkov]

On 2/24/25 16:48, Caleb Sander Mateos wrote:
> On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> Split out a helper out of __io_import_rw_buffer() that handles vectored
>> buffers. I'll need it for registered vectored buffers, but it also looks
>> cleaner, especially with parameters being properly named.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   io_uring/rw.c | 57 ++++++++++++++++++++++++++++-----------------------
>>   1 file changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/io_uring/rw.c b/io_uring/rw.c
>> index e636be4850a7..0e0d2a19f21d 100644
>> --- a/io_uring/rw.c
>> +++ b/io_uring/rw.c
>> @@ -76,41 +76,23 @@ static int io_iov_buffer_select_prep(struct io_kiocb *req)
>>          return 0;
>>   }
>>
>> -static int __io_import_rw_buffer(int ddir, struct io_kiocb *req,
>> -                            struct io_async_rw *io,
>> -                            unsigned int issue_flags)
>> +static int io_import_vec(int ddir, struct io_kiocb *req,
>> +                        struct io_async_rw *io, void __user *uvec,
>> +                        size_t uvec_segs)
> 
> Could use a more specific type for uvec:  const struct iovec __user *uvec?

Indeed

-- 
Pavel Begunkov

Re: [PATCH 1/4] io_uring/rw: allocate async data in io_prep_rw()

[Pavel Begunkov]

On 2/24/25 16:52, Caleb Sander Mateos wrote:
> On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> io_prep_rw() relies on async_data being allocated in io_prep_rw_setup().
>> Be a bit more explicit and move the allocation earlier into io_prep_rw()
>> and don't hide it in a call chain.
> 
> Hmm, where is async_data currently used in io_prep_rw()? I don't see

It calls io_prep_rw_pi(), which uses it inside, that's the "relies"
part.

> any reference to async_data in io_prep_rw() until your patch 4,
> "io_uring/rw: open code io_prep_rw_setup()". Would it make sense to
> combine the 2 patches?

Sure, if it rebases cleanly.

-- 
Pavel Begunkov

Re: [PATCH 1/4] io_uring/rw: allocate async data in io_prep_rw()

[Pavel Begunkov]

On 2/24/25 19:21, Pavel Begunkov wrote:
> On 2/24/25 16:52, Caleb Sander Mateos wrote:
>> On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>>
>>> io_prep_rw() relies on async_data being allocated in io_prep_rw_setup().
>>> Be a bit more explicit and move the allocation earlier into io_prep_rw()
>>> and don't hide it in a call chain.
>>
>> Hmm, where is async_data currently used in io_prep_rw()? I don't see
> 
> It calls io_prep_rw_pi(), which uses it inside, that's the "relies"
> part.
> 
>> any reference to async_data in io_prep_rw() until your patch 4,
>> "io_uring/rw: open code io_prep_rw_setup()". Would it make sense to
>> combine the 2 patches?
> 
> Sure, if it rebases cleanly.

It doesn't... I'd rather prefer to keep it as is then.

-- 
Pavel Begunkov