[PATCHSET 0/3] Improve FMODE_NOWAIT coverage

[PATCHSET 0/3] Improve FMODE_NOWAIT coverage

[Jens Axboe]

Hi,

io_uring jumps through some hoops to check block devices and sockets
for sanity wrt nonblocking read/write attempts. We can get rid of that
if we just flag sockets and block devices as being sane in that regard.
Patches 1-2 do that for sockets and block devices, and then patch 3 can
remove some cruft on the io_uring side that is pretty ugly.

-- 
Jens Axboe

[PATCH 1/3] net: set FMODE_NOWAIT for sockets

[Jens Axboe]

The socket read/write functions deal with O_NONBLOCK and IOCB_NOWAIT
just fine, so we can flag them as being FMODE_NOWAIT compliant. With
this, we can remove socket special casing in io_uring when checking
if a file type is sane for nonblocking IO, and it's also the defined
way to flag file types as such in the kernel.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index a7b4b37d86df..6861dbbfadb6 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -471,6 +471,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 		return file;
 	}
 
+	file->f_mode |= FMODE_NOWAIT;
 	sock->file = file;
 	file->private_data = sock;
 	stream_open(SOCK_INODE(sock), file);
-- 
2.39.2

[PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Jens Axboe]

We set this unconditionally, but it really should be dependent on if
the underlying device is nowait compliant.

Cc: linux-block@vger.kernel.org
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/fops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/fops.c b/block/fops.c
index d2e6be4e3d1c..ab750e8a040f 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -481,7 +481,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	 * during an unstable branch.
 	 */
 	filp->f_flags |= O_LARGEFILE;
-	filp->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC;
+	filp->f_mode |= FMODE_BUF_RASYNC;
 
 	if (filp->f_flags & O_NDELAY)
 		filp->f_mode |= FMODE_NDELAY;
@@ -494,6 +494,9 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
+	if (bdev_nowait(bdev))
+		filp->f_mode |= FMODE_NOWAIT;
+
 	filp->private_data = bdev;
 	filp->f_mapping = bdev->bd_inode->i_mapping;
 	filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
-- 
2.39.2

[PATCH 3/3] io_uring: rely solely on FMODE_NOWAIT

[Jens Axboe]

Now that we have both sockets and block devices setting FMODE_NOWAIT
appropriately, we can get rid of all the odd special casing in
__io_file_supports_nowait() and rely soley on FMODE_NOWAIT and
O_NONBLOCK rather than special case sockets and (in particular) bdevs.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/io_uring.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 3bca7a79efda..7c426584e35a 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1758,11 +1758,6 @@ static void io_iopoll_req_issued(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
-static bool io_bdev_nowait(struct block_device *bdev)
-{
-	return !bdev || bdev_nowait(bdev);
-}
-
 /*
  * 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
@@ -1770,22 +1765,6 @@ static bool io_bdev_nowait(struct block_device *bdev)
  */
 static bool __io_file_supports_nowait(struct file *file, umode_t mode)
 {
-	if (S_ISBLK(mode)) {
-		if (IS_ENABLED(CONFIG_BLOCK) &&
-		    io_bdev_nowait(I_BDEV(file->f_mapping->host)))
-			return true;
-		return false;
-	}
-	if (S_ISSOCK(mode))
-		return true;
-	if (S_ISREG(mode)) {
-		if (IS_ENABLED(CONFIG_BLOCK) &&
-		    io_bdev_nowait(file->f_inode->i_sb->s_bdev) &&
-		    !io_is_uring_fops(file))
-			return true;
-		return false;
-	}
-
 	/* any ->read/write should understand O_NONBLOCK */
 	if (file->f_flags & O_NONBLOCK)
 		return true;
-- 
2.39.2

Re: [PATCH 3/3] io_uring: rely solely on FMODE_NOWAIT

[Linus Torvalds]

On Tue, May 9, 2023 at 8:19 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> Now that we have both sockets and block devices setting FMODE_NOWAIT
> appropriately, we can get rid of all the odd special casing in
> __io_file_supports_nowait() and rely soley on FMODE_NOWAIT and
> O_NONBLOCK rather than special case sockets and (in particular) bdevs.

Yup, looks good to me. Thanks.

               Linus

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Christoph Hellwig]

On Tue, May 09, 2023 at 09:19:09AM -0600, Jens Axboe wrote:
> We set this unconditionally, but it really should be dependent on if
> the underlying device is nowait compliant.

Somehow I only see patch 2 of 3 of whatever series this is supposed to
be in my linux-block mbox, something is broken with your patch sending
script.

The change itself looks fine even standalone, though:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Jens Axboe]

On 5/10/23 7:30 AM, Christoph Hellwig wrote:
> On Tue, May 09, 2023 at 09:19:09AM -0600, Jens Axboe wrote:
>> We set this unconditionally, but it really should be dependent on if
>> the underlying device is nowait compliant.
> 
> Somehow I only see patch 2 of 3 of whatever series this is supposed to
> be in my linux-block mbox, something is broken with your patch sending
> script.
> 
> The change itself looks fine even standalone, though:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks - the 2/3 only is on purpose, 1/3 is a networking ditto and
3/3 is just io_uring now being able to delete some code. So this one
was supposed to be able to stand on its own, should've had the cover
letter for everyone obviously though.

