[PATCHSET for-next 0/2] Make pipe honor IOCB_NOWAIT

[PATCHSET for-next 0/2] Make pipe honor IOCB_NOWAIT

[Jens Axboe]

Hi,

File types that implement read_iter/write_iter should check for
IOCB_NOWAIT in conjunction with O_NONBLOCK, so it can correctly bail
with -EAGAIN if we need to block for space/data. pipe doesn't currently
do that, and that's the primary reason for why io_uring needs to use
a slower path for it.

Add the appropriate check, and expand the io_uring "understands nonblock"
check so that we catch it.

-- 
Jens Axboe

[PATCH 1/2] pipe: honor iocb IOCB_NOWAIT flag as well

[Jens Axboe]

It's not enough to just check the file O_NONBLOCK flag, we should also
check if the iocb being passed in has been flagged as non-blocking as
well.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/pipe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 42c7ff41c2db..58fee8816564 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -342,7 +342,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		if (ret)
 			break;
-		if (filp->f_flags & O_NONBLOCK) {
+		if (filp->f_flags & O_NONBLOCK || iocb->ki_flags & IOCB_NOWAIT) {
 			ret = -EAGAIN;
 			break;
 		}
@@ -547,7 +547,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 			continue;
 
 		/* Wait for buffer space to become available. */
-		if (filp->f_flags & O_NONBLOCK) {
+		if (filp->f_flags & O_NONBLOCK || iocb->ki_flags & IOCB_NOWAIT) {
 			if (!ret)
 				ret = -EAGAIN;
 			break;
-- 
2.39.2

[PATCH 2/2] io_uring: assume read_iter/write_iter are safe for nonblocking

[Jens Axboe]

All read_iter/write_iter must check IOCB_NOWAIT like they check for
O_NONBLOCK, and return -EAGAIN if we need to be sleeping.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/filetable.h |  4 ++--
 io_uring/io_uring.c  | 13 +++++++++----
 io_uring/rw.c        |  2 +-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/io_uring/filetable.h b/io_uring/filetable.h
index 351111ff8882..e221b5b9134f 100644
--- a/io_uring/filetable.h
+++ b/io_uring/filetable.h
@@ -21,7 +21,7 @@ int io_fixed_fd_remove(struct io_ring_ctx *ctx, unsigned int offset);
 int io_register_file_alloc_range(struct io_ring_ctx *ctx,
 				 struct io_uring_file_index_range __user *arg);
 
-unsigned int io_file_get_flags(struct file *file);
+unsigned int io_file_get_flags(struct file *file, fmode_t fmode);
 
 static inline void io_file_bitmap_clear(struct io_file_table *table, int bit)
 {
@@ -56,7 +56,7 @@ static inline void io_fixed_file_set(struct io_fixed_file *file_slot,
 {
 	unsigned long file_ptr = (unsigned long) file;
 
-	file_ptr |= io_file_get_flags(file);
+	file_ptr |= io_file_get_flags(file, file->f_mode);
 	file_slot->file_ptr = file_ptr;
 }
 
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index fd1cc35a1c00..1592faec41e2 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -422,7 +422,7 @@ static void io_prep_async_work(struct io_kiocb *req)
 		req->work.flags |= IO_WQ_WORK_CONCURRENT;
 
 	if (req->file && !io_req_ffs_set(req))
-		req->flags |= io_file_get_flags(req->file) << REQ_F_SUPPORT_NOWAIT_BIT;
+		req->flags |= io_file_get_flags(req->file, 0) << REQ_F_SUPPORT_NOWAIT_BIT;
 
 	if (req->flags & REQ_F_ISREG) {
 		if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
@@ -1719,7 +1719,8 @@ static bool io_bdev_nowait(struct block_device *bdev)
  * 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)
+static bool __io_file_supports_nowait(struct file *file, umode_t mode,
+				      fmode_t fmode)
 {
 	if (S_ISBLK(mode)) {
 		if (IS_ENABLED(CONFIG_BLOCK) &&
@@ -1740,6 +1741,10 @@ 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;
+	if (fmode & FMODE_READ && file->f_op->read_iter)
+		return true;
+	if (fmode & FMODE_WRITE && file->f_op->write_iter)
+		return true;
 	return file->f_mode & FMODE_NOWAIT;
 }
 
@@ -1748,14 +1753,14 @@ static bool __io_file_supports_nowait(struct file *file, umode_t mode)
  * any file. For now, just ensure that anything potentially problematic is done
  * inline.
  */
-unsigned int io_file_get_flags(struct file *file)
+unsigned int io_file_get_flags(struct file *file, fmode_t fmode)
 {
 	umode_t mode = file_inode(file)->i_mode;
 	unsigned int res = 0;
 
 	if (S_ISREG(mode))
 		res |= FFS_ISREG;
-	if (__io_file_supports_nowait(file, mode))
+	if (__io_file_supports_nowait(file, mode, fmode))
 		res |= FFS_NOWAIT;
 	return res;
 }
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 4c233910e200..33c87eb061d2 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -668,7 +668,7 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode)
 		return -EBADF;
 
 	if (!io_req_ffs_set(req))
-		req->flags |= io_file_get_flags(file) << REQ_F_SUPPORT_NOWAIT_BIT;
+		req->flags |= io_file_get_flags(file, mode) << REQ_F_SUPPORT_NOWAIT_BIT;
 
 	kiocb->ki_flags = file->f_iocb_flags;
 	ret = kiocb_set_rw_flags(kiocb, rw->flags);
-- 
2.39.2

Re: [PATCHSET for-next 0/2] Make pipe honor IOCB_NOWAIT

[Dave Chinner]

On Tue, Mar 07, 2023 at 08:45:31AM -0700, Jens Axboe wrote:
> Hi,
> 
> File types that implement read_iter/write_iter should check for
> IOCB_NOWAIT

Since when? If so, what's the point of setting FMODE_NOWAIT when the
struct file is opened to indicate the file has comprehensive
IOCB_NOWAIT support in the underlying IO path?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHSET for-next 0/2] Make pipe honor IOCB_NOWAIT

[Jens Axboe]

On 3/7/23 5:19 PM, Dave Chinner wrote:
> On Tue, Mar 07, 2023 at 08:45:31AM -0700, Jens Axboe wrote:
>> Hi,
>>
>> File types that implement read_iter/write_iter should check for
>> IOCB_NOWAIT
> 
> Since when? If so, what's the point of setting FMODE_NOWAIT when the
> struct file is opened to indicate the file has comprehensive
> IOCB_NOWAIT support in the underlying IO path?

Guess I missed that FMODE_NOWAIT is supposed to be added for that,
my naive assumption was that the iter based one should check. Which
is a bad sad, but at least there's a flag for it.

But the good news is that I can drop the io_uring patch, just need
to revise the pipe patch. I'll send a v2.

-- 
Jens Axboe