[PATCH 0/4] io_uring/btrfs: remove struct io_uring_cmd_data

[PATCH 0/4] io_uring/btrfs: remove struct io_uring_cmd_data

[Caleb Sander Mateos]

btrfs's ->uring_cmd() implementations are the only ones using io_uring_cmd_data
to store data that lasts for the lifetime of the uring_cmd. But all uring_cmds
have to pay the memory and CPU cost of initializing this field and freeing the
pointer if necessary when the uring_cmd ends. There is already a pdu field in
struct io_uring_cmd that ->uring_cmd() implementations can use for storage. The
only benefit of op_data seems to be that io_uring initializes it, so
->uring_cmd() can read it to tell if there was a previous call to ->uring_cmd().

Introduce a flag IORING_URING_CMD_REISSUE that ->uring_cmd() implementations can
use to tell if this is the first call to ->uring_cmd() or a reissue of the
uring_cmd. Switch btrfs to use the pdu storage for its btrfs_uring_encoded_data.
If IORING_URING_CMD_REISSUE is unset, allocate a new btrfs_uring_encoded_data.
If it's set, use the existing one in op_data. Free the btrfs_uring_encoded_data
in the btrfs layer instead of relying on io_uring to free op_data. Finally,
remove io_uring_cmd_data since it's now unused.

Caleb Sander Mateos (4):
  btrfs/ioctl: don't skip accounting in early ENOTTY return
  io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag
  btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd
  io_uring/cmd: remove struct io_uring_cmd_data

 fs/btrfs/ioctl.c             | 41 +++++++++++++++++++++++++-----------
 include/linux/io_uring/cmd.h | 11 ++--------
 io_uring/uring_cmd.c         | 14 +++---------
 io_uring/uring_cmd.h         |  1 -
 4 files changed, 34 insertions(+), 33 deletions(-)

-- 
2.45.2

[PATCH 1/4] btrfs/ioctl: don't skip accounting in early ENOTTY return

[Caleb Sander Mateos]

btrfs_uring_encoded_read() returns early with -ENOTTY if the uring_cmd
is issued with IO_URING_F_COMPAT but the kernel doesn't support compat
syscalls. However, this early return bypasses the syscall accounting.
goto out_acct instead to ensure the syscall is counted.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: 34310c442e17 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
---
 fs/btrfs/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 913acef3f0a9..ff15160e2581 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4827,11 +4827,12 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 
 	if (issue_flags & IO_URING_F_COMPAT) {
 #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
 		copy_end = offsetofend(struct btrfs_ioctl_encoded_io_args_32, flags);
 #else
-		return -ENOTTY;
+		ret = -ENOTTY;
+		goto out_acct;
 #endif
 	} else {
 		copy_end = copy_end_kernel;
 	}
 
-- 
2.45.2

[PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Caleb Sander Mateos]

Add a flag IORING_URING_CMD_REISSUE that ->uring_cmd() implementations
can use to tell whether this is the first or subsequent issue of the
uring_cmd. This will allow ->uring_cmd() implementations to store
information in the io_uring_cmd's pdu across issues.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 include/linux/io_uring/cmd.h | 2 ++
 io_uring/uring_cmd.c         | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 53408124c1e5..29892f54e0ac 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -6,10 +6,12 @@
 #include <linux/io_uring_types.h>
 #include <linux/blk-mq.h>
 
 /* only top 8 bits of sqe->uring_cmd_flags for kernel internal use */
 #define IORING_URING_CMD_CANCELABLE	(1U << 30)
+/* io_uring_cmd is being issued again */
+#define IORING_URING_CMD_REISSUE	(1U << 31)
 
 struct io_uring_cmd {
 	struct file	*file;
 	const struct io_uring_sqe *sqe;
 	/* callback to defer completions to task context */
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 929cad6ee326..7cddc4c1c554 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -257,10 +257,12 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 			req->iopoll_start = ktime_get_ns();
 		}
 	}
 
 	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
