[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