[RFC PATCH v4 0/5] introduce io_uring_cmd_import_fixed_vec

[RFC PATCH v4 0/5] introduce io_uring_cmd_import_fixed_vec

[Sidong Yang]

This patche series introduce io_uring_cmd_import_vec. With this function,
Multiple fixed buffer could be used in uring cmd. It's vectored version
for io_uring_cmd_import_fixed(). Also this patch series includes a usage
for new api for encoded read/write in btrfs by using uring cmd.

There was approximately 10 percent of performance improvements through benchmark.
The benchmark code is in
https://github.com/SidongYang/btrfs-encoded-io-test/blob/main/main.c

./main -l
Elapsed time: 0.598997 seconds
./main -l -f
Elapsed time: 0.540332 seconds

Additionally, There is a commit that fixed a memory bug in btrfs uring encoded
read.

v2:
 - don't export iou_vc, use bvec for btrfs
 - use io_is_compat for checking compat
 - reduce allocation/free for import fixed vec

v3:
 - add iou_vec cache in io_uring_cmd and use it
 - also encoded write fixed supported

v4:
 - add a patch that introduce io_async_cmd
 - add a patch that fixes a bug in btrfs encoded read

Sidong Yang (5):
  io_uring/cmd: introduce io_async_cmd for hide io_uring_cmd_data
  io-uring/cmd: add iou_vec field for io_uring_cmd
  io-uring/cmd: introduce io_uring_cmd_import_fixed_vec
  btrfs: ioctl: introduce btrfs_uring_import_iovec()
  btrfs: ioctl: don't free iov when -EAGAIN in uring encoded read

 fs/btrfs/ioctl.c             | 35 ++++++++++++++++-----
 include/linux/io_uring/cmd.h | 14 +++++++++
 io_uring/io_uring.c          |  4 +--
 io_uring/opdef.c             |  3 +-
 io_uring/uring_cmd.c         | 60 +++++++++++++++++++++++++++++++-----
 io_uring/uring_cmd.h         | 10 ++++++
 6 files changed, 108 insertions(+), 18 deletions(-)

-- 
2.43.0

[RFC PATCH v4 1/5] io_uring/cmd: introduce io_async_cmd for hide io_uring_cmd_data

[Sidong Yang]

io_uring_cmd_data should not be exposed for drivers to avoid to be
abused its private fields. io_async_cmd is new struct that has
io_uring_cmd_data for offset 0. So driver could be use async_data as
io_uring_cmd_data as used before. And private fields would be added in
io_async_cmd.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
 io_uring/io_uring.c  |  2 +-
 io_uring/opdef.c     |  2 +-
 io_uring/uring_cmd.c | 21 ++++++++++++++-------
 io_uring/uring_cmd.h |  6 ++++++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 5ff30a7092ed..513f036bccbb 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -335,7 +335,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 			    sizeof(struct io_async_rw),
 			    offsetof(struct io_async_rw, clear));
 	ret |= io_alloc_cache_init(&ctx->uring_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct io_uring_cmd_data), 0);
+			    sizeof(struct io_async_cmd), 0);
 	spin_lock_init(&ctx->msg_lock);
 	ret |= io_alloc_cache_init(&ctx->msg_cache, IO_ALLOC_CACHE_MAX,
 			    sizeof(struct io_kiocb), 0);
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 7fd173197b1e..e4aa61a414fb 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -416,7 +416,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.plug			= 1,
 		.iopoll			= 1,
 		.iopoll_queue		= 1,
-		.async_size		= sizeof(struct io_uring_cmd_data),
+		.async_size		= sizeof(struct io_async_cmd),
 		.prep			= io_uring_cmd_prep,
 		.issue			= io_uring_cmd,
 	},
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index de39b602aa82..e4cd6fe9fd47 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -19,7 +19,8 @@
 static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