+	if (ret == -EAGAIN)
+		ioucmd->flags |= IORING_URING_CMD_REISSUE;
 	if (ret == -EAGAIN || ret == -EIOCBQUEUED)
 		return ret;
 	if (ret < 0)
 		req_set_fail(req);
 	io_req_uring_cleanup(req, issue_flags);
-- 
2.45.2

[PATCH 3/4] btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd

[Caleb Sander Mateos]

btrfs is the only user of struct io_uring_cmd_data and its op_data
field. Switch its ->uring_cmd() implementations to store the struct
btrfs_uring_encoded_data * in the struct io_btrfs_cmd, overlayed with
io_uring_cmd's pdu field. This avoids having to touch another cache line
to access the struct btrfs_uring_encoded_data *, and allows op_data and
struct io_uring_cmd_data to be removed.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 fs/btrfs/ioctl.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ff15160e2581..6183729c93a1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4627,10 +4627,17 @@ static int btrfs_ioctl_encoded_write(struct file *file, void __user *argp, bool
 		add_wchar(current, ret);
 	inc_syscw(current);
 	return ret;
 }
 
+struct btrfs_uring_encoded_data {
+	struct btrfs_ioctl_encoded_io_args args;
+	struct iovec iovstack[UIO_FASTIOV];
+	struct iovec *iov;
+	struct iov_iter iter;
+};
+
 /*
  * Context that's attached to an encoded read io_uring command, in cmd->pdu. It
  * contains the fields in btrfs_uring_read_extent that are necessary to finish
  * off and cleanup the I/O in btrfs_uring_read_finished.
  */
@@ -4648,10 +4655,11 @@ struct btrfs_uring_priv {
 	int err;
 	bool compressed;
 };
 
 struct io_btrfs_cmd {
+	struct btrfs_uring_encoded_data *data;
 	struct btrfs_uring_priv *priv;
 };
 
 static void btrfs_uring_read_finished(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
@@ -4706,10 +4714,11 @@ static void btrfs_uring_read_finished(struct io_uring_cmd *cmd, unsigned int iss
 		__free_page(priv->pages[index]);
 
 	kfree(priv->pages);
 	kfree(priv->iov);
 	kfree(priv);
+	kfree(bc->data);
 }
 
 void btrfs_uring_read_extent_endio(void *ctx, int err)
 {
 	struct btrfs_uring_priv *priv = ctx;
@@ -4789,17 +4798,10 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
 	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
 	kfree(priv);
 	return ret;
 }
 
-struct btrfs_uring_encoded_data {
-	struct btrfs_ioctl_encoded_io_args args;
-	struct iovec iovstack[UIO_FASTIOV];
-	struct iovec *iov;
-	struct iov_iter iter;
-};
-
 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);
 	size_t copy_end;
 	int ret;
@@ -4811,11 +4813,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 	loff_t pos;
 	struct kiocb kiocb;
 	struct extent_state *cached_state = NULL;
 	u64 start, lockend;
 	void __user *sqe_addr;
-	struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
+	struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
+	struct btrfs_uring_encoded_data *data = NULL;
+
+	if (cmd->flags & IORING_URING_CMD_REISSUE)
+		data = bc->data;
 
 	if (!capable(CAP_SYS_ADMIN)) {
 		ret = -EPERM;
 		goto out_acct;
 	}
@@ -4841,11 +4847,11 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 		if (!data) {
 			ret = -ENOMEM;
 			goto out_acct;
 		}
 
-		io_uring_cmd_get_async_data(cmd)->op_data = data;
+		bc->data = data;
 
 		if (issue_flags & IO_URING_F_COMPAT) {
 #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
 			struct btrfs_ioctl_encoded_io_args_32 args32;
 
@@ -4939,21 +4945,28 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 out_acct:
 	if (ret > 0)
 		add_rchar(current, ret);
 	inc_syscr(current);
 
+	if (ret != -EIOCBQUEUED && ret != -EAGAIN)
+		kfree(data);
+
 	return ret;
 }
 
 static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
 	loff_t pos;
 	struct kiocb kiocb;
 	struct file *file;
 	ssize_t ret;
 	void __user *sqe_addr;
