[PATCH] io_uring: fix blocking inline submission

[PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

There is a complaint against sys_io_uring_enter() blocking if it submits
stdin reads. The problem is in __io_file_supports_async(), which
sees that it's a cdev and allows it to be processed inline.

Punt char devices using generic rules of io_file_supports_async(),
including checking for presence of *_iter() versions of rw callbacks.
Apparently, it will affect most of cdevs with some exceptions like
null and zero devices.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---

"...For now, just ensure that anything potentially problematic is done
inline". I believe this part is outdated, but what use cases we miss?
Anything that we care about?

IMHO the best option is to do like in this patch and add
(read,write)_iter(), to places we care about.

/dev/[u]random, consoles, any else?

 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 42380ed563c4..44d1859f0dfb 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2616,7 +2616,7 @@ static bool __io_file_supports_async(struct file *file, int rw)
 			return true;
 		return false;
 	}
-	if (S_ISCHR(mode) || S_ISSOCK(mode))
+	if (S_ISSOCK(mode))
 		return true;
 	if (S_ISREG(mode)) {
 		if (IS_ENABLED(CONFIG_BLOCK) &&
-- 
2.31.1

Re: [PATCH] io_uring: fix blocking inline submission

[Hao Xu]

在 2021/6/9 下午7:07, Pavel Begunkov 写道:
> There is a complaint against sys_io_uring_enter() blocking if it submits
> stdin reads. The problem is in __io_file_supports_async(), which
> sees that it's a cdev and allows it to be processed inline.
> 
> Punt char devices using generic rules of io_file_supports_async(),
> including checking for presence of *_iter() versions of rw callbacks.
> Apparently, it will affect most of cdevs with some exceptions like
> null and zero devices.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> 
> "...For now, just ensure that anything potentially problematic is done
> inline". I believe this part is outdated, but what use cases we miss?
> Anything that we care about?
> 
> IMHO the best option is to do like in this patch and add
> (read,write)_iter(), to places we care about.
> 
> /dev/[u]random, consoles, any else?
> 
This reminds me another thing, once I did nowait read on a brd(block
ramdisk), I saw a 10%~30% regression after __io_file_supports_async()
added. brd is bio based device (block layer doesn't support nowait IO
for this kind of device), so theoretically it makes sense to punt it to
iowq threads in advance in __io_file_supports_async(), but actually
what originally happen is: IOCB_NOWAIT is not delivered to block
layer(REQ_NOWAIT) and then the IO request is executed inline (It seems
brd device won't block). This finally makes 'check it in advance'
slower..
>   fs/io_uring.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 42380ed563c4..44d1859f0dfb 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2616,7 +2616,7 @@ static bool __io_file_supports_async(struct file *file, int rw)
>   			return true;
>   		return false;
>   	}
> -	if (S_ISCHR(mode) || S_ISSOCK(mode))
> +	if (S_ISSOCK(mode))
>   		return true;
>   	if (S_ISREG(mode)) {
>   		if (IS_ENABLED(CONFIG_BLOCK) &&
> 

Re: [PATCH] io_uring: fix blocking inline submission

[Jens Axboe]

On 6/9/21 5:07 AM, Pavel Begunkov wrote:
> There is a complaint against sys_io_uring_enter() blocking if it submits
> stdin reads. The problem is in __io_file_supports_async(), which
> sees that it's a cdev and allows it to be processed inline.
> 
> Punt char devices using generic rules of io_file_supports_async(),
> including checking for presence of *_iter() versions of rw callbacks.
> Apparently, it will affect most of cdevs with some exceptions like
> null and zero devices.

I don't like this, we really should fix the file types, they are
broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
the write equiv).

For cases where there is no iter variant of the read/write handlers,
then yes we should not return true from __io_file_supports_async().

-- 
Jens Axboe

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 4:07 PM, Jens Axboe wrote:
> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>> There is a complaint against sys_io_uring_enter() blocking if it submits
>> stdin reads. The problem is in __io_file_supports_async(), which
>> sees that it's a cdev and allows it to be processed inline.
>>
>> Punt char devices using generic rules of io_file_supports_async(),
>> including checking for presence of *_iter() versions of rw callbacks.
>> Apparently, it will affect most of cdevs with some exceptions like
>> null and zero devices.
> 
> I don't like this, we really should fix the file types, they are
> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
> the write equiv).
> 
> For cases where there is no iter variant of the read/write handlers,
> then yes we should not return true from __io_file_supports_async().

I'm confused. The patch doesn't punt them unconditionally, but make
it go through the generic path of __io_file_supports_async()
including checks for read_iter/write_iter. So if a chrdev has
*_iter() it should continue to work as before.

It fixes the symptom that means the change punts it async, and so
I assume tty doesn't have _iter()s for some reason. Will take a
look at the tty driver soon to stop blind guessing.

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix blocking inline submission

[Jens Axboe]

On 6/9/21 9:34 AM, Pavel Begunkov wrote:
> On 6/9/21 4:07 PM, Jens Axboe wrote:
>> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>>> There is a complaint against sys_io_uring_enter() blocking if it submits
>>> stdin reads. The problem is in __io_file_supports_async(), which
>>> sees that it's a cdev and allows it to be processed inline.
>>>
>>> Punt char devices using generic rules of io_file_supports_async(),
>>> including checking for presence of *_iter() versions of rw callbacks.
>>> Apparently, it will affect most of cdevs with some exceptions like
>>> null and zero devices.
>>
>> I don't like this, we really should fix the file types, they are
>> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
>> the write equiv).
>>
>> For cases where there is no iter variant of the read/write handlers,
>> then yes we should not return true from __io_file_supports_async().
> 
> I'm confused. The patch doesn't punt them unconditionally, but make
> it go through the generic path of __io_file_supports_async()
> including checks for read_iter/write_iter. So if a chrdev has
> *_iter() it should continue to work as before.

Ah ok, yes then that is indeed fine.

> It fixes the symptom that means the change punts it async, and so
> I assume tty doesn't have _iter()s for some reason. Will take a
> look at the tty driver soon to stop blind guessing.

I think they do, but they don't honor IOCB_NOWAIT for example. I'd
be curious if the patch actually fixes the reported case, even though
it is most likely the right thing to do. If not, then the fops handler
need fixing for that driver.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 12:07 PM, Pavel Begunkov wrote:
> There is a complaint against sys_io_uring_enter() blocking if it submits
> stdin reads. The problem is in __io_file_supports_async(), which
> sees that it's a cdev and allows it to be processed inline.
> 
> Punt char devices using generic rules of io_file_supports_async(),
> including checking for presence of *_iter() versions of rw callbacks.
> Apparently, it will affect most of cdevs with some exceptions like
> null and zero devices.

Reported-by: Birk Hirdman <lonjil@gmail.com>

 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> 
> "...For now, just ensure that anything potentially problematic is done
> inline". I believe this part is outdated, but what use cases we miss?
> Anything that we care about?
> 
> IMHO the best option is to do like in this patch and add
> (read,write)_iter(), to places we care about.
> 
> /dev/[u]random, consoles, any else?
> 
>  fs/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 42380ed563c4..44d1859f0dfb 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2616,7 +2616,7 @@ static bool __io_file_supports_async(struct file *file, int rw)
>  			return true;
>  		return false;
>  	}
> -	if (S_ISCHR(mode) || S_ISSOCK(mode))
> +	if (S_ISSOCK(mode))
>  		return true;
>  	if (S_ISREG(mode)) {
>  		if (IS_ENABLED(CONFIG_BLOCK) &&
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 4:36 PM, Jens Axboe wrote:
> On 6/9/21 9:34 AM, Pavel Begunkov wrote:
>> On 6/9/21 4:07 PM, Jens Axboe wrote:
>>> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>>>> There is a complaint against sys_io_uring_enter() blocking if it submits
>>>> stdin reads. The problem is in __io_file_supports_async(), which
>>>> sees that it's a cdev and allows it to be processed inline.
>>>>
>>>> Punt char devices using generic rules of io_file_supports_async(),
>>>> including checking for presence of *_iter() versions of rw callbacks.
>>>> Apparently, it will affect most of cdevs with some exceptions like
>>>> null and zero devices.
>>>
>>> I don't like this, we really should fix the file types, they are
>>> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
>>> the write equiv).
>>>
>>> For cases where there is no iter variant of the read/write handlers,
>>> then yes we should not return true from __io_file_supports_async().
>>
>> I'm confused. The patch doesn't punt them unconditionally, but make
>> it go through the generic path of __io_file_supports_async()
>> including checks for read_iter/write_iter. So if a chrdev has
>> *_iter() it should continue to work as before.
> 
> Ah ok, yes then that is indeed fine.
> 
>> It fixes the symptom that means the change punts it async, and so
>> I assume tty doesn't have _iter()s for some reason. Will take a
>> look at the tty driver soon to stop blind guessing.
> 
> I think they do, but they don't honor IOCB_NOWAIT for example. I'd
> be curious if the patch actually fixes the reported case, even though
> it is most likely the right thing to do. If not, then the fops handler
> need fixing for that driver.

Yep, weird, but fixes it for me. A simple repro was attached
to the issue.

https://github.com/axboe/liburing/issues/354

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix blocking inline submission

[Jens Axboe]

On 6/9/21 9:41 AM, Pavel Begunkov wrote:
> On 6/9/21 4:36 PM, Jens Axboe wrote:
>> On 6/9/21 9:34 AM, Pavel Begunkov wrote:
>>> On 6/9/21 4:07 PM, Jens Axboe wrote:
>>>> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>>>>> There is a complaint against sys_io_uring_enter() blocking if it submits
>>>>> stdin reads. The problem is in __io_file_supports_async(), which
>>>>> sees that it's a cdev and allows it to be processed inline.
>>>>>
>>>>> Punt char devices using generic rules of io_file_supports_async(),
>>>>> including checking for presence of *_iter() versions of rw callbacks.
>>>>> Apparently, it will affect most of cdevs with some exceptions like
>>>>> null and zero devices.
>>>>
>>>> I don't like this, we really should fix the file types, they are
>>>> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
>>>> the write equiv).
>>>>
>>>> For cases where there is no iter variant of the read/write handlers,
>>>> then yes we should not return true from __io_file_supports_async().
>>>
>>> I'm confused. The patch doesn't punt them unconditionally, but make
>>> it go through the generic path of __io_file_supports_async()
>>> including checks for read_iter/write_iter. So if a chrdev has
>>> *_iter() it should continue to work as before.
>>
>> Ah ok, yes then that is indeed fine.
>>
>>> It fixes the symptom that means the change punts it async, and so
>>> I assume tty doesn't have _iter()s for some reason. Will take a
>>> look at the tty driver soon to stop blind guessing.
>>
>> I think they do, but they don't honor IOCB_NOWAIT for example. I'd
>> be curious if the patch actually fixes the reported case, even though
>> it is most likely the right thing to do. If not, then the fops handler
>> need fixing for that driver.
> 
> Yep, weird, but fixes it for me. A simple repro was attached
> to the issue.
> 
> https://github.com/axboe/liburing/issues/354