-- 
Jens Axboe

Re: [PATCH 1/3] net: set FMODE_NOWAIT for sockets

[Paolo Abeni]

On Tue, 2023-05-09 at 09:19 -0600, Jens Axboe wrote:
> The socket read/write functions deal with O_NONBLOCK and IOCB_NOWAIT
> just fine, so we can flag them as being FMODE_NOWAIT compliant. With
> this, we can remove socket special casing in io_uring when checking
> if a file type is sane for nonblocking IO, and it's also the defined
> way to flag file types as such in the kernel.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index a7b4b37d86df..6861dbbfadb6 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -471,6 +471,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
>  		return file;
>  	}
>  
> +	file->f_mode |= FMODE_NOWAIT;
>  	sock->file = file;
>  	file->private_data = sock;
>  	stream_open(SOCK_INODE(sock), file);

The patch looks sane to me:

Reviewed-by: Paolo Abeni <pabeni@redhat.com>

I understand the intention is merging patch via the io_uring tree? If
so, no objections on my side: hopefully it should not cause any
conflicts with the netdev tree.

Thanks,

Paolo

Re: [PATCH 1/3] net: set FMODE_NOWAIT for sockets

[Jens Axboe]

On 5/11/23 2:03?AM, Paolo Abeni wrote:
> On Tue, 2023-05-09 at 09:19 -0600, Jens Axboe wrote:
>> The socket read/write functions deal with O_NONBLOCK and IOCB_NOWAIT
>> just fine, so we can flag them as being FMODE_NOWAIT compliant. With
>> this, we can remove socket special casing in io_uring when checking
>> if a file type is sane for nonblocking IO, and it's also the defined
>> way to flag file types as such in the kernel.
>>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  net/socket.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index a7b4b37d86df..6861dbbfadb6 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -471,6 +471,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
>>  		return file;
>>  	}
>>  
>> +	file->f_mode |= FMODE_NOWAIT;
>>  	sock->file = file;
>>  	file->private_data = sock;
>>  	stream_open(SOCK_INODE(sock), file);
> 
> The patch looks sane to me:
> 
> Reviewed-by: Paolo Abeni <pabeni@redhat.com>
> 
> I understand the intention is merging patch via the io_uring tree? If
> so, no objections on my side: hopefully it should not cause any
> conflicts with the netdev tree.

If it's fine with you guys, then yeah that would make my life easier.
Risk of conflicts should be very low, and trivial if it does occur.
Thanks for the review!

-- 
Jens Axboe

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Christoph Hellwig]

So it turns out this gets into the way of my planned cleanup to move
all the tatic FMODE_ flags out of this basically full field into a new
static one in file_operations.  Do you think it is ok to go back to
always claiming FMODE_NOWAIT for block devices and then just punt for
the drivers that don't support it like the patch below?

---
From 05a591ac066d9d2d57c4967bbd49c8bc63b04abf Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Tue, 20 Jun 2023 07:53:13 +0200
Subject: block: always set FMODE_NOWAIT

Block devices are the only file_operation that do not set FMODE_NOWAIT
unconditionall in ->open and thus get in the way of a planned cleanup to
move this flags into a static field in file_operations.   Switch to
always set FMODE_NOWAIT and just return -EAGAIN if it isn't actually
supported.  This just affects minor ->submit_bio based drivers as all
blk-mq drivers and the important remappers (dm, md, nvme-multipath)
support nowait I/O.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/fops.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 555b1b9ecd2cb9..8068d0c85ae75b 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -505,7 +505,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	 * during an unstable branch.
 	 */
 	filp->f_flags |= O_LARGEFILE;
-	filp->f_mode |= FMODE_BUF_RASYNC;
+	filp->f_mode |= FMODE_BUF_RASYNC | FMODE_NOWAIT;
 
 	/*
 	 * Use the file private data to store the holder for exclusive openes.
@@ -519,9 +519,6 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	if (bdev_nowait(bdev))
-		filp->f_mode |= FMODE_NOWAIT;
-
 	filp->f_mapping = bdev->bd_inode->i_mapping;
 	filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
 	return 0;
@@ -563,6 +560,9 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if ((iocb->ki_flags & (IOCB_NOWAIT | IOCB_DIRECT)) == IOCB_NOWAIT)
 		return -EOPNOTSUPP;
 
+	if (!bdev_nowait(bdev))
+		return -EAGAIN;
+
 	size -= iocb->ki_pos;
 	if (iov_iter_count(from) > size) {
 		shorted = iov_iter_count(from) - size;
@@ -585,6 +585,9 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t ret = 0;
 	size_t count;
 
+	if (!bdev_nowait(bdev))
+		return -EAGAIN;
+
 	if (unlikely(pos + iov_iter_count(to) > size)) {
 		if (pos >= size)
 			return 0;
-- 
2.39.2

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Christoph Hellwig]

On Mon, Jun 19, 2023 at 11:18:55PM -0700, Christoph Hellwig wrote:
> So it turns out this gets into the way of my planned cleanup to move
> all the tatic FMODE_ flags out of this basically full field into a new
> static one in file_operations.  Do you think it is ok to go back to
> always claiming FMODE_NOWAIT for block devices and then just punt for
> the drivers that don't support it like the patch below?

Except that the version I posted if of course completly broken as my
testing rig told me.  This is the version that actually works:

---
From 7916df7434e1570978b9c81c65aa1ec1f3396b13 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Tue, 20 Jun 2023 07:53:13 +0200
Subject: block: always set FMODE_NOWAIT

Block devices are the only file_operations that do not set FMODE_NOWAIT
unconditionall in ->open and thus get in the way of a planned cleanup to
move this flags into a static field in file_operations.   Switch to
always set FMODE_NOWAIT and just return -EAGAIN if it isn't actually
supported.  This just affects minor ->submit_bio based drivers as all
blk-mq drivers and the important remappers (dm, md, nvme-multipath)
support nowait I/O.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/fops.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 555b1b9ecd2cb9..58c2f65ae4a57e 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -505,7 +505,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	 * during an unstable branch.
 	 */
 	filp->f_flags |= O_LARGEFILE;