-	struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
+	struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
+	struct btrfs_uring_encoded_data *data = NULL;
+
+	if (cmd->flags & IORING_URING_CMD_REISSUE)
+		data = bc->data;
 
 	if (!capable(CAP_SYS_ADMIN)) {
 		ret = -EPERM;
 		goto out_acct;
 	}
@@ -4971,11 +4984,11 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu
 		if (!data) {
 			ret = -ENOMEM;
 			goto out_acct;
 		}
 
-		io_uring_cmd_get_async_data(cmd)->op_data = data;
+		bc->data = data;
 
 		if (issue_flags & IO_URING_F_COMPAT) {
 #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
 			struct btrfs_ioctl_encoded_io_args_32 args32;
 
@@ -5061,10 +5074,13 @@ static int btrfs_uring_encoded_write(struct io_uring_cmd *cmd, unsigned int issu
 	kfree(data->iov);
 out_acct:
 	if (ret > 0)
 		add_wchar(current, ret);
 	inc_syscw(current);
+
+	if (ret != -EAGAIN)
+		kfree(data);
 	return ret;
 }
 
 int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
-- 
2.45.2

[PATCH 4/4] io_uring/cmd: remove struct io_uring_cmd_data

[Caleb Sander Mateos]

There are no more users of struct io_uring_cmd_data and its op_data
field. Remove it to shave 8 bytes from struct io_async_cmd and eliminate
a store and load for every uring_cmd.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 include/linux/io_uring/cmd.h |  9 ---------
 io_uring/uring_cmd.c         | 12 +-----------
 io_uring/uring_cmd.h         |  1 -
 3 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 29892f54e0ac..cfa6d0c0c322 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -19,14 +19,10 @@ struct io_uring_cmd {
 	u32		cmd_op;
 	u32		flags;
 	u8		pdu[32]; /* available inline for free use */
 };
 
-struct io_uring_cmd_data {
-	void			*op_data;
-};
-
 static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)
 {
 	return sqe->cmd;
 }
 
@@ -135,15 +131,10 @@ static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
 static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd)
 {
 	return cmd_to_io_kiocb(cmd)->tctx->task;
 }
 
-static inline struct io_uring_cmd_data *io_uring_cmd_get_async_data(struct io_uring_cmd *cmd)
-{
-	return cmd_to_io_kiocb(cmd)->async_data;
-}
-
 /*
  * Return uring_cmd's context reference as its context handle for driver to
  * track per-context resource, such as registered kernel IO buffer
  */
 static inline void *io_uring_cmd_ctx_handle(struct io_uring_cmd *cmd)
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 7cddc4c1c554..376404ffb3f9 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -23,25 +23,19 @@ void io_cmd_cache_free(const void *entry)
 
 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_async_cmd *ac = req->async_data;
-	struct io_uring_cmd_data *cache = &ac->data;
-
-	if (cache->op_data) {
-		kfree(cache->op_data);
-		cache->op_data = NULL;
-	}
 
 	if (issue_flags & IO_URING_F_UNLOCKED)
 		return;
 
 	io_alloc_cache_vec_kasan(&ac->vec);
 	if (ac->vec.nr > IO_VEC_CACHE_SOFT_CAP)
 		io_vec_free(&ac->vec);
 
-	if (io_alloc_cache_put(&req->ctx->cmd_cache, cache)) {
+	if (io_alloc_cache_put(&req->ctx->cmd_cache, ac)) {
 		ioucmd->sqe = NULL;
 		req->async_data = NULL;
 		req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
 	}
 }
@@ -185,17 +179,13 @@ 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_async_cmd *ac;
 
-	/* see io_uring_cmd_get_async_data() */
-	BUILD_BUG_ON(offsetof(struct io_async_cmd, data) != 0);
-
 	ac = io_uring_alloc_async_data(&req->ctx->cmd_cache, req);
 	if (!ac)
 		return -ENOMEM;