Ah ok, all good then for now. I have applied the patch, and added
the reported-by as well.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 4:43 PM, Jens Axboe wrote:
> On 6/9/21 9:41 AM, Pavel Begunkov wrote:
>> On 6/9/21 4:36 PM, Jens Axboe wrote:
>>> On 6/9/21 9:34 AM, Pavel Begunkov wrote:
>>>> On 6/9/21 4:07 PM, Jens Axboe wrote:
>>>>> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>>>>>> There is a complaint against sys_io_uring_enter() blocking if it submits
>>>>>> stdin reads. The problem is in __io_file_supports_async(), which
>>>>>> sees that it's a cdev and allows it to be processed inline.
>>>>>>
>>>>>> Punt char devices using generic rules of io_file_supports_async(),
>>>>>> including checking for presence of *_iter() versions of rw callbacks.
>>>>>> Apparently, it will affect most of cdevs with some exceptions like
>>>>>> null and zero devices.
>>>>>
>>>>> I don't like this, we really should fix the file types, they are
>>>>> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
>>>>> the write equiv).
>>>>>
>>>>> For cases where there is no iter variant of the read/write handlers,
>>>>> then yes we should not return true from __io_file_supports_async().
>>>>
>>>> I'm confused. The patch doesn't punt them unconditionally, but make
>>>> it go through the generic path of __io_file_supports_async()
>>>> including checks for read_iter/write_iter. So if a chrdev has
>>>> *_iter() it should continue to work as before.
>>>
>>> Ah ok, yes then that is indeed fine.
>>>
>>>> It fixes the symptom that means the change punts it async, and so
>>>> I assume tty doesn't have _iter()s for some reason. Will take a
>>>> look at the tty driver soon to stop blind guessing.
>>>
>>> I think they do, but they don't honor IOCB_NOWAIT for example. I'd
>>> be curious if the patch actually fixes the reported case, even though
>>> it is most likely the right thing to do. If not, then the fops handler
>>> need fixing for that driver.
>>
>> Yep, weird, but fixes it for me. A simple repro was attached
>> to the issue.
>>
>> https://github.com/axboe/liburing/issues/354
> 
> Ah ok, all good then for now. I have applied the patch, and added
> the reported-by as well.

