[PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Jens Axboe]

S_ISBLK is marked as unbounded work for async preparation, because it
doesn't match S_ISREG. That is incorrect, as any read/write to a block
device is also a bounded operation. Fix it up and ensure that S_ISBLK
isn't marked unbounded.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6d7a1b69712b..a16b7df934d1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
 	if (req->flags & REQ_F_ISREG) {
 		if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
 			io_wq_hash_work(&req->work, file_inode(req->file));
-	} else {
+	} else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
 		if (def->unbound_nonreg_file)
 			req->work.flags |= IO_WQ_WORK_UNBOUND;
 	}

-- 
Jens Axboe

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Hao Xu]

在 2021/4/1 下午10:57, Jens Axboe 写道:
> S_ISBLK is marked as unbounded work for async preparation, because it
> doesn't match S_ISREG. That is incorrect, as any read/write to a block
> device is also a bounded operation. Fix it up and ensure that S_ISBLK
> isn't marked unbounded.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
execute. What is the difference between bounded and unbounded?

Thanks,
Hao
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6d7a1b69712b..a16b7df934d1 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>   	if (req->flags & REQ_F_ISREG) {
>   		if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>   			io_wq_hash_work(&req->work, file_inode(req->file));
> -	} else {
> +	} else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>   		if (def->unbound_nonreg_file)
>   			req->work.flags |= IO_WQ_WORK_UNBOUND;
>   	}
> 

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Pavel Begunkov]

On 02/04/2021 09:52, Hao Xu wrote:
> 在 2021/4/1 下午10:57, Jens Axboe 写道:
>> S_ISBLK is marked as unbounded work for async preparation, because it
>> doesn't match S_ISREG. That is incorrect, as any read/write to a block
>> device is also a bounded operation. Fix it up and ensure that S_ISBLK
>> isn't marked unbounded.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
> Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
> execute. What is the difference between bounded and unbounded?

Unbounded works are not bounded in execution time, i.e. they may take
forever to complete. E.g. recv depends on the other end to send something,
that not necessarily will ever happen.


> 
> Thanks,
> Hao
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6d7a1b69712b..a16b7df934d1 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>>       if (req->flags & REQ_F_ISREG) {
>>           if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>>               io_wq_hash_work(&req->work, file_inode(req->file));
>> -    } else {
>> +    } else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>>           if (def->unbound_nonreg_file)
>>               req->work.flags |= IO_WQ_WORK_UNBOUND;
>>       }
>>
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Pavel Begunkov]

On 02/04/2021 11:32, Pavel Begunkov wrote:
> On 02/04/2021 09:52, Hao Xu wrote:
>> 在 2021/4/1 下午10:57, Jens Axboe 写道:
>>> S_ISBLK is marked as unbounded work for async preparation, because it
>>> doesn't match S_ISREG. That is incorrect, as any read/write to a block
>>> device is also a bounded operation. Fix it up and ensure that S_ISBLK
>>> isn't marked unbounded.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>> Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
>> execute. What is the difference between bounded and unbounded?
> 
> Unbounded works are not bounded in execution time, i.e. they may take
> forever to complete. E.g. recv depends on the other end to send something,
> that not necessarily will ever happen.

To elaborate a bit, one example of how it's used: because unbounded may
stay for long, it always spawns a new worker thread for each of them.

If app submits SQEs as below, and send's are not actually sent for execution
but stashed somewhere internally in a list, e.g. waiting for a worker thread
to get free, it would just hang from the userspace perspective.

recv(fd1), recv(fd1), send(fd1), send(fd1)


>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 6d7a1b69712b..a16b7df934d1 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>>>       if (req->flags & REQ_F_ISREG) {
>>>           if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>>>               io_wq_hash_work(&req->work, file_inode(req->file));
>>> -    } else {
>>> +    } else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>>>           if (def->unbound_nonreg_file)
>>>               req->work.flags |= IO_WQ_WORK_UNBOUND;
>>>       }

-- 
Pavel Begunkov

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Hao Xu]

在 2021/4/2 下午6:48, Pavel Begunkov 写道:
> On 02/04/2021 11:32, Pavel Begunkov wrote:
>> On 02/04/2021 09:52, Hao Xu wrote:
>>> 在 2021/4/1 下午10:57, Jens Axboe 写道:
>>>> S_ISBLK is marked as unbounded work for async preparation, because it
>>>> doesn't match S_ISREG. That is incorrect, as any read/write to a block
>>>> device is also a bounded operation. Fix it up and ensure that S_ISBLK
>>>> isn't marked unbounded.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>> Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
>>> execute. What is the difference between bounded and unbounded?
>>
>> Unbounded works are not bounded in execution time, i.e. they may take
>> forever to complete. E.g. recv depends on the other end to send something,
>> that not necessarily will ever happen.
> 
> To elaborate a bit, one example of how it's used: because unbounded may
> stay for long, it always spawns a new worker thread for each of them.
> 
> If app submits SQEs as below, and send's are not actually sent for execution
> but stashed somewhere internally in a list, e.g. waiting for a worker thread
> to get free, it would just hang from the userspace perspective.
> 
> recv(fd1), recv(fd1), send(fd1), send(fd1)
> 
Hi Pavel, thank you for the patient explanation, I got the meaning of
  bound/unbond now, but it seems there is no difference handling bounded 
and unbounded work in the current code?
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 6d7a1b69712b..a16b7df934d1 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>>>>        if (req->flags & REQ_F_ISREG) {
>>>>            if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>>>>                io_wq_hash_work(&req->work, file_inode(req->file));
>>>> -    } else {
>>>> +    } else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>>>>            if (def->unbound_nonreg_file)
>>>>                req->work.flags |= IO_WQ_WORK_UNBOUND;
>>>>        }
> 

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Pavel Begunkov]

