[PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Pavel Begunkov]

It seems no opcode may return -EAGAIN for non-blocking case and expect
to be reissued. Remove retry code from io_wq_submit_work().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b9dd94143c30..b2ce8a3d3dd1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4622,26 +4622,14 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
 	struct io_wq_work *work = *workptr;
 	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
 	struct io_kiocb *nxt = NULL;
-	int ret = 0;
+	int ret;
 
 	/* if NO_CANCEL is set, we must still run the work */
 	if ((work->flags & (IO_WQ_WORK_CANCEL|IO_WQ_WORK_NO_CANCEL)) ==
 				IO_WQ_WORK_CANCEL) {
 		ret = -ECANCELED;
-	}
-
-	if (!ret) {
-		do {
-			ret = io_issue_sqe(req, NULL, &nxt, false);
-			/*
-			 * We can get EAGAIN for polled IO even though we're
-			 * forcing a sync submission from here, since we can't
-			 * wait for request slots on the block side.
-			 */
-			if (ret != -EAGAIN)
-				break;
-			cond_resched();
-		} while (1);
+	} else {
+		ret = io_issue_sqe(req, NULL, &nxt, false);
 	}
 
 	/* drop submission reference */
-- 
2.24.0

Re: [PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Jens Axboe]

On 2/24/20 2:15 AM, Pavel Begunkov wrote:
> It seems no opcode may return -EAGAIN for non-blocking case and expect
> to be reissued. Remove retry code from io_wq_submit_work().

There's actually a comment right there on how that's possible :-)

Normally, for block IO, we can wait for request slots if we run out.
For polled IO, that isn't possible since the task itself is the one
that will find completions and hence free request slots as well. If
the submitting task is allowed to sleep waiting for requests that it
itself are supposed to find and complete, then we'd hang. Hence we
return -EAGAIN for that case, and have no other choice for polled
IO than to retry.

-- 
Jens Axboe

Re: [PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 1073 bytes --]

On 24/02/2020 18:27, Jens Axboe wrote:
> On 2/24/20 2:15 AM, Pavel Begunkov wrote:
>> It seems no opcode may return -EAGAIN for non-blocking case and expect
>> to be reissued. Remove retry code from io_wq_submit_work().
> 
> There's actually a comment right there on how that's possible :-)

Yeah, I saw it and understand the motive, and how it may happen, but can't
find a line, which can actually return -EAGAIN. Could you please point to an
example?

E.g. I suppose for io_read() it may happen in call_read_iter(), but its result
(i.e. res2) will be written to cqe, but not returned.
> Normally, for block IO, we can wait for request slots if we run out.
> For polled IO, that isn't possible since the task itself is the one
> that will find completions and hence free request slots as well. If
> the submitting task is allowed to sleep waiting for requests that it
> itself are supposed to find and complete, then we'd hang. Hence we
> return -EAGAIN for that case, and have no other choice for polled
> IO than to retry.


-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Jens Axboe]

On 2/24/20 8:40 AM, Pavel Begunkov wrote:
> On 24/02/2020 18:27, Jens Axboe wrote:
>> On 2/24/20 2:15 AM, Pavel Begunkov wrote:
>>> It seems no opcode may return -EAGAIN for non-blocking case and expect
>>> to be reissued. Remove retry code from io_wq_submit_work().
>>
>> There's actually a comment right there on how that's possible :-)
> 
> Yeah, I saw it and understand the motive, and how it may happen, but can't
> find a line, which can actually return -EAGAIN. Could you please point to an
> example?

Just give it a whirl, should be easy to reproduce if you just do:

# echo 2 > /sys/block/nvme0n1/queue/nr_requests
# fio/t/io_uring /dev/nvme0n1

or something like that. It's propagated from the kiocb endio handler,
through, req->result at the bottom of io_issue_sqe()

	if (ctx->flags & IORING_SETUP_IOPOLL) {
		const bool in_async = io_wq_current_is_worker();

		if (req->result == -EAGAIN)                                     
			return -EAGAIN;
	[...]


-- 
Jens Axboe

Re: [PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 1144 bytes --]

On 24/02/2020 21:16, Jens Axboe wrote:
> On 2/24/20 8:40 AM, Pavel Begunkov wrote:
>> On 24/02/2020 18:27, Jens Axboe wrote:
>>> On 2/24/20 2:15 AM, Pavel Begunkov wrote:
>>>> It seems no opcode may return -EAGAIN for non-blocking case and expect
>>>> to be reissued. Remove retry code from io_wq_submit_work().
>>>
>>> There's actually a comment right there on how that's possible :-)
>>
>> Yeah, I saw it and understand the motive, and how it may happen, but can't
>> find a line, which can actually return -EAGAIN. Could you please point to an
>> example?
> 
> Just give it a whirl, should be easy to reproduce if you just do:
> 
> # echo 2 > /sys/block/nvme0n1/queue/nr_requests
> # fio/t/io_uring /dev/nvme0n1
> 
> or something like that. It's propagated from the kiocb endio handler,
> through, req->result at the bottom of io_issue_sqe()

I see now, thanks! What a jungle

> 
> 	if (ctx->flags & IORING_SETUP_IOPOLL) {
> 		const bool in_async = io_wq_current_is_worker();
> 
> 		if (req->result == -EAGAIN)                                     
> 			return -EAGAIN;
> 	[...]
> 
> 

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] io_uring: remove retries from io_wq_submit_work()

[Jens Axboe]

On 2/24/20 11:33 AM, Pavel Begunkov wrote:
> On 24/02/2020 21:16, Jens Axboe wrote:
>> On 2/24/20 8:40 AM, Pavel Begunkov wrote:
>>> On 24/02/2020 18:27, Jens Axboe wrote:
>>>> On 2/24/20 2:15 AM, Pavel Begunkov wrote:
>>>>> It seems no opcode may return -EAGAIN for non-blocking case and expect
>>>>> to be reissued. Remove retry code from io_wq_submit_work().
>>>>
>>>> There's actually a comment right there on how that's possible :-)
>>>
>>> Yeah, I saw it and understand the motive, and how it may happen, but can't
>>> find a line, which can actually return -EAGAIN. Could you please point to an
>>> example?
>>
>> Just give it a whirl, should be easy to reproduce if you just do:
>>
>> # echo 2 > /sys/block/nvme0n1/queue/nr_requests
>> # fio/t/io_uring /dev/nvme0n1
>>
>> or something like that. It's propagated from the kiocb endio handler,
>> through, req->result at the bottom of io_issue_sqe()
> 
> I see now, thanks! What a jungle

The problem is, as you alluded to, that errors are returned through the
callback. Not sure if you recall, but I did make an attempt at doing
inline -EAGAIN returns, but had to abort and never got around to fixing
it up. See the revert here:

commit 7b6620d7db566a46f49b4b9deab9fa061fd4b59b
Author: Jens Axboe <axboe@kernel.dk>
Date:   Thu Aug 15 11:09:16 2019 -0600

    block: remove REQ_NOWAIT_INLINE

Without that, we can't do better than what we do now. With the inline
NOWAIT, we'd get the same treatment on polled and non-polled, and we
could kill that check. Which would be lovely...

-- 
Jens Axboe