-	struct io_uring_cmd_data *cache = req->async_data;
+	struct io_async_cmd *ac = req->async_data;
+	struct io_uring_cmd_data *cache = &ac->data;
 
 	if (cache->op_data) {
 		kfree(cache->op_data);
@@ -169,12 +170,18 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 				   const struct io_uring_sqe *sqe)
 {
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
-	struct io_uring_cmd_data *cache;
+	struct io_async_cmd *ac;
 
-	cache = io_uring_alloc_async_data(&req->ctx->uring_cache, req);
-	if (!cache)
+	/*
+	 * 'data' must be at offset 0 to allow casting io_async_cmd to
+	 * io_uring_cmd_data in io_uring_cmd_get_async_data().
+	 */
+	BUILD_BUG_ON(offsetof(struct io_async_cmd, data) != 0);
+
+	ac = io_uring_alloc_async_data(&req->ctx->uring_cache, req);
+	if (!ac)
 		return -ENOMEM;
-	cache->op_data = NULL;
+	ac->data.op_data = NULL;
 
 	/*
 	 * Unconditionally cache the SQE for now - this is only needed for
@@ -183,8 +190,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 	 * that it doesn't read in per-op data, play it safe and ensure that
 	 * any SQE data is stable beyond prep. This can later get relaxed.
 	 */
-	memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
-	ioucmd->sqe = cache->sqes;
+	memcpy(ac->data.sqes, sqe, uring_sqe_size(req->ctx));
+	ioucmd->sqe = ac->data.sqes;
 	return 0;
 }
 
diff --git a/io_uring/uring_cmd.h b/io_uring/uring_cmd.h
index f6837ee0955b..f3593012658c 100644
--- a/io_uring/uring_cmd.h
+++ b/io_uring/uring_cmd.h
@@ -1,5 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/io_uring/cmd.h>
+
+struct io_async_cmd {
+	struct io_uring_cmd_data data;
+};
+
 int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags);
 int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 
-- 
2.43.0

[RFC PATCH v4 2/5] io-uring/cmd: add iou_vec field for io_uring_cmd

[Sidong Yang]

This patch adds iou_vec field for io_uring_cmd. Also it needs to be
cleanup for cache. It could be used in uring cmd api that imports
multiple fixed buffers.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
 io_uring/io_uring.c  |  2 +-
 io_uring/opdef.c     |  1 +
 io_uring/uring_cmd.c | 20 ++++++++++++++++++++
 io_uring/uring_cmd.h |  4 ++++
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 513f036bccbb..08506d1224c5 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -289,7 +289,7 @@ static void io_free_alloc_caches(struct io_ring_ctx *ctx)
 	io_alloc_cache_free(&ctx->apoll_cache, kfree);
 	io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free);
 	io_alloc_cache_free(&ctx->rw_cache, io_rw_cache_free);
-	io_alloc_cache_free(&ctx->uring_cache, kfree);
+	io_alloc_cache_free(&ctx->uring_cache, io_cmd_cache_free);
 	io_alloc_cache_free(&ctx->msg_cache, kfree);
 	io_futex_cache_free(ctx);
 	io_rsrc_cache_free(ctx);
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index e4aa61a414fb..489384c0438b 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -755,6 +755,7 @@ const struct io_cold_def io_cold_defs[] = {
 	},
 	[IORING_OP_URING_CMD] = {
 		.name			= "URING_CMD",
+		.cleanup		= io_uring_cmd_cleanup,
 	},
 	[IORING_OP_SEND_ZC] = {
 		.name			= "SEND_ZC",
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index e4cd6fe9fd47..bf4002e93ec5 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -29,6 +29,13 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (issue_flags & IO_URING_F_UNLOCKED)
 		return;
+
+	req->flags &= ~REQ_F_NEED_CLEANUP;
+
+	io_alloc_cache_vec_kasan(&ac->iou_vec);
+	if (ac->iou_vec.nr > IO_VEC_CACHE_SOFT_CAP)
+		io_vec_free(&ac->iou_vec);
+
 	if (io_alloc_cache_put(&req->ctx->uring_cache, cache)) {
 		ioucmd->sqe = NULL;
 		req->async_data = NULL;
@@ -36,6 +43,11 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
+void io_uring_cmd_cleanup(struct io_kiocb *req)
+{
+	io_req_uring_cleanup(req, 0);
+}
+
 bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,
 				   struct io_uring_task *tctx, bool cancel_all)
 {
@@ -346,3 +358,11 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags)
 }
 EXPORT_SYMBOL_GPL(io_uring_cmd_sock);
 #endif