-	filp->f_mode |= FMODE_BUF_RASYNC;
+	filp->f_mode |= FMODE_BUF_RASYNC | FMODE_NOWAIT;
 
 	/*
 	 * Use the file private data to store the holder for exclusive openes.
@@ -519,9 +519,6 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	if (bdev_nowait(bdev))
-		filp->f_mode |= FMODE_NOWAIT;
-
 	filp->f_mapping = bdev->bd_inode->i_mapping;
 	filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
 	return 0;
@@ -563,6 +560,9 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if ((iocb->ki_flags & (IOCB_NOWAIT | IOCB_DIRECT)) == IOCB_NOWAIT)
 		return -EOPNOTSUPP;
 
+	if ((iocb->ki_flags & IOCB_NOWAIT) && !bdev_nowait(bdev))
+		return -EAGAIN;
+
 	size -= iocb->ki_pos;
 	if (iov_iter_count(from) > size) {
 		shorted = iov_iter_count(from) - size;
@@ -585,6 +585,9 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t ret = 0;
 	size_t count;
 
+	if ((iocb->ki_flags & IOCB_NOWAIT) && !bdev_nowait(bdev))
+		return -EAGAIN;
+
 	if (unlikely(pos + iov_iter_count(to) > size)) {
 		if (pos >= size)
 			return 0;
-- 
2.39.2

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Jens Axboe]

On 6/20/23 12:18?AM, Christoph Hellwig wrote:
> So it turns out this gets into the way of my planned cleanup to move
> all the tatic FMODE_ flags out of this basically full field into a new
> static one in file_operations.  Do you think it is ok to go back to
> always claiming FMODE_NOWAIT for block devices and then just punt for
> the drivers that don't support it like the patch below?

I think we need stronger justification than that, it's much nicer to
have it in the open path than doing the same check over and over for
each IO.

With your new proposed scheme, why can't the check and FMODE_NOWAIT set
still be in open?

-- 
Jens Axboe

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Christoph Hellwig]

On Tue, Jun 20, 2023 at 07:24:56AM -0600, Jens Axboe wrote:
> I think we need stronger justification than that, it's much nicer to
> have it in the open path than doing the same check over and over for
> each IO.
> 
> With your new proposed scheme, why can't the check and FMODE_NOWAIT set
> still be in open?

Because I want to move the by now huge number of static flags out of
file->f_mode and into file->f_op.flags.  It's just that with this patch
this one flag isn't static anymore for block devices.  We could also
do two sets of file operations assuming we never allow run-time changes
for QUEUE_FLAG_NOWAIT.  If we care about optimizing fo async I/O on the
few drivers not supporting QUEUE_FLAG_NOWAIT that's probably the next
best thing.

Re: [PATCH 2/3] block: mark bdev files as FMODE_NOWAIT if underlying device supports it

[Jens Axboe]

On 6/20/23 7:29?AM, Christoph Hellwig wrote:
> On Tue, Jun 20, 2023 at 07:24:56AM -0600, Jens Axboe wrote:
>> I think we need stronger justification than that, it's much nicer to
>> have it in the open path than doing the same check over and over for
>> each IO.
>>
>> With your new proposed scheme, why can't the check and FMODE_NOWAIT set
>> still be in open?
> 
> Because I want to move the by now huge number of static flags out of
> file->f_mode and into file->f_op.flags.  It's just that with this patch
> this one flag isn't static anymore for block devices.  We could also
> do two sets of file operations assuming we never allow run-time changes
> for QUEUE_FLAG_NOWAIT.  If we care about optimizing fo async I/O on the
> few drivers not supporting QUEUE_FLAG_NOWAIT that's probably the next
> best thing.

Doing two sets of fops makes sense to me, and then hopefully down the
line we can shave it down to 1 again once everything sets FMODE_NOWAIT.

-- 
Jens Axboe