-	ac->data.op_data = NULL;
 
 	/*
 	 * Unconditionally cache the SQE for now - this is only needed for
 	 * requests that go async, but prep handlers must ensure that any
 	 * sqe data is stable beyond prep. Since uring_cmd is special in
diff --git a/io_uring/uring_cmd.h b/io_uring/uring_cmd.h
index e6a5142c890e..351c58c26679 100644
--- a/io_uring/uring_cmd.h
+++ b/io_uring/uring_cmd.h
@@ -2,11 +2,10 @@
 
 #include <linux/io_uring/cmd.h>
 #include <linux/io_uring_types.h>
 
 struct io_async_cmd {
-	struct io_uring_cmd_data	data;
 	struct iou_vec			vec;
 	struct io_uring_sqe		sqes[2];
 };
 
 int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags);
-- 
2.45.2

Re: [PATCH 0/4] io_uring/btrfs: remove struct io_uring_cmd_data

[David Sterba]

On Thu, Jun 19, 2025 at 01:27:44PM -0600, Caleb Sander Mateos wrote:
> btrfs's ->uring_cmd() implementations are the only ones using io_uring_cmd_data
> to store data that lasts for the lifetime of the uring_cmd. But all uring_cmds
> have to pay the memory and CPU cost of initializing this field and freeing the
> pointer if necessary when the uring_cmd ends. There is already a pdu field in
> struct io_uring_cmd that ->uring_cmd() implementations can use for storage. The
> only benefit of op_data seems to be that io_uring initializes it, so
> ->uring_cmd() can read it to tell if there was a previous call to ->uring_cmd().
> 
> Introduce a flag IORING_URING_CMD_REISSUE that ->uring_cmd() implementations can
> use to tell if this is the first call to ->uring_cmd() or a reissue of the
> uring_cmd. Switch btrfs to use the pdu storage for its btrfs_uring_encoded_data.
> If IORING_URING_CMD_REISSUE is unset, allocate a new btrfs_uring_encoded_data.
> If it's set, use the existing one in op_data. Free the btrfs_uring_encoded_data
> in the btrfs layer instead of relying on io_uring to free op_data. Finally,
> remove io_uring_cmd_data since it's now unused.
> 
> Caleb Sander Mateos (4):
>   btrfs/ioctl: don't skip accounting in early ENOTTY return
>   io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag
>   btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd
>   io_uring/cmd: remove struct io_uring_cmd_data

The first patch is a fix so it can be put to a -rc queue.

The rest change the io_uring logic, so it's not up to me, but regarding
how to merge them either via btrfs or io_uring tree work for me.

Re: [PATCH 0/4] io_uring/btrfs: remove struct io_uring_cmd_data

[David Sterba]

On Fri, Jun 20, 2025 at 05:47:43PM +0200, David Sterba wrote:
> On Thu, Jun 19, 2025 at 01:27:44PM -0600, Caleb Sander Mateos wrote:
> > btrfs's ->uring_cmd() implementations are the only ones using io_uring_cmd_data
> > to store data that lasts for the lifetime of the uring_cmd. But all uring_cmds
> > have to pay the memory and CPU cost of initializing this field and freeing the
> > pointer if necessary when the uring_cmd ends. There is already a pdu field in
> > struct io_uring_cmd that ->uring_cmd() implementations can use for storage. The
> > only benefit of op_data seems to be that io_uring initializes it, so
> > ->uring_cmd() can read it to tell if there was a previous call to ->uring_cmd().
> > 
> > Introduce a flag IORING_URING_CMD_REISSUE that ->uring_cmd() implementations can
> > use to tell if this is the first call to ->uring_cmd() or a reissue of the
> > uring_cmd. Switch btrfs to use the pdu storage for its btrfs_uring_encoded_data.
> > If IORING_URING_CMD_REISSUE is unset, allocate a new btrfs_uring_encoded_data.
> > If it's set, use the existing one in op_data. Free the btrfs_uring_encoded_data
> > in the btrfs layer instead of relying on io_uring to free op_data. Finally,
> > remove io_uring_cmd_data since it's now unused.
> > 
> > Caleb Sander Mateos (4):
> >   btrfs/ioctl: don't skip accounting in early ENOTTY return
> >   io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag
> >   btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd
> >   io_uring/cmd: remove struct io_uring_cmd_data
> 
> The first patch is a fix so it can be put to a -rc queue.

I've picked the first patch to for-next.

> The rest change the io_uring logic, so it's not up to me, but regarding
> how to merge them either via btrfs or io_uring tree work for me.

This is still pending. I can take it but need an ack.

Re: [PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Jens Axboe]

On 6/19/25 1:27 PM, Caleb Sander Mateos wrote:
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 929cad6ee326..7cddc4c1c554 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -257,10 +257,12 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>  			req->iopoll_start = ktime_get_ns();
>  		}
>  	}
>  
>  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
> +	if (ret == -EAGAIN)
> +		ioucmd->flags |= IORING_URING_CMD_REISSUE;
>  	if (ret == -EAGAIN || ret == -EIOCBQUEUED)
>  		return ret;

Probably fold that under the next statement?

	if (ret == -EAGAIN || ret == -EIOCBQUEUED) {
		if (ret == -EAGAIN) {
			ioucmd->flags |= IORING_URING_CMD_REISSUE;
		return ret;
	}

?

-- 
Jens Axboe

Re: [PATCH 3/4] btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd

[Jens Axboe]

> @@ -4811,11 +4813,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>  	loff_t pos;
>  	struct kiocb kiocb;
>  	struct extent_state *cached_state = NULL;
>  	u64 start, lockend;
>  	void __user *sqe_addr;
> -	struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
> +	struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
> +	struct btrfs_uring_encoded_data *data = NULL;
> +
> +	if (cmd->flags & IORING_URING_CMD_REISSUE)
> +		data = bc->data;

Can this be a btrfs io_btrfs_cmd specific flag? Doesn't seem like it
would need to be io_uring wide.

-- 
Jens Axboe

Re: [PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Daniel Vacek]

On Tue, 1 Jul 2025 at 21:04, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 6/19/25 1:27 PM, Caleb Sander Mateos wrote:
> > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > index 929cad6ee326..7cddc4c1c554 100644
> > --- a/io_uring/uring_cmd.c
> > +++ b/io_uring/uring_cmd.c
> > @@ -257,10 +257,12 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> >                       req->iopoll_start = ktime_get_ns();
> >               }
> >       }
> >
> >       ret = file->f_op->uring_cmd(ioucmd, issue_flags);
> > +     if (ret == -EAGAIN)
> > +             ioucmd->flags |= IORING_URING_CMD_REISSUE;
> >       if (ret == -EAGAIN || ret == -EIOCBQUEUED)
> >               return ret;
>
> Probably fold that under the next statement?
>
>         if (ret == -EAGAIN || ret == -EIOCBQUEUED) {
>                 if (ret == -EAGAIN) {
>                         ioucmd->flags |= IORING_URING_CMD_REISSUE;
>                 return ret;
>         }
>
> ?

I'd argue the original looks simpler, cleaner.

> --
> Jens Axboe
>

Re: [PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Ammar Faizi]

On 7/2/25 1:27 PM, Daniel Vacek wrote:
> On Tue, 1 Jul 2025 at 21:04, Jens Axboe <axboe@kernel.dk> wrote:
>> Probably fold that under the next statement?
>>
>>          if (ret == -EAGAIN || ret == -EIOCBQUEUED) {
>>                  if (ret == -EAGAIN) {
>>                          ioucmd->flags |= IORING_URING_CMD_REISSUE;
>>                  return ret;
>>          }
>>
>> ?
> 
> I'd argue the original looks simpler, cleaner.

I propose doing it this way:

	if (ret == -EAGAIN) {
		ioucmd->flags |= IORING_URING_CMD_REISSUE;
		return ret;
	}

	if (ret == -EIOCBQUEUED)
		return ret;

It's simpler because the -EAGAIN is only checked once :)

-- 
Ammar Faizi

Re: [PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Alan Huang]

On Jul 2, 2025, at 14:44, Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
> 
> On 7/2/25 1:27 PM, Daniel Vacek wrote:
>> On Tue, 1 Jul 2025 at 21:04, Jens Axboe <axboe@kernel.dk> wrote:
>>> Probably fold that under the next statement?
>>> 
>>>         if (ret == -EAGAIN || ret == -EIOCBQUEUED) {
>>>                 if (ret == -EAGAIN) {
>>>                         ioucmd->flags |= IORING_URING_CMD_REISSUE;
>>>                 return ret;
>>>         }
>>> 
>>> ?
>> I'd argue the original looks simpler, cleaner.
> 
> I propose doing it this way:
> 
> if (ret == -EAGAIN) {
> ioucmd->flags |= IORING_URING_CMD_REISSUE;
> return ret;
> }
> 
> if (ret == -EIOCBQUEUED)
> return ret;
> 
> It's simpler because the -EAGAIN is only checked once :)


Agreed


> 
> -- 
> Ammar Faizi
> 

Re: [PATCH 2/4] io_uring/cmd: introduce IORING_URING_CMD_REISSUE flag

[Jens Axboe]

On 7/2/25 12:44 AM, Ammar Faizi wrote:
> On 7/2/25 1:27 PM, Daniel Vacek wrote:
>> On Tue, 1 Jul 2025 at 21:04, Jens Axboe <axboe@kernel.dk> wrote:
>>> Probably fold that under the next statement?
>>>
>>>          if (ret == -EAGAIN || ret == -EIOCBQUEUED) {
>>>                  if (ret == -EAGAIN) {
>>>                          ioucmd->flags |= IORING_URING_CMD_REISSUE;
>>>                  return ret;
>>>          }
>>>
>>> ?
>>
>> I'd argue the original looks simpler, cleaner.
> 
> I propose doing it this way:
> 
>     if (ret == -EAGAIN) {
>         ioucmd->flags |= IORING_URING_CMD_REISSUE;
>         return ret;
>     }
> 
>     if (ret == -EIOCBQUEUED)
>         return ret;
> 
> It's simpler because the -EAGAIN is only checked once :)

Mine was mostly done for code generation reasons, though probably
the compiler is smart enough. I did consider yours as well, it's
more readable. However I'd then write it as:

if (ret == -EAGAIN) {
	ioucmd->flags |= IORING_URING_CMD_REISSUE;
	return -EAGAIN;
} else if (ret == -EIOCBQUEUED) {
	return -EIOCBQUEUED;
}

But we're obviously nitpicking now. The bigger question as posed in
another patch in this series is whether we need IORING_URING_CMD_REISSUE
at all in the first place.

-- 
Jens Axboe

Re: [PATCH 3/4] btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd

[Caleb Sander Mateos]

On Tue, Jul 1, 2025 at 3:06 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> > @@ -4811,11 +4813,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
> >       loff_t pos;
> >       struct kiocb kiocb;
> >       struct extent_state *cached_state = NULL;
> >       u64 start, lockend;
> >       void __user *sqe_addr;
> > -     struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
> > +     struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
> > +     struct btrfs_uring_encoded_data *data = NULL;
> > +
> > +     if (cmd->flags & IORING_URING_CMD_REISSUE)
> > +             data = bc->data;
>
> Can this be a btrfs io_btrfs_cmd specific flag? Doesn't seem like it
> would need to be io_uring wide.

Maybe. But where are you thinking it would be stored? I don't think
io_uring_cmd's pdu field would work because it's not initialized
before the first call to ->uring_cmd(). That's the whole reason I
needed to add a flag to tell whether this was the first call to
->uring_cmd() or a subsequent one.
I also put the flag in the uring_cmd layer because that's where
op_data was defined. Even though btrfs is the only current user of
op_data, it seems like it was intended as a generic mechanism that
other ->uring_cmd() implementations might want to use. It seems like
the same argument would apply to this flag.
Thoughts?

Best,
Caleb

Re: [PATCH 3/4] btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd

[Jens Axboe]

On 7/2/25 1:51 PM, Caleb Sander Mateos wrote:
> On Tue, Jul 1, 2025 at 3:06?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>>> @@ -4811,11 +4813,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>>>       loff_t pos;
>>>       struct kiocb kiocb;
>>>       struct extent_state *cached_state = NULL;
>>>       u64 start, lockend;
>>>       void __user *sqe_addr;
>>> -     struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
>>> +     struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
>>> +     struct btrfs_uring_encoded_data *data = NULL;
>>> +
>>> +     if (cmd->flags & IORING_URING_CMD_REISSUE)
>>> +             data = bc->data;
>>
>> Can this be a btrfs io_btrfs_cmd specific flag? Doesn't seem like it
>> would need to be io_uring wide.
> 
> Maybe. But where are you thinking it would be stored? I don't think
> io_uring_cmd's pdu field would work because it's not initialized
> before the first call to ->uring_cmd(). That's the whole reason I
> needed to add a flag to tell whether this was the first call to
> ->uring_cmd() or a subsequent one.
> I also put the flag in the uring_cmd layer because that's where
> op_data was defined. Even though btrfs is the only current user of
> op_data, it seems like it was intended as a generic mechanism that
> other ->uring_cmd() implementations might want to use. It seems like
> the same argument would apply to this flag.
> Thoughts?

It's probably fine as-is, it was just some quick reading of it.

I'd like to stage this up so we can get it done for 6.17. Can you
respind with the other minor comments addressed? And then we can attempt
to work this out with the btrfs side.

-- 
Jens Axboe

Re: [PATCH 3/4] btrfs/ioctl: store btrfs_uring_encoded_data in io_btrfs_cmd

[Caleb Sander Mateos]

On Tue, Jul 8, 2025 at 2:17 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/2/25 1:51 PM, Caleb Sander Mateos wrote:
> > On Tue, Jul 1, 2025 at 3:06?PM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >>> @@ -4811,11 +4813,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
> >>>       loff_t pos;
> >>>       struct kiocb kiocb;
> >>>       struct extent_state *cached_state = NULL;
> >>>       u64 start, lockend;
> >>>       void __user *sqe_addr;
> >>> -     struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
> >>> +     struct io_btrfs_cmd *bc = io_uring_cmd_to_pdu(cmd, struct io_btrfs_cmd);
> >>> +     struct btrfs_uring_encoded_data *data = NULL;
> >>> +
> >>> +     if (cmd->flags & IORING_URING_CMD_REISSUE)
> >>> +             data = bc->data;
> >>
> >> Can this be a btrfs io_btrfs_cmd specific flag? Doesn't seem like it
> >> would need to be io_uring wide.
> >
> > Maybe. But where are you thinking it would be stored? I don't think
> > io_uring_cmd's pdu field would work because it's not initialized
> > before the first call to ->uring_cmd(). That's the whole reason I
> > needed to add a flag to tell whether this was the first call to
> > ->uring_cmd() or a subsequent one.
> > I also put the flag in the uring_cmd layer because that's where
> > op_data was defined. Even though btrfs is the only current user of
> > op_data, it seems like it was intended as a generic mechanism that
> > other ->uring_cmd() implementations might want to use. It seems like
> > the same argument would apply to this flag.
> > Thoughts?
>
> It's probably fine as-is, it was just some quick reading of it.
>
> I'd like to stage this up so we can get it done for 6.17. Can you
> respind with the other minor comments addressed? And then we can attempt
> to work this out with the btrfs side.

Sure, I can definitely incorporate the refactoring suggestion. Will
try to resend the patch series today.

Best,
Caleb