On 04/04/2021 11:57, Hao Xu wrote:
> 在 2021/4/2 下午6:48, Pavel Begunkov 写道:
>> On 02/04/2021 11:32, Pavel Begunkov wrote:
>>> On 02/04/2021 09:52, Hao Xu wrote:
>>>> 在 2021/4/1 下午10:57, Jens Axboe 写道:
>>>>> S_ISBLK is marked as unbounded work for async preparation, because it
>>>>> doesn't match S_ISREG. That is incorrect, as any read/write to a block
>>>>> device is also a bounded operation. Fix it up and ensure that S_ISBLK
>>>>> isn't marked unbounded.
>>>>>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>
>>>> Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
>>>> execute. What is the difference between bounded and unbounded?
>>>
>>> Unbounded works are not bounded in execution time, i.e. they may take
>>> forever to complete. E.g. recv depends on the other end to send something,
>>> that not necessarily will ever happen.
>>
>> To elaborate a bit, one example of how it's used: because unbounded may
>> stay for long, it always spawns a new worker thread for each of them.
>>
>> If app submits SQEs as below, and send's are not actually sent for execution
>> but stashed somewhere internally in a list, e.g. waiting for a worker thread
>> to get free, it would just hang from the userspace perspective.
>>
>> recv(fd1), recv(fd1), send(fd1), send(fd1)
>>
> Hi Pavel, thank you for the patient explanation, I got the meaning of
>  bound/unbond now, but it seems there is no difference handling bounded and unbounded work in the current code?

It was used in io-wq, so see in io-wq.[c,h] files. Not sure about 5.12+, but
a quick look shows: 

wqe->acct[IO_WQ_ACCT_BOUND].max_workers = bounded;
...
wqe->acct[IO_WQ_ACCT_UNBOUND].max_workers =
			task_rlimit(current, RLIMIT_NPROC);



>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index 6d7a1b69712b..a16b7df934d1 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>>>>>        if (req->flags & REQ_F_ISREG) {
>>>>>            if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>>>>>                io_wq_hash_work(&req->work, file_inode(req->file));
>>>>> -    } else {
>>>>> +    } else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>>>>>            if (def->unbound_nonreg_file)
>>>>>                req->work.flags |= IO_WQ_WORK_UNBOUND;
>>>>>        }
>>
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: don't mark S_ISBLK async work as unbounded

[Hao Xu]

在 2021/4/4 下午7:02, Pavel Begunkov 写道:
> On 04/04/2021 11:57, Hao Xu wrote:
>> 在 2021/4/2 下午6:48, Pavel Begunkov 写道:
>>> On 02/04/2021 11:32, Pavel Begunkov wrote:
>>>> On 02/04/2021 09:52, Hao Xu wrote:
>>>>> 在 2021/4/1 下午10:57, Jens Axboe 写道:
>>>>>> S_ISBLK is marked as unbounded work for async preparation, because it
>>>>>> doesn't match S_ISREG. That is incorrect, as any read/write to a block
>>>>>> device is also a bounded operation. Fix it up and ensure that S_ISBLK
>>>>>> isn't marked unbounded.
>>>>>>
>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>>
>>>>> Hi Jens, I saw a (un)bounded work is for a (un)bounded worker to
>>>>> execute. What is the difference between bounded and unbounded?
>>>>
>>>> Unbounded works are not bounded in execution time, i.e. they may take
>>>> forever to complete. E.g. recv depends on the other end to send something,
>>>> that not necessarily will ever happen.
>>>
>>> To elaborate a bit, one example of how it's used: because unbounded may
>>> stay for long, it always spawns a new worker thread for each of them.
>>>
>>> If app submits SQEs as below, and send's are not actually sent for execution
>>> but stashed somewhere internally in a list, e.g. waiting for a worker thread
>>> to get free, it would just hang from the userspace perspective.
>>>
>>> recv(fd1), recv(fd1), send(fd1), send(fd1)
>>>
>> Hi Pavel, thank you for the patient explanation, I got the meaning of
>>   bound/unbond now, but it seems there is no difference handling bounded and unbounded work in the current code?
> 
> It was used in io-wq, so see in io-wq.[c,h] files. Not sure about 5.12+, but
> a quick look shows:
> 
> wqe->acct[IO_WQ_ACCT_BOUND].max_workers = bounded;
> ...
> wqe->acct[IO_WQ_ACCT_UNBOUND].max_workers =
> 			task_rlimit(current, RLIMIT_NPROC);
> 
Thanks, Pavel.:)
> 
> 
>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>> index 6d7a1b69712b..a16b7df934d1 100644
>>>>>> --- a/fs/io_uring.c
>>>>>> +++ b/fs/io_uring.c
>>>>>> @@ -1213,7 +1213,7 @@ static void io_prep_async_work(struct io_kiocb *req)
>>>>>>         if (req->flags & REQ_F_ISREG) {
>>>>>>             if (def->hash_reg_file || (ctx->flags & IORING_SETUP_IOPOLL))
>>>>>>                 io_wq_hash_work(&req->work, file_inode(req->file));
>>>>>> -    } else {
>>>>>> +    } else if (!req->file || !S_ISBLK(file_inode(req->file)->i_mode)) {
>>>>>>             if (def->unbound_nonreg_file)
>>>>>>                 req->work.flags |= IO_WQ_WORK_UNBOUND;
>>>>>>         }
>>>
>>
>