io_uring req flags cleanups

io_uring req flags cleanups

[Christoph Hellwig]

Hi Jens,

while looking at the NOWAIT flag handling I found various bits of code
related to it pretty convoluted and confusing.  This series tries to
clean them up, let me know what you think.

Diffstat:
 cancel.c    |    5 +----
 filetable.c |   11 ++++-------
 filetable.h |   28 +++++++++++++++++-----------
 io_uring.c  |   41 ++++++++++-------------------------------
 io_uring.h  |    5 -----
 msg_ring.c  |    4 +---
 rsrc.c      |    8 ++++----
 rw.c        |    4 ++--
 8 files changed, 39 insertions(+), 67 deletions(-)

[PATCH 1/8] io_uring: remove __io_file_supports_nowait

[Christoph Hellwig]

Now that this only checks O_NONBLOCK and FMODE_NOWAIT, the helper is
complete overkilļ, and the comments are confusing bordering to wrong.
Just inline the check into the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/io_uring.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index f181876e415b9a..7e735724940f7f 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1766,19 +1766,6 @@ static void io_iopoll_req_issued(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
-/*
- * If we tracked the file through the SCM inflight mechanism, we could support
- * any file. For now, just ensure that anything potentially problematic is done
- * inline.
- */
-static bool __io_file_supports_nowait(struct file *file, umode_t mode)
-{
-	/* any ->read/write should understand O_NONBLOCK */
-	if (file->f_flags & O_NONBLOCK)
-		return true;
-	return file->f_mode & FMODE_NOWAIT;
-}
-
 /*
  * If we tracked the file through the SCM inflight mechanism, we could support
  * any file. For now, just ensure that anything potentially problematic is done
@@ -1791,7 +1778,7 @@ unsigned int io_file_get_flags(struct file *file)
 
 	if (S_ISREG(mode))
 		res |= FFS_ISREG;
-	if (__io_file_supports_nowait(file, mode))
+	if ((file->f_flags & O_NONBLOCK) || (file->f_mode & FMODE_NOWAIT))
 		res |= FFS_NOWAIT;
 	return res;
 }
-- 
2.39.2

[PATCH 2/8] io_uring: remove the mode variable in io_file_get_flags

[Christoph Hellwig]

The variable is only once now, so don't bother with it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/io_uring.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7e735724940f7f..2d13f636de93c7 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1773,10 +1773,9 @@ static void io_iopoll_req_issued(struct io_kiocb *req, unsigned int issue_flags)
  */
 unsigned int io_file_get_flags(struct file *file)
 {
-	umode_t mode = file_inode(file)->i_mode;
 	unsigned int res = 0;
 
-	if (S_ISREG(mode))
+	if (S_ISREG(file_inode(file)->i_mode))
 		res |= FFS_ISREG;
 	if ((file->f_flags & O_NONBLOCK) || (file->f_mode & FMODE_NOWAIT))
 		res |= FFS_NOWAIT;
-- 
2.39.2

[PATCH 3/8] io_uring: remove a confusing comment above io_file_get_flags

[Christoph Hellwig]

The SCM inflight mechanism has nothing to do with the fact that a file
might be a regular file or not and if it supports non-blocking
operations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/io_uring.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2d13f636de93c7..79f3cabec5b934 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1766,11 +1766,6 @@ static void io_iopoll_req_issued(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
-/*
- * If we tracked the file through the SCM inflight mechanism, we could support
- * any file. For now, just ensure that anything potentially problematic is done
- * inline.
- */
 unsigned int io_file_get_flags(struct file *file)
 {
 	unsigned int res = 0;
-- 
2.39.2

[PATCH 4/8] io_uring: remove io_req_ffs_set

[Christoph Hellwig]

Just checking the flag directly makes it a lot more obvious what is
going on here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/io_uring.c | 2 +-
 io_uring/io_uring.h | 5 -----
 io_uring/rw.c       | 2 +-
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 79f3cabec5b934..0e0bdb6ac9a202 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -424,7 +424,7 @@ static void io_prep_async_work(struct io_kiocb *req)
 	if (req->flags & REQ_F_FORCE_ASYNC)
 		req->work.flags |= IO_WQ_WORK_CONCURRENT;
 
-	if (req->file && !io_req_ffs_set(req))
+	if (req->file && !(req->flags & REQ_F_FIXED_FILE))
 		req->flags |= io_file_get_flags(req->file) << REQ_F_SUPPORT_NOWAIT_BIT;
 
 	if (req->file && (req->flags & REQ_F_ISREG)) {
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index a937b4b75aee98..9718897133db59 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -57,11 +57,6 @@ struct file *io_file_get_normal(struct io_kiocb *req, int fd);
 struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
 			       unsigned issue_flags);
 
-static inline bool io_req_ffs_set(struct io_kiocb *req)
-{
-	return req->flags & REQ_F_FIXED_FILE;
-}
-
 void __io_req_task_work_add(struct io_kiocb *req, unsigned flags);
 bool io_is_uring_fops(struct file *file);
 bool io_alloc_async_data(struct io_kiocb *req);
diff --git a/io_uring/rw.c b/io_uring/rw.c
index c23d8baf028769..1cf5742f2ae9cb 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -666,7 +666,7 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode)
 	if (unlikely(!file || !(file->f_mode & mode)))
 		return -EBADF;
 
-	if (!io_req_ffs_set(req))
+	if (!(req->flags & REQ_F_FIXED_FILE))
 		req->flags |= io_file_get_flags(file) << REQ_F_SUPPORT_NOWAIT_BIT;
 
 	kiocb->ki_flags = file->f_iocb_flags;
-- 
2.39.2

[PATCH 5/8] io_uring: return REQ_F_ flags from io_file_get_flags

[Christoph Hellwig]

Two of the three callers want them, so return the more usual format,
and shift into the FFS_ form only for the fixed file table.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/filetable.h | 6 ++----
 io_uring/io_uring.c  | 6 +++---
 io_uring/rw.c        | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/io_uring/filetable.h b/io_uring/filetable.h
index 351111ff888274..697cb68adc8169 100644
--- a/io_uring/filetable.h
+++ b/io_uring/filetable.h
@@ -54,10 +54,8 @@ static inline struct file *io_file_from_index(struct io_file_table *table,
 static inline void io_fixed_file_set(struct io_fixed_file *file_slot,
 				     struct file *file)
 {
-	unsigned long file_ptr = (unsigned long) file;
-
-	file_ptr |= io_file_get_flags(file);
-	file_slot->file_ptr = file_ptr;
+	file_slot->file_ptr = (unsigned long)file |
+		(io_file_get_flags(file) >> REQ_F_SUPPORT_NOWAIT_BIT);
 }
 
 static inline void io_reset_alloc_hint(struct io_ring_ctx *ctx)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 0e0bdb6ac9a202..1f348753694bfe 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -425,7 +425,7 @@ static void io_prep_async_work(struct io_kiocb *req)
 		req->work.flags |= IO_WQ_WORK_CONCURRENT;
 
 	if (req->file && !(req->flags & REQ_F_FIXED_FILE))
-		req->flags |= io_file_get_flags(req->file) << REQ_F_SUPPORT_NOWAIT_BIT;
+		req->flags |= io_file_get_flags(req->file);
 
 	if (req->file && (req->flags & REQ_F_ISREG)) {
 		bool should_hash = def->hash_reg_file;
@@ -1771,9 +1771,9 @@ unsigned int io_file_get_flags(struct file *file)
 	unsigned int res = 0;
 
 	if (S_ISREG(file_inode(file)->i_mode))
-		res |= FFS_ISREG;
+		res |= REQ_F_ISREG;
 	if ((file->f_flags & O_NONBLOCK) || (file->f_mode & FMODE_NOWAIT))
-		res |= FFS_NOWAIT;
+		res |= REQ_F_SUPPORT_NOWAIT;
 	return res;
 }
 
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 1cf5742f2ae9cb..1bce2208b65c4f 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -667,7 +667,7 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode)
 		return -EBADF;
 
 	if (!(req->flags & REQ_F_FIXED_FILE))
-		req->flags |= io_file_get_flags(file) << REQ_F_SUPPORT_NOWAIT_BIT;
+		req->flags |= io_file_get_flags(file);
 
 	kiocb->ki_flags = file->f_iocb_flags;
 	ret = kiocb_set_rw_flags(kiocb, rw->flags);
-- 
2.39.2

[PATCH 6/8] io_uring: use io_file_from_index in __io_sync_cancel

[Christoph Hellwig]

Use io_file_from_index instead of open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/cancel.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/io_uring/cancel.c b/io_uring/cancel.c
index b4f5dfacc0c31d..58c46c852bdd9a 100644
--- a/io_uring/cancel.c
+++ b/io_uring/cancel.c
@@ -216,13 +216,10 @@ static int __io_sync_cancel(struct io_uring_task *tctx,
 	/* fixed must be grabbed every time since we drop the uring_lock */
 	if ((cd->flags & IORING_ASYNC_CANCEL_FD) &&
 	    (cd->flags & IORING_ASYNC_CANCEL_FD_FIXED)) {
-		unsigned long file_ptr;
-
 		if (unlikely(fd >= ctx->nr_user_files))
 			return -EBADF;
 		fd = array_index_nospec(fd, ctx->nr_user_files);
-		file_ptr = io_fixed_file_slot(&ctx->file_table, fd)->file_ptr;
-		cd->file = (struct file *) (file_ptr & FFS_MASK);
+		cd->file = io_file_from_index(&ctx->file_table, fd);
 		if (!cd->file)
 			return -EBADF;
 	}
-- 
2.39.2

[PATCH 7/8] io_uring: use io_file_from_index in io_msg_grab_file

[Christoph Hellwig]

Use io_file_from_index instead of open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/msg_ring.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index 85fd7ce5f05b85..cd6dcf634ba3cd 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -162,14 +162,12 @@ static struct file *io_msg_grab_file(struct io_kiocb *req, unsigned int issue_fl
 	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
 	struct io_ring_ctx *ctx = req->ctx;
 	struct file *file = NULL;
-	unsigned long file_ptr;
 	int idx = msg->src_fd;
 
 	io_ring_submit_lock(ctx, issue_flags);
 	if (likely(idx < ctx->nr_user_files)) {
 		idx = array_index_nospec(idx, ctx->nr_user_files);
-		file_ptr = io_fixed_file_slot(&ctx->file_table, idx)->file_ptr;
-		file = (struct file *) (file_ptr & FFS_MASK);
+		file = io_file_from_index(&ctx->file_table, idx);
 		if (file)
 			get_file(file);
 	}
-- 
2.39.2

[PATCH 8/8] io_uring: add helpers to decode the fixed file file_ptr

[Christoph Hellwig]

Remove all the open coded magic on slot->file_ptr by introducing two
helpers that return the file pointer and the flags instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 io_uring/filetable.c | 11 ++++-------
 io_uring/filetable.h | 22 +++++++++++++++-------
 io_uring/io_uring.c  | 10 ++++------
 io_uring/rsrc.c      |  8 ++++----
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/io_uring/filetable.c b/io_uring/filetable.c
index 0f6fa791a47de6..e7d749991de426 100644
--- a/io_uring/filetable.c
+++ b/io_uring/filetable.c
@@ -78,10 +78,8 @@ static int io_install_fixed_file(struct io_ring_ctx *ctx, struct file *file,
 	file_slot = io_fixed_file_slot(&ctx->file_table, slot_index);
 
 	if (file_slot->file_ptr) {
-		struct file *old_file;
-
-		old_file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-		ret = io_queue_rsrc_removal(ctx->file_data, slot_index, old_file);
+		ret = io_queue_rsrc_removal(ctx->file_data, slot_index,
+					    io_slot_file(file_slot));
 		if (ret)
 			return ret;
 
@@ -140,7 +138,6 @@ int io_fixed_fd_install(struct io_kiocb *req, unsigned int issue_flags,
 int io_fixed_fd_remove(struct io_ring_ctx *ctx, unsigned int offset)
 {
 	struct io_fixed_file *file_slot;
-	struct file *file;
 	int ret;
 
 	if (unlikely(!ctx->file_data))
@@ -153,8 +150,8 @@ int io_fixed_fd_remove(struct io_ring_ctx *ctx, unsigned int offset)
 	if (!file_slot->file_ptr)
 		return -EBADF;
 
-	file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-	ret = io_queue_rsrc_removal(ctx->file_data, offset, file);
+	ret = io_queue_rsrc_removal(ctx->file_data, offset,
+				    io_slot_file(file_slot));
 	if (ret)
 		return ret;
 
diff --git a/io_uring/filetable.h b/io_uring/filetable.h
index 697cb68adc8169..b47adf170c314d 100644
--- a/io_uring/filetable.h
+++ b/io_uring/filetable.h
@@ -5,10 +5,6 @@
 #include <linux/file.h>
 #include <linux/io_uring_types.h>
 
-#define FFS_NOWAIT		0x1UL
-#define FFS_ISREG		0x2UL
-#define FFS_MASK		~(FFS_NOWAIT|FFS_ISREG)
-
 bool io_alloc_file_tables(struct io_file_table *table, unsigned nr_files);
 void io_free_file_tables(struct io_file_table *table);
 
@@ -43,12 +39,24 @@ io_fixed_file_slot(struct io_file_table *table, unsigned i)
 	return &table->files[i];
 }
 
+#define FFS_NOWAIT		0x1UL
+#define FFS_ISREG		0x2UL
+#define FFS_MASK		~(FFS_NOWAIT|FFS_ISREG)
+
+static inline unsigned int io_slot_flags(struct io_fixed_file *slot)
+{
+	return (slot->file_ptr & ~FFS_MASK) << REQ_F_SUPPORT_NOWAIT_BIT;
+}
+
+static inline struct file *io_slot_file(struct io_fixed_file *slot)
+{
+	return (struct file *)(slot->file_ptr & FFS_MASK);
+}
+
 static inline struct file *io_file_from_index(struct io_file_table *table,
 					      int index)
 {
-	struct io_fixed_file *slot = io_fixed_file_slot(table, index);
-
-	return (struct file *) (slot->file_ptr & FFS_MASK);
+	return io_slot_file(io_fixed_file_slot(table, index));
 }
 
 static inline void io_fixed_file_set(struct io_fixed_file *file_slot,
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 1f348753694bfe..ae4cb3c4e73034 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2028,19 +2028,17 @@ inline struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
 				      unsigned int issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	struct io_fixed_file *slot;
 	struct file *file = NULL;
-	unsigned long file_ptr;
 
 	io_ring_submit_lock(ctx, issue_flags);
 
 	if (unlikely((unsigned int)fd >= ctx->nr_user_files))
 		goto out;
 	fd = array_index_nospec(fd, ctx->nr_user_files);
-	file_ptr = io_fixed_file_slot(&ctx->file_table, fd)->file_ptr;
-	file = (struct file *) (file_ptr & FFS_MASK);
-	file_ptr &= ~FFS_MASK;
-	/* mask in overlapping REQ_F and FFS bits */
-	req->flags |= (file_ptr << REQ_F_SUPPORT_NOWAIT_BIT);
+	slot = io_fixed_file_slot(&ctx->file_table, fd);
+	file = io_slot_file(slot);
+	req->flags |= io_slot_flags(slot);
 	io_req_set_rsrc_node(req, ctx, 0);
 out:
 	io_ring_submit_unlock(ctx, issue_flags);
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index d46f72a5ef7323..a2dce7ef3a7877 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -354,7 +354,6 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 	__s32 __user *fds = u64_to_user_ptr(up->data);
 	struct io_rsrc_data *data = ctx->file_data;
 	struct io_fixed_file *file_slot;
-	struct file *file;
 	int fd, i, err = 0;
 	unsigned int done;
 
@@ -382,15 +381,16 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 		file_slot = io_fixed_file_slot(&ctx->file_table, i);
 
 		if (file_slot->file_ptr) {
-			file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-			err = io_queue_rsrc_removal(data, i, file);
+			err = io_queue_rsrc_removal(data, i,
+						    io_slot_file(file_slot));
 			if (err)
 				break;
 			file_slot->file_ptr = 0;
 			io_file_bitmap_clear(&ctx->file_table, i);
 		}
 		if (fd != -1) {
-			file = fget(fd);
+			struct file *file = fget(fd);
+
 			if (!file) {
 				err = -EBADF;
 				break;
-- 
2.39.2

Re: io_uring req flags cleanups

[Jens Axboe]

On Tue, 20 Jun 2023 13:32:27 +0200, Christoph Hellwig wrote:
> while looking at the NOWAIT flag handling I found various bits of code
> related to it pretty convoluted and confusing.  This series tries to
> clean them up, let me know what you think.
> 
> Diffstat:
>  cancel.c    |    5 +----
>  filetable.c |   11 ++++-------
>  filetable.h |   28 +++++++++++++++++-----------
>  io_uring.c  |   41 ++++++++++-------------------------------
>  io_uring.h  |    5 -----
>  msg_ring.c  |    4 +---
>  rsrc.c      |    8 ++++----
>  rw.c        |    4 ++--
>  8 files changed, 39 insertions(+), 67 deletions(-)
> 
> [...]

Applied, thanks!

[1/8] io_uring: remove __io_file_supports_nowait
      commit: b9a6c9459a5aec7bfd9b763554d15148367f1806
[2/8] io_uring: remove the mode variable in io_file_get_flags
      commit: 53cfd5cea7f36bac7f3d45de4fea77e0c8d57aee
[3/8] io_uring: remove a confusing comment above io_file_get_flags
      commit: b57c7cd1c17616ae9db5614525ba703f384afd05
[4/8] io_uring: remove io_req_ffs_set
      commit: 3beed235d1a1d0a4ab093ab67ea6b2841e9d4fa2
[5/8] io_uring: return REQ_F_ flags from io_file_get_flags
      commit: 8487f083c6ff6e02b2ec14f22ef2b0079a1b6425
[6/8] io_uring: use io_file_from_index in __io_sync_cancel
      commit: 60a666f097a8d722a3907925d21e363add289c8c
[7/8] io_uring: use io_file_from_index in io_msg_grab_file
      commit: f432c8c8c12b84c5465b1ffddb6feb7d6b19c1ca
[8/8] io_uring: add helpers to decode the fixed file file_ptr
      commit: 4bfb0c9af832a182a54e549123a634e0070c8d4f

Best regards,
-- 
Jens Axboe