Great, thanks. Will check what's with the tty part

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 4:47 PM, Pavel Begunkov wrote:
> On 6/9/21 4:43 PM, Jens Axboe wrote:
>> On 6/9/21 9:41 AM, Pavel Begunkov wrote:
>>> On 6/9/21 4:36 PM, Jens Axboe wrote:
>>>> On 6/9/21 9:34 AM, Pavel Begunkov wrote:
>>>>> On 6/9/21 4:07 PM, Jens Axboe wrote:
>>>>>> On 6/9/21 5:07 AM, Pavel Begunkov wrote:
>>>>>>> There is a complaint against sys_io_uring_enter() blocking if it submits
>>>>>>> stdin reads. The problem is in __io_file_supports_async(), which
>>>>>>> sees that it's a cdev and allows it to be processed inline.
>>>>>>>
>>>>>>> Punt char devices using generic rules of io_file_supports_async(),
>>>>>>> including checking for presence of *_iter() versions of rw callbacks.
>>>>>>> Apparently, it will affect most of cdevs with some exceptions like
>>>>>>> null and zero devices.
>>>>>>
>>>>>> I don't like this, we really should fix the file types, they are
>>>>>> broken if they don't honor IOCB_NOWAIT and have ->read_iter() (or
>>>>>> the write equiv).
>>>>>>
>>>>>> For cases where there is no iter variant of the read/write handlers,
>>>>>> then yes we should not return true from __io_file_supports_async().
>>>>>
>>>>> I'm confused. The patch doesn't punt them unconditionally, but make
>>>>> it go through the generic path of __io_file_supports_async()
>>>>> including checks for read_iter/write_iter. So if a chrdev has
>>>>> *_iter() it should continue to work as before.
>>>>
>>>> Ah ok, yes then that is indeed fine.
>>>>
>>>>> It fixes the symptom that means the change punts it async, and so
>>>>> I assume tty doesn't have _iter()s for some reason. Will take a
>>>>> look at the tty driver soon to stop blind guessing.
>>>>
>>>> I think they do, but they don't honor IOCB_NOWAIT for example. I'd
>>>> be curious if the patch actually fixes the reported case, even though
>>>> it is most likely the right thing to do. If not, then the fops handler
>>>> need fixing for that driver.
>>>
>>> Yep, weird, but fixes it for me. A simple repro was attached
>>> to the issue.
>>>
>>> https://github.com/axboe/liburing/issues/354
>>
>> Ah ok, all good then for now. I have applied the patch, and added
>> the reported-by as well.
> 
> Great, thanks. Will check what's with the tty part

