[PATCHSET 0/5] io_uring fixes for 5.9

[PATCHSET 0/5] io_uring fixes for 5.9

[Jens Axboe]

Various fixes that were found through testing and inspection. Some by
myself, others reported.

-- 
Jens Axboe

[PATCH 1/5] io_uring: grab any needed state during defer prep

[Jens Axboe]

Always grab work environment for deferred links. The assumption that we
will be running it always from the task in question is false, as exiting
tasks may mean that we're deferring this one to a thread helper. And at
that point it's too late to grab the work environment.

Fixes: debb85f496c9 ("io_uring: factor out grab_env() from defer_prep()")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 175fb647d099..be9d628e7854 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5449,6 +5449,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
 	if (unlikely(ret))
 		return ret;
 
+	io_prep_async_work(req);
+
 	switch (req->opcode) {
 	case IORING_OP_NOP:
 		break;
-- 
2.28.0

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 14/09/2020 19:25, Jens Axboe wrote:
> Always grab work environment for deferred links. The assumption that we
> will be running it always from the task in question is false, as exiting
> tasks may mean that we're deferring this one to a thread helper. And at
> that point it's too late to grab the work environment.

That's a shame. Do you mean that's from lines like below?

int io_async_buf_func()
{
    ...
    if (!io_req_task_work_add()) {
        ...
        tsk = io_wq_get_task(req->ctx->io_wq);
        task_work_add(tsk, &req->task_work, 0);
    }
}

It looks like failing such requests would be a better option.
io_uring_flush() would want them killed for PF_EXITING processes anyway.

> 
> Fixes: debb85f496c9 ("io_uring: factor out grab_env() from defer_prep()")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/io_uring.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 175fb647d099..be9d628e7854 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5449,6 +5449,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
>  	if (unlikely(ret))
>  		return ret;
>  
> +	io_prep_async_work(req);
> +
>  	switch (req->opcode) {
>  	case IORING_OP_NOP:
>  		break;
> 

-- 
Pavel Begunkov

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 19/09/2020 18:27, Pavel Begunkov wrote:
> On 14/09/2020 19:25, Jens Axboe wrote:
>> Always grab work environment for deferred links. The assumption that we
>> will be running it always from the task in question is false, as exiting
>> tasks may mean that we're deferring this one to a thread helper. And at
>> that point it's too late to grab the work environment.
> 
> That's a shame. Do you mean that's from lines like below?
> 
> int io_async_buf_func()
> {
>     ...
>     if (!io_req_task_work_add()) {
>         ...
>         tsk = io_wq_get_task(req->ctx->io_wq);
>         task_work_add(tsk, &req->task_work, 0);
>     }
> }
> 
> It looks like failing such requests would be a better option.
> io_uring_flush() would want them killed for PF_EXITING processes anyway.

Forgot that they will be cancelled there. So, how it could happen?
Is that the initial thread will run task_work but loosing
some resources like mm prior to that? e.g. in do_exit()

> 
>>
>> Fixes: debb85f496c9 ("io_uring: factor out grab_env() from defer_prep()")
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  fs/io_uring.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 175fb647d099..be9d628e7854 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5449,6 +5449,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
>>  	if (unlikely(ret))
>>  		return ret;
>>  
>> +	io_prep_async_work(req);
>> +
>>  	switch (req->opcode) {
>>  	case IORING_OP_NOP:
>>  		break;
>>
> 

-- 
Pavel Begunkov

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 19/09/2020 19:56, Pavel Begunkov wrote:
> On 19/09/2020 18:27, Pavel Begunkov wrote:
>> On 14/09/2020 19:25, Jens Axboe wrote:
>>> Always grab work environment for deferred links. The assumption that we
>>> will be running it always from the task in question is false, as exiting
>>> tasks may mean that we're deferring this one to a thread helper. And at
>>> that point it's too late to grab the work environment.
> Forgot that they will be cancelled there. So, how it could happen?
> Is that the initial thread will run task_work but loosing
> some resources like mm prior to that? e.g. in do_exit()

Jens, please let me know when you get time for that. I was thinking that
you were meaning do_exit(), which does task_work_run() after killing mm,
etc., but you mentioned a thread helper in the description... Which one
do you mean?

>
>>
>>>
>>> Fixes: debb85f496c9 ("io_uring: factor out grab_env() from defer_prep()")
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>  fs/io_uring.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 175fb647d099..be9d628e7854 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -5449,6 +5449,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
>>>  	if (unlikely(ret))
>>>  		return ret;
>>>  
>>> +	io_prep_async_work(req);
>>> +
>>>  	switch (req->opcode) {
>>>  	case IORING_OP_NOP:
>>>  		break;
>>>
>>
> 

-- 
Pavel Begunkov

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 02/10/2020 19:14, Pavel Begunkov wrote:
> On 19/09/2020 19:56, Pavel Begunkov wrote:
>> On 19/09/2020 18:27, Pavel Begunkov wrote:
>>> On 14/09/2020 19:25, Jens Axboe wrote:
>>>> Always grab work environment for deferred links. The assumption that we
>>>> will be running it always from the task in question is false, as exiting
>>>> tasks may mean that we're deferring this one to a thread helper. And at
>>>> that point it's too late to grab the work environment.
>> Forgot that they will be cancelled there. So, how it could happen?
>> Is that the initial thread will run task_work but loosing
>> some resources like mm prior to that? e.g. in do_exit()
> 
> Jens, please let me know when you get time for that. I was thinking that
> you were meaning do_exit(), which does task_work_run() after killing mm,
> etc., but you mentioned a thread helper in the description... Which one
> do you mean?

Either it refers to stuff after io_ring_ctx_wait_and_kill(), which
delegates the rest to io_ring_exit_work() via @system_unbound_wq.

> 
>>
>>>
>>>>
>>>> Fixes: debb85f496c9 ("io_uring: factor out grab_env() from defer_prep()")
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>  fs/io_uring.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 175fb647d099..be9d628e7854 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -5449,6 +5449,8 @@ static int io_req_defer_prep(struct io_kiocb *req,
>>>>  	if (unlikely(ret))
>>>>  		return ret;
>>>>  
>>>> +	io_prep_async_work(req);
>>>> +
>>>>  	switch (req->opcode) {
>>>>  	case IORING_OP_NOP:
>>>>  		break;
>>>>
>>>
>>
> 

-- 
Pavel Begunkov

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Jens Axboe]

On 10/2/20 10:34 AM, Pavel Begunkov wrote:
> On 02/10/2020 19:14, Pavel Begunkov wrote:
>> On 19/09/2020 19:56, Pavel Begunkov wrote:
>>> On 19/09/2020 18:27, Pavel Begunkov wrote:
>>>> On 14/09/2020 19:25, Jens Axboe wrote:
>>>>> Always grab work environment for deferred links. The assumption that we
>>>>> will be running it always from the task in question is false, as exiting
>>>>> tasks may mean that we're deferring this one to a thread helper. And at
>>>>> that point it's too late to grab the work environment.
>>> Forgot that they will be cancelled there. So, how it could happen?
>>> Is that the initial thread will run task_work but loosing
>>> some resources like mm prior to that? e.g. in do_exit()
>>
>> Jens, please let me know when you get time for that. I was thinking that
>> you were meaning do_exit(), which does task_work_run() after killing mm,
>> etc., but you mentioned a thread helper in the description... Which one
>> do you mean?
> 
> Either it refers to stuff after io_ring_ctx_wait_and_kill(), which
> delegates the rest to io_ring_exit_work() via @system_unbound_wq.

We punt the request to task_work. task_work is run, we're still in the
right context. We fail with -EAGAIN, and then call io_queue_async_work()
and we're not doing async prep at that point.

-- 
Jens Axboe

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 02/10/2020 20:01, Jens Axboe wrote:
> On 10/2/20 10:34 AM, Pavel Begunkov wrote:
>> On 02/10/2020 19:14, Pavel Begunkov wrote:
>>> On 19/09/2020 19:56, Pavel Begunkov wrote:
>>>> On 19/09/2020 18:27, Pavel Begunkov wrote:
>>>>> On 14/09/2020 19:25, Jens Axboe wrote:
>>>>>> Always grab work environment for deferred links. The assumption that we
>>>>>> will be running it always from the task in question is false, as exiting
>>>>>> tasks may mean that we're deferring this one to a thread helper. And at
>>>>>> that point it's too late to grab the work environment.
>>>> Forgot that they will be cancelled there. So, how it could happen?
>>>> Is that the initial thread will run task_work but loosing
>>>> some resources like mm prior to that? e.g. in do_exit()
>>>
>>> Jens, please let me know when you get time for that. I was thinking that
>>> you were meaning do_exit(), which does task_work_run() after killing mm,
>>> etc., but you mentioned a thread helper in the description... Which one
>>> do you mean?
>>
>> Either it refers to stuff after io_ring_ctx_wait_and_kill(), which
>> delegates the rest to io_ring_exit_work() via @system_unbound_wq.
> 
> We punt the request to task_work. task_work is run, we're still in the
> right context. We fail with -EAGAIN, and then call io_queue_async_work()
> and we're not doing async prep at that point.

I'm missing something. io_queue_async_work() calls io_prep_async_link()
-> io_prep_async_work() before actually queuing.

-- 
Pavel Begunkov

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Jens Axboe]

On 10/2/20 11:01 AM, Jens Axboe wrote:
> On 10/2/20 10:34 AM, Pavel Begunkov wrote:
>> On 02/10/2020 19:14, Pavel Begunkov wrote:
>>> On 19/09/2020 19:56, Pavel Begunkov wrote:
>>>> On 19/09/2020 18:27, Pavel Begunkov wrote:
>>>>> On 14/09/2020 19:25, Jens Axboe wrote:
>>>>>> Always grab work environment for deferred links. The assumption that we
>>>>>> will be running it always from the task in question is false, as exiting
>>>>>> tasks may mean that we're deferring this one to a thread helper. And at
>>>>>> that point it's too late to grab the work environment.
>>>> Forgot that they will be cancelled there. So, how it could happen?
>>>> Is that the initial thread will run task_work but loosing
>>>> some resources like mm prior to that? e.g. in do_exit()
>>>
>>> Jens, please let me know when you get time for that. I was thinking that
>>> you were meaning do_exit(), which does task_work_run() after killing mm,
>>> etc., but you mentioned a thread helper in the description... Which one
>>> do you mean?
>>
>> Either it refers to stuff after io_ring_ctx_wait_and_kill(), which
>> delegates the rest to io_ring_exit_work() via @system_unbound_wq.
> 
> We punt the request to task_work. task_work is run, we're still in the
> right context. We fail with -EAGAIN, and then call io_queue_async_work()
> and we're not doing async prep at that point.

BTW, I think we can improve on this for 5.10, on top of your cleanups.
So that would certainly be welcome!

-- 
Jens Axboe

Re: [PATCH 1/5] io_uring: grab any needed state during defer prep

[Pavel Begunkov]

On 02/10/2020 20:09, Jens Axboe wrote:
> On 10/2/20 11:01 AM, Jens Axboe wrote:
>> On 10/2/20 10:34 AM, Pavel Begunkov wrote:
>>> On 02/10/2020 19:14, Pavel Begunkov wrote:
>>>> On 19/09/2020 19:56, Pavel Begunkov wrote:
>>>>> On 19/09/2020 18:27, Pavel Begunkov wrote:
>>>>>> On 14/09/2020 19:25, Jens Axboe wrote:
>>>>>>> Always grab work environment for deferred links. The assumption that we
>>>>>>> will be running it always from the task in question is false, as exiting
>>>>>>> tasks may mean that we're deferring this one to a thread helper. And at
>>>>>>> that point it's too late to grab the work environment.
>>>>> Forgot that they will be cancelled there. So, how it could happen?
>>>>> Is that the initial thread will run task_work but loosing
>>>>> some resources like mm prior to that? e.g. in do_exit()
>>>>
>>>> Jens, please let me know when you get time for that. I was thinking that
>>>> you were meaning do_exit(), which does task_work_run() after killing mm,
>>>> etc., but you mentioned a thread helper in the description... Which one
>>>> do you mean?
>>>
>>> Either it refers to stuff after io_ring_ctx_wait_and_kill(), which
>>> delegates the rest to io_ring_exit_work() via @system_unbound_wq.
>>
>> We punt the request to task_work. task_work is run, we're still in the
>> right context. We fail with -EAGAIN, and then call io_queue_async_work()
>> and we're not doing async prep at that point.
> 
> BTW, I think we can improve on this for 5.10, on top of your cleanups.
> So that would certainly be welcome!

Yeah, I was going for that, it'll be in my way anyway.

-- 
Pavel Begunkov

[PATCH 2/5] io_uring: drop 'ctx' ref on task work cancelation

[Jens Axboe]

If task_work ends up being marked for cancelation, we go through a
cancelation helper instead of the queue path. In converting task_work to
always hold a ctx reference, this path was missed. Make sure that
io_req_task_cancel() puts the reference that is being held against the
ctx.

Fixes: 6d816e088c35 ("io_uring: hold 'ctx' reference around task_work queue + execute")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index be9d628e7854..01756a131be6 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1787,8 +1787,10 @@ static void __io_req_task_cancel(struct io_kiocb *req, int error)
 static void io_req_task_cancel(struct callback_head *cb)
 {
 	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
+	struct io_ring_ctx *ctx = req->ctx;
 
 	__io_req_task_cancel(req, -ECANCELED);
+	percpu_ref_put(&ctx->refs);
 }
 
 static void __io_req_task_submit(struct io_kiocb *req)
-- 
2.28.0

[PATCH 3/5] io_uring: don't run task work on an exiting task

[Jens Axboe]

This isn't safe, and isn't needed either. We are guaranteed that any
work we queue is on a live task (and will be run), or it goes to
our backup io-wq threads if the task is exiting.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 01756a131be6..a29c8913b1f0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1753,6 +1753,9 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb,
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret, notify;
 
+	if (tsk->flags & PF_EXITING)
+		return -ESRCH;
+
 	/*
 	 * SQPOLL kernel thread doesn't need notification, just a wakeup. For
 	 * all other cases, use TWA_SIGNAL unconditionally to ensure we're
@@ -2012,6 +2015,12 @@ static inline unsigned int io_put_rw_kbuf(struct io_kiocb *req)
 
 static inline bool io_run_task_work(void)
 {
+	/*
+	 * Not safe to run on exiting task, and the task_work handling will
+	 * not add work to such a task.
+	 */
+	if (unlikely(current->flags & PF_EXITING))
+		return false;
 	if (current->task_works) {
 		__set_current_state(TASK_RUNNING);
 		task_work_run();
@@ -8184,6 +8193,8 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
 		/* cancel this request, or head link requests */
 		io_attempt_cancel(ctx, cancel_req);
 		io_put_req(cancel_req);
+		/* cancellations _may_ trigger task work */
+		io_run_task_work();
 		schedule();
 		finish_wait(&ctx->inflight_wait, &wait);
 	}
-- 
2.28.0

[PATCH 4/5] io_uring: don't re-setup vecs/iter in io_resumit_prep() is already there

[Jens Axboe]

If we already have mapped the necessary data for retry, then don't set
it up again. It's a pointless operation, and we leak the iovec if it's
a large (non-stack) vec.

Fixes: b63534c41e20 ("io_uring: re-issue block requests that failed because of resources")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a9ff6e47eb16..05670a615663 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2293,13 +2293,17 @@ static bool io_resubmit_prep(struct io_kiocb *req, int error)
 		goto end_req;
 	}
 
-	ret = io_import_iovec(rw, req, &iovec, &iter, false);
-	if (ret < 0)
-		goto end_req;
-	ret = io_setup_async_rw(req, iovec, inline_vecs, &iter, false);
-	if (!ret)
+	if (!req->io) {
+		ret = io_import_iovec(rw, req, &iovec, &iter, false);
+		if (ret < 0)
+			goto end_req;
+		ret = io_setup_async_rw(req, iovec, inline_vecs, &iter, false);
+		if (!ret)
+			return true;
+		kfree(iovec);
+	} else {
 		return true;
-	kfree(iovec);
+	}
 end_req:
 	req_set_fail_links(req);
 	io_req_complete(req, ret);
-- 
2.28.0

[PATCH 5/5] io_uring: don't use retry based buffered reads for non-async bdev

[Jens Axboe]

Some block devices, like dm, bubble back -EAGAIN through the completion
handler. We cannot easily support the retry based buffered aio for those,
so catch it early and use the old style io-wq based fallback.

Fixes: bcf5a06304d6 ("io_uring: support true async buffered reads, if file provides it")
Reported-by: Zorro Lang <zlang@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 05670a615663..c9be9a607401 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3100,6 +3100,13 @@ static bool io_rw_should_retry(struct io_kiocb *req)
 	if (file_can_poll(req->file) || !(req->file->f_mode & FMODE_BUF_RASYNC))
 		return false;
 
+	/*
+	 * If we can't do nonblock submit without -EAGAIN direct return,
+	 * then don't use the retry based approach.
+	 */
+	if (!io_file_supports_async(req->file, READ))
+		return false;
+
 	wait->wait.func = io_async_buf_func;
 	wait->wait.private = req;
 	wait->wait.flags = 0;
-- 
2.28.0