+
+void io_cmd_cache_free(const void *entry)
+{
+	struct io_async_cmd *ac = (struct io_async_cmd *)entry;
+
+	io_vec_free(&ac->iou_vec);
+	kfree(ac);
+}
diff --git a/io_uring/uring_cmd.h b/io_uring/uring_cmd.h
index f3593012658c..8986224e0c57 100644
--- a/io_uring/uring_cmd.h
+++ b/io_uring/uring_cmd.h
@@ -1,13 +1,17 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/io_uring_types.h>
 
 #include <linux/io_uring/cmd.h>
 
 struct io_async_cmd {
 	struct io_uring_cmd_data data;
+	struct iou_vec iou_vec;
 };
 
 int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags);
 int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
+void io_uring_cmd_cleanup(struct io_kiocb *req);
 
 bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,
 				   struct io_uring_task *tctx, bool cancel_all);
+void io_cmd_cache_free(const void *entry);
-- 
2.43.0

[RFC PATCH v4 3/5] io-uring/cmd: introduce io_uring_cmd_import_fixed_vec

[Sidong Yang]

io_uring_cmd_import_fixed_vec() could be used for using multiple
fixed buffer in uring_cmd callback.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
 include/linux/io_uring/cmd.h | 14 ++++++++++++++
 io_uring/uring_cmd.c         | 19 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 598cacda4aa3..ab7ecef60787 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -44,6 +44,12 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
 			      struct io_uring_cmd *ioucmd,
 			      unsigned int issue_flags);
 