Apparently, they have *_iter but reasonably don't set FMODE_NOWAIT.
Similar goes for /dev/null and others, either they don't have
*_iter or don't set FMODE_NOWAIT and ignore IOCB_NOWAIT.

Bottom line, we need to tinker cdevs that we're interested to
be efficient.

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix blocking inline submission

[Pavel Begunkov]

On 6/9/21 1:42 PM, Hao Xu wrote:
> 在 2021/6/9 下午7:07, Pavel Begunkov 写道:
>> There is a complaint against sys_io_uring_enter() blocking if it submits
>> stdin reads. The problem is in __io_file_supports_async(), which
>> sees that it's a cdev and allows it to be processed inline.
>>
>> Punt char devices using generic rules of io_file_supports_async(),
>> including checking for presence of *_iter() versions of rw callbacks.
>> Apparently, it will affect most of cdevs with some exceptions like
>> null and zero devices.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>
>> "...For now, just ensure that anything potentially problematic is done
>> inline". I believe this part is outdated, but what use cases we miss?
>> Anything that we care about?
>>
>> IMHO the best option is to do like in this patch and add
>> (read,write)_iter(), to places we care about.
>>
>> /dev/[u]random, consoles, any else?
>>
> This reminds me another thing, once I did nowait read on a brd(block
> ramdisk), I saw a 10%~30% regression after __io_file_supports_async()
> added. brd is bio based device (block layer doesn't support nowait IO
> for this kind of device), so theoretically it makes sense to punt it to
> iowq threads in advance in __io_file_supports_async(), but actually
> what originally happen is: IOCB_NOWAIT is not delivered to block
> layer(REQ_NOWAIT) and then the IO request is executed inline (It seems

IIUC we fixed it was fixed by

f8b78caf21d5bc3fcfc40c18898f9d52ed1451a5 ("block: don't ignore REQ_NOWAIT for direct IO")

> brd device won't block). This finally makes 'check it in advance'
> slower..

Trying to understand what kind of slower it is... It makes an attempt
to execute it inline. Does it return -EAGAIN? Always succeed
submission (e.g. queued for truly async execution ki_complete style)?

>>   fs/io_uring.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 42380ed563c4..44d1859f0dfb 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -2616,7 +2616,7 @@ static bool __io_file_supports_async(struct file *file, int rw)
>>               return true;
>>           return false;
>>       }
>> -    if (S_ISCHR(mode) || S_ISSOCK(mode))
>> +    if (S_ISSOCK(mode))
>>           return true;
>>       if (S_ISREG(mode)) {
>>           if (IS_ENABLED(CONFIG_BLOCK) &&
>>
> 

-- 
Pavel Begunkov