+int io_uring_cmd_import_fixed_vec(struct io_uring_cmd *ioucmd,
+				  const struct iovec __user *uvec,
+				  unsigned long uvec_segs, int ddir,
+				  unsigned int issue_flags,
+				  struct iov_iter *iter);
+
 /*
  * Completes the request, i.e. posts an io_uring CQE and deallocates @ioucmd
  * and the corresponding io_uring request.
@@ -76,6 +82,14 @@ io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
 {
 	return -EOPNOTSUPP;
 }
+int io_uring_cmd_import_fixed_vec(struct io_uring_cmd *ioucmd,
+				  const struct iovec __user *uvec,
+				  unsigned long uvec_segs, int ddir,
+				  unsigned int issue_flags,
+				  struct iov_iter *iter)
+{
+	return -EOPNOTSUPP;
+}
 static inline void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret,
 		u64 ret2, unsigned issue_flags)
 {
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index bf4002e93ec5..effcd01b8a35 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -274,6 +274,25 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
 }
 EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed);
 
+int io_uring_cmd_import_fixed_vec(struct io_uring_cmd *ioucmd,
+				  const struct iovec __user *uvec,
+				  unsigned long uvec_segs, int ddir,
+				  unsigned int issue_flags,
+				  struct iov_iter *iter)
+{
+	struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
+	struct io_async_cmd *ac = req->async_data;
+	int ret;
+
+	ret = io_prep_reg_iovec(req, &ac->iou_vec, uvec, uvec_segs);
+	if (ret)
+		return ret;
+
+	return io_import_reg_vec(ddir, iter, req, &ac->iou_vec, ac->iou_vec.nr,
+				 issue_flags);
+}
+EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed_vec);
+
 void io_uring_cmd_issue_blocking(struct io_uring_cmd *ioucmd)
 {
 	struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
-- 
2.43.0

[RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Sidong Yang]

This patch introduces btrfs_uring_import_iovec(). In encoded read/write
with uring cmd, it uses import_iovec without supporting fixed buffer.
btrfs_using_import_iovec() could use fixed buffer if cmd flags has
IORING_URING_CMD_FIXED.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
 fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6c18bad53cd3..a7b52fd99059 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
 	struct iov_iter iter;
 };
 
+static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
+				    unsigned int issue_flags, int rw)
+{
+	struct btrfs_uring_encoded_data *data =
+		io_uring_cmd_get_async_data(cmd)->op_data;
+	int ret;
+
+	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
+		data->iov = NULL;
+		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
+						    data->args.iovcnt,
+						    ITER_DEST, issue_flags,
+						    &data->iter);
+	} else {
+		data->iov = data->iovstack;
+		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
+				   ARRAY_SIZE(data->iovstack), &data->iov,
+				   &data->iter);
+	}
+	return ret;
+}
+
 static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
 	size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags);
@@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 			goto out_acct;
 		}
 
-		data->iov = data->iovstack;
-		ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt,
-				   ARRAY_SIZE(data->iovstack), &data->iov,
-				   &data->iter);
+		ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST);
 		if (ret < 0)
 			goto out_acct;
 
@@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu
 		if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset)
 			goto out_acct;
 
-		data->iov = data->iovstack;
-		ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt,
-				   ARRAY_SIZE(data->iovstack), &data->iov,
-				   &data->iter);
+		ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE);
 		if (ret < 0)
 			goto out_acct;
 
-- 
2.43.0

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Caleb Sander Mateos]

On Mon, Mar 17, 2025 at 7:00 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>
> This patch introduces btrfs_uring_import_iovec(). In encoded read/write
> with uring cmd, it uses import_iovec without supporting fixed buffer.
> btrfs_using_import_iovec() could use fixed buffer if cmd flags has
> IORING_URING_CMD_FIXED.
>
> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> ---
>  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6c18bad53cd3..a7b52fd99059 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
>         struct iov_iter iter;
>  };
>
> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
> +                                   unsigned int issue_flags, int rw)
> +{
> +       struct btrfs_uring_encoded_data *data =
> +               io_uring_cmd_get_async_data(cmd)->op_data;
> +       int ret;
> +
> +       if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
> +               data->iov = NULL;
> +               ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
> +                                                   data->args.iovcnt,
> +                                                   ITER_DEST, issue_flags,

Why ITER_DEST instead of rw?

Best,
Caleb

> +                                                   &data->iter);
> +       } else {
> +               data->iov = data->iovstack;
> +               ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
> +                                  ARRAY_SIZE(data->iovstack), &data->iov,
> +                                  &data->iter);
> +       }
> +       return ret;
> +}
> +
>  static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags)
>  {
>         size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags);
> @@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>                         goto out_acct;
>                 }
>
> -               data->iov = data->iovstack;
> -               ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt,
> -                                  ARRAY_SIZE(data->iovstack), &data->iov,
> -                                  &data->iter);
> +               ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST);
>                 if (ret < 0)
>                         goto out_acct;
>
> @@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu
>                 if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset)
>                         goto out_acct;
>
> -               data->iov = data->iovstack;
> -               ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt,
> -                                  ARRAY_SIZE(data->iovstack), &data->iov,
> -                                  &data->iter);
> +               ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE);
>                 if (ret < 0)
>                         goto out_acct;
>
> --
> 2.43.0
>
>

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Sidong Yang]

On Mon, Mar 17, 2025 at 08:37:04AM -0700, Caleb Sander Mateos wrote:
> On Mon, Mar 17, 2025 at 7:00 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >
> > This patch introduces btrfs_uring_import_iovec(). In encoded read/write
> > with uring cmd, it uses import_iovec without supporting fixed buffer.
> > btrfs_using_import_iovec() could use fixed buffer if cmd flags has
> > IORING_URING_CMD_FIXED.
> >
> > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > ---
> >  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 6c18bad53cd3..a7b52fd99059 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
> >         struct iov_iter iter;
> >  };
> >
> > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
> > +                                   unsigned int issue_flags, int rw)
> > +{
> > +       struct btrfs_uring_encoded_data *data =
> > +               io_uring_cmd_get_async_data(cmd)->op_data;
> > +       int ret;
> > +
> > +       if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
> > +               data->iov = NULL;
> > +               ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
> > +                                                   data->args.iovcnt,
> > +                                                   ITER_DEST, issue_flags,
> 
> Why ITER_DEST instead of rw?

Oh, it's a mistake. It should be rw.

Thanks,
Sidong

> 
> Best,
> Caleb
> 
> > +                                                   &data->iter);
> > +       } else {
> > +               data->iov = data->iovstack;
> > +               ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
> > +                                  ARRAY_SIZE(data->iovstack), &data->iov,
> > +                                  &data->iter);
> > +       }
> > +       return ret;
> > +}
> > +
> >  static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags)
> >  {
> >         size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags);
> > @@ -4874,10 +4896,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
> >                         goto out_acct;
> >                 }
> >
> > -               data->iov = data->iovstack;
> > -               ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt,
> > -                                  ARRAY_SIZE(data->iovstack), &data->iov,
> > -                                  &data->iter);
> > +               ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_DEST);
> >                 if (ret < 0)
> >                         goto out_acct;
> >
> > @@ -5022,10 +5041,7 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu
> >                 if (data->args.len > data->args.unencoded_len - data->args.unencoded_offset)
> >                         goto out_acct;
> >
> > -               data->iov = data->iovstack;
> > -               ret = import_iovec(ITER_SOURCE, data->args.iov, data->args.iovcnt,
> > -                                  ARRAY_SIZE(data->iovstack), &data->iov,
> > -                                  &data->iter);
> > +               ret = btrfs_uring_import_iovec(cmd, issue_flags, ITER_SOURCE);
> >                 if (ret < 0)
> >                         goto out_acct;
> >
> > --
> > 2.43.0
> >
> >

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Jens Axboe]

On 3/17/25 7:57 AM, Sidong Yang wrote:
> This patch introduces btrfs_uring_import_iovec(). In encoded read/write
> with uring cmd, it uses import_iovec without supporting fixed buffer.
> btrfs_using_import_iovec() could use fixed buffer if cmd flags has
> IORING_URING_CMD_FIXED.
> 
> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> ---
>  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6c18bad53cd3..a7b52fd99059 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
>  	struct iov_iter iter;
>  };
>  
> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
> +				    unsigned int issue_flags, int rw)
> +{
> +	struct btrfs_uring_encoded_data *data =
> +		io_uring_cmd_get_async_data(cmd)->op_data;
> +	int ret;
> +
> +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
> +		data->iov = NULL;
> +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
> +						    data->args.iovcnt,
> +						    ITER_DEST, issue_flags,
> +						    &data->iter);
> +	} else {
> +		data->iov = data->iovstack;
> +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
> +				   ARRAY_SIZE(data->iovstack), &data->iov,
> +				   &data->iter);
> +	}
> +	return ret;
> +}

How can 'cmd' be NULL here?


-- 
Jens Axboe

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Sidong Yang]

On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote:
> On 3/17/25 7:57 AM, Sidong Yang wrote:
> > This patch introduces btrfs_uring_import_iovec(). In encoded read/write
> > with uring cmd, it uses import_iovec without supporting fixed buffer.
> > btrfs_using_import_iovec() could use fixed buffer if cmd flags has
> > IORING_URING_CMD_FIXED.
> > 
> > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > ---
> >  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 6c18bad53cd3..a7b52fd99059 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
> >  	struct iov_iter iter;
> >  };
> >  
> > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
> > +				    unsigned int issue_flags, int rw)
> > +{
> > +	struct btrfs_uring_encoded_data *data =
> > +		io_uring_cmd_get_async_data(cmd)->op_data;
> > +	int ret;
> > +
> > +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
> > +		data->iov = NULL;
> > +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
> > +						    data->args.iovcnt,
> > +						    ITER_DEST, issue_flags,
> > +						    &data->iter);
> > +	} else {
> > +		data->iov = data->iovstack;
> > +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
> > +				   ARRAY_SIZE(data->iovstack), &data->iov,
> > +				   &data->iter);
> > +	}
> > +	return ret;
> > +}
> 
> How can 'cmd' be NULL here?

It seems that there is no checkes for 'cmd' before and it works same as before.
But I think it's better to add a check in function start for safety.

> 
> 
> -- 
> Jens Axboe
> 

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Pavel Begunkov]

On 3/18/25 01:58, Sidong Yang wrote:
> On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote:
>> On 3/17/25 7:57 AM, Sidong Yang wrote:
>>> This patch introduces btrfs_uring_import_iovec(). In encoded read/write
>>> with uring cmd, it uses import_iovec without supporting fixed buffer.
>>> btrfs_using_import_iovec() could use fixed buffer if cmd flags has
>>> IORING_URING_CMD_FIXED.
>>>
>>> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
>>> ---
>>>   fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index 6c18bad53cd3..a7b52fd99059 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
>>>   	struct iov_iter iter;
>>>   };
>>>   
>>> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
>>> +				    unsigned int issue_flags, int rw)
>>> +{
>>> +	struct btrfs_uring_encoded_data *data =
>>> +		io_uring_cmd_get_async_data(cmd)->op_data;
>>> +	int ret;
>>> +
>>> +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
>>> +		data->iov = NULL;
>>> +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
>>> +						    data->args.iovcnt,
>>> +						    ITER_DEST, issue_flags,
>>> +						    &data->iter);
>>> +	} else {
>>> +		data->iov = data->iovstack;
>>> +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
>>> +				   ARRAY_SIZE(data->iovstack), &data->iov,
>>> +				   &data->iter);
>>> +	}
>>> +	return ret;
>>> +}
>>
>> How can 'cmd' be NULL here?
> 
> It seems that there is no checkes for 'cmd' before and it works same as before.
> But I think it's better to add a check in function start for safety.

The check goes two lines after you already dereferenced it, and it
seems to be called from io_uring cmd specific code. The null check
only adds to confusion.

-- 
Pavel Begunkov

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Sidong Yang]

On Tue, Mar 18, 2025 at 07:25:51AM +0000, Pavel Begunkov wrote:
> On 3/18/25 01:58, Sidong Yang wrote:
> > On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote:
> > > On 3/17/25 7:57 AM, Sidong Yang wrote:
> > > > This patch introduces btrfs_uring_import_iovec(). In encoded read/write
> > > > with uring cmd, it uses import_iovec without supporting fixed buffer.
> > > > btrfs_using_import_iovec() could use fixed buffer if cmd flags has
> > > > IORING_URING_CMD_FIXED.
> > > > 
> > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > > > ---
> > > >   fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
> > > >   1 file changed, 24 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > > index 6c18bad53cd3..a7b52fd99059 100644
> > > > --- a/fs/btrfs/ioctl.c
> > > > +++ b/fs/btrfs/ioctl.c
> > > > @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
> > > >   	struct iov_iter iter;
> > > >   };
> > > > +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
> > > > +				    unsigned int issue_flags, int rw)
> > > > +{
> > > > +	struct btrfs_uring_encoded_data *data =
> > > > +		io_uring_cmd_get_async_data(cmd)->op_data;
> > > > +	int ret;
> > > > +
> > > > +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
> > > > +		data->iov = NULL;
> > > > +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
> > > > +						    data->args.iovcnt,
> > > > +						    ITER_DEST, issue_flags,
> > > > +						    &data->iter);
> > > > +	} else {
> > > > +		data->iov = data->iovstack;
> > > > +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
> > > > +				   ARRAY_SIZE(data->iovstack), &data->iov,
> > > > +				   &data->iter);
> > > > +	}
> > > > +	return ret;
> > > > +}
> > > 
> > > How can 'cmd' be NULL here?
> > 
> > It seems that there is no checkes for 'cmd' before and it works same as before.
> > But I think it's better to add a check in function start for safety.
> 
> The check goes two lines after you already dereferenced it, and it
> seems to be called from io_uring cmd specific code. The null check
> only adds to confusion.

You mean 'cmd' already dereferenced for async_data. Is it okay to just delete code
checking 'cmd' like below?

if (cmd->flags & IORING_URING_CMD_FIXED) {

Thanks,
Sidong

> 
> -- 
> Pavel Begunkov
> 

Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Jens Axboe]

On 3/18/25 1:55 AM, Sidong Yang wrote:
> On Tue, Mar 18, 2025 at 07:25:51AM +0000, Pavel Begunkov wrote:
>> On 3/18/25 01:58, Sidong Yang wrote:
>>> On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote:
>>>> On 3/17/25 7:57 AM, Sidong Yang wrote:
>>>>> This patch introduces btrfs_uring_import_iovec(). In encoded read/write
>>>>> with uring cmd, it uses import_iovec without supporting fixed buffer.
>>>>> btrfs_using_import_iovec() could use fixed buffer if cmd flags has
>>>>> IORING_URING_CMD_FIXED.
>>>>>
>>>>> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
>>>>> ---
>>>>>   fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>> index 6c18bad53cd3..a7b52fd99059 100644
>>>>> --- a/fs/btrfs/ioctl.c
>>>>> +++ b/fs/btrfs/ioctl.c
>>>>> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
>>>>>   	struct iov_iter iter;
>>>>>   };
>>>>> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
>>>>> +				    unsigned int issue_flags, int rw)
>>>>> +{
>>>>> +	struct btrfs_uring_encoded_data *data =
>>>>> +		io_uring_cmd_get_async_data(cmd)->op_data;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
>>>>> +		data->iov = NULL;
>>>>> +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
>>>>> +						    data->args.iovcnt,
>>>>> +						    ITER_DEST, issue_flags,
>>>>> +						    &data->iter);
>>>>> +	} else {
>>>>> +		data->iov = data->iovstack;
>>>>> +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
>>>>> +				   ARRAY_SIZE(data->iovstack), &data->iov,
>>>>> +				   &data->iter);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>
>>>> How can 'cmd' be NULL here?
>>>
>>> It seems that there is no checkes for 'cmd' before and it works same as before.
>>> But I think it's better to add a check in function start for safety.
>>
>> The check goes two lines after you already dereferenced it, and it
>> seems to be called from io_uring cmd specific code. The null check
>> only adds to confusion.
> 
> You mean 'cmd' already dereferenced for async_data. Is it okay to just
> delete code checking 'cmd' like below?
> 
> if (cmd->flags & IORING_URING_CMD_FIXED) {

Either 'cmd' may be NULL and it's valid (and the current code is fine),
or it'd be invalid, in which case that should return an error. But we
don't add random pointer == NULL checks as defensive programming. And
this one should never ever see cmd == NULL, hence the code needs to go
away. It's just confusing otherwise. Consider you reading some code
trying to understand what it does, and it has a check for cmd == NULL in
there. That would make you wonder "hmm what cases pass in a NULL for cmd
here?". When the answer is "this should never happen", don't have the
check. It just obfuscates the code.

-- 
Jens Axboe

[RFC PATCH v4 5/5] btrfs: ioctl: don't free iov when -EAGAIN in uring encoded read

[Sidong Yang]

This patch fixes a bug on encoded_read. In btrfs_uring_encoded_read(),
btrfs_encoded_read could return -EAGAIN when receiving requests concurrently.
And data->iov goes to out_free and it freed and return -EAGAIN. io-uring
subsystem would call it again and it doesn't reset data. And data->iov
freed and iov_iter reference it. iov_iter would be used in
btrfs_uring_read_finished() and could be raise memory bug.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a7b52fd99059..02fa8dd1a3ce 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4922,6 +4922,9 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 
 	ret = btrfs_encoded_read(&kiocb, &data->iter, &data->args, &cached_state,
 				 &disk_bytenr, &disk_io_size);
+
+	if (ret == -EAGAIN)
+		goto out_acct;
 	if (ret < 0 && ret != -EIOCBQUEUED)
 		goto out_free;
 
-- 
2.43.0

Re: [RFC PATCH v4 5/5] btrfs: ioctl: don't free iov when -EAGAIN in uring encoded read

[Pavel Begunkov]

On 3/17/25 13:57, Sidong Yang wrote:
> This patch fixes a bug on encoded_read. In btrfs_uring_encoded_read(),
> btrfs_encoded_read could return -EAGAIN when receiving requests concurrently.
> And data->iov goes to out_free and it freed and return -EAGAIN. io-uring
> subsystem would call it again and it doesn't reset data. And data->iov
> freed and iov_iter reference it. iov_iter would be used in
> btrfs_uring_read_finished() and could be raise memory bug.

Fixes should go first. Please send it separately, and CC Mark.
A "Fixes" tag would also be good to have.

> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> ---
>   fs/btrfs/ioctl.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a7b52fd99059..02fa8dd1a3ce 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4922,6 +4922,9 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   
>   	ret = btrfs_encoded_read(&kiocb, &data->iter, &data->args, &cached_state,
>   				 &disk_bytenr, &disk_io_size);
> +
> +	if (ret == -EAGAIN)
> +		goto out_acct;
>   	if (ret < 0 && ret != -EIOCBQUEUED)
>   		goto out_free;
>   

-- 
Pavel Begunkov

Re: [RFC PATCH v4 5/5] btrfs: ioctl: don't free iov when -EAGAIN in uring encoded read

[Sidong Yang]

On Tue, Mar 18, 2025 at 07:21:00AM +0000, Pavel Begunkov wrote:
> On 3/17/25 13:57, Sidong Yang wrote:
> > This patch fixes a bug on encoded_read. In btrfs_uring_encoded_read(),
> > btrfs_encoded_read could return -EAGAIN when receiving requests concurrently.
> > And data->iov goes to out_free and it freed and return -EAGAIN. io-uring
> > subsystem would call it again and it doesn't reset data. And data->iov
> > freed and iov_iter reference it. iov_iter would be used in
> > btrfs_uring_read_finished() and could be raise memory bug.
> 
> Fixes should go first. Please send it separately, and CC Mark.
> A "Fixes" tag would also be good to have.

Okay, I'll remove this from patch series.

Thanks,
Sidong

> 
> > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > ---
> >   fs/btrfs/ioctl.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index a7b52fd99059..02fa8dd1a3ce 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -4922,6 +4922,9 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
> >   	ret = btrfs_encoded_read(&kiocb, &data->iter, &data->args, &cached_state,
> >   				 &disk_bytenr, &disk_io_size);
> > +
> > +	if (ret == -EAGAIN)
> > +		goto out_acct;
> >   	if (ret < 0 && ret != -EIOCBQUEUED)
> >   		goto out_free;
> 
> -- 
> Pavel Begunkov
> 

Re: [RFC PATCH v4 0/5] introduce io_uring_cmd_import_fixed_vec

[Pavel Begunkov]

On 3/17/25 13:57, Sidong Yang wrote:
> This patche series introduce io_uring_cmd_import_vec. With this function,
> Multiple fixed buffer could be used in uring cmd. It's vectored version
> for io_uring_cmd_import_fixed(). Also this patch series includes a usage
> for new api for encoded read/write in btrfs by using uring cmd.

You're vigorously ignoring the previous comment, you can't stick
your name to my patches and send them as your own, that's not
going to work. git format-patch and other tools allow to send
other's patches in the same patch set without mutilating them.

-- 
Pavel Begunkov

Re: [RFC PATCH v4 0/5] introduce io_uring_cmd_import_fixed_vec

[Sidong Yang]

On Tue, Mar 18, 2025 at 07:30:51AM +0000, Pavel Begunkov wrote:
> On 3/17/25 13:57, Sidong Yang wrote:
> > This patche series introduce io_uring_cmd_import_vec. With this function,
> > Multiple fixed buffer could be used in uring cmd. It's vectored version
> > for io_uring_cmd_import_fixed(). Also this patch series includes a usage
> > for new api for encoded read/write in btrfs by using uring cmd.
> 
> You're vigorously ignoring the previous comment, you can't stick
> your name to my patches and send them as your own, that's not
> going to work. git format-patch and other tools allow to send
> other's patches in the same patch set without mutilating them.

I'm just not familiar with this. That wasn't my intention. Sorry, Your
patches will be included without modification.

Thanks,
Sidong
> 
> -- 
> Pavel Begunkov
> 

Re: [RFC PATCH v4 0/5] introduce io_uring_cmd_import_fixed_vec

[Jens Axboe]

On 3/18/25 1:41 AM, Sidong Yang wrote:
> On Tue, Mar 18, 2025 at 07:30:51AM +0000, Pavel Begunkov wrote:
>> On 3/17/25 13:57, Sidong Yang wrote:
>>> This patche series introduce io_uring_cmd_import_vec. With this function,
>>> Multiple fixed buffer could be used in uring cmd. It's vectored version
>>> for io_uring_cmd_import_fixed(). Also this patch series includes a usage
>>> for new api for encoded read/write in btrfs by using uring cmd.
>>
>> You're vigorously ignoring the previous comment, you can't stick
>> your name to my patches and send them as your own, that's not
>> going to work. git format-patch and other tools allow to send
>> other's patches in the same patch set without mutilating them.
> 
> I'm just not familiar with this. That wasn't my intention. Sorry, Your
> patches will be included without modification.

Assuming these are from a git branch you have, just ensure they are
committed with Pavel as the author, and git send-email will do the right
thing when sending out the patch.

-- 
Jens Axboe