io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHSET 0/5] io_uring fixes for 5.9
@ 2020-09-14 16:25 Jens Axboe
  2020-09-14 16:25 ` [PATCH 1/5] io_uring: grab any needed state during defer prep Jens Axboe
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring

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

-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
@ 2020-09-14 16:25 ` Jens Axboe
  2020-09-19 15:27   ` Pavel Begunkov
  2020-09-14 16:25 ` [PATCH 2/5] io_uring: drop 'ctx' ref on task work cancelation Jens Axboe
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring; +Cc: 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] io_uring: drop 'ctx' ref on task work cancelation
  2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
  2020-09-14 16:25 ` [PATCH 1/5] io_uring: grab any needed state during defer prep Jens Axboe
@ 2020-09-14 16:25 ` Jens Axboe
  2020-09-14 16:25 ` [PATCH 3/5] io_uring: don't run task work on an exiting task Jens Axboe
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring; +Cc: 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] io_uring: don't run task work on an exiting task
  2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
  2020-09-14 16:25 ` [PATCH 1/5] io_uring: grab any needed state during defer prep Jens Axboe
  2020-09-14 16:25 ` [PATCH 2/5] io_uring: drop 'ctx' ref on task work cancelation Jens Axboe
@ 2020-09-14 16:25 ` Jens Axboe
  2020-09-14 16:25 ` [PATCH 4/5] io_uring: don't re-setup vecs/iter in io_resumit_prep() is already there Jens Axboe
  2020-09-14 16:25 ` [PATCH 5/5] io_uring: don't use retry based buffered reads for non-async bdev Jens Axboe
  4 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring; +Cc: 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] io_uring: don't re-setup vecs/iter in io_resumit_prep() is already there
  2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
                   ` (2 preceding siblings ...)
  2020-09-14 16:25 ` [PATCH 3/5] io_uring: don't run task work on an exiting task Jens Axboe
@ 2020-09-14 16:25 ` Jens Axboe
  2020-09-14 16:25 ` [PATCH 5/5] io_uring: don't use retry based buffered reads for non-async bdev Jens Axboe
  4 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring; +Cc: 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] io_uring: don't use retry based buffered reads for non-async bdev
  2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
                   ` (3 preceding siblings ...)
  2020-09-14 16:25 ` [PATCH 4/5] io_uring: don't re-setup vecs/iter in io_resumit_prep() is already there Jens Axboe
@ 2020-09-14 16:25 ` Jens Axboe
  4 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-09-14 16:25 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, Zorro Lang

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-09-14 16:25 ` [PATCH 1/5] io_uring: grab any needed state during defer prep Jens Axboe
@ 2020-09-19 15:27   ` Pavel Begunkov
  2020-09-19 16:56     ` Pavel Begunkov
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Begunkov @ 2020-09-19 15:27 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-09-19 15:27   ` Pavel Begunkov
@ 2020-09-19 16:56     ` Pavel Begunkov
  2020-10-02 16:14       ` Pavel Begunkov
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Begunkov @ 2020-09-19 16:56 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-09-19 16:56     ` Pavel Begunkov
@ 2020-10-02 16:14       ` Pavel Begunkov
  2020-10-02 16:34         ` Pavel Begunkov
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Begunkov @ 2020-10-02 16:14 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-10-02 16:14       ` Pavel Begunkov
@ 2020-10-02 16:34         ` Pavel Begunkov
  2020-10-02 17:01           ` Jens Axboe
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Begunkov @ 2020-10-02 16:34 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-10-02 16:34         ` Pavel Begunkov
@ 2020-10-02 17:01           ` Jens Axboe
  2020-10-02 17:08             ` Pavel Begunkov
  2020-10-02 17:09             ` Jens Axboe
  0 siblings, 2 replies; 14+ messages in thread
From: Jens Axboe @ 2020-10-02 17:01 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-10-02 17:01           ` Jens Axboe
@ 2020-10-02 17:08             ` Pavel Begunkov
  2020-10-02 17:09             ` Jens Axboe
  1 sibling, 0 replies; 14+ messages in thread
From: Pavel Begunkov @ 2020-10-02 17:08 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-10-02 17:01           ` Jens Axboe
  2020-10-02 17:08             ` Pavel Begunkov
@ 2020-10-02 17:09             ` Jens Axboe
  2020-10-02 17:14               ` Pavel Begunkov
  1 sibling, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2020-10-02 17:09 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] io_uring: grab any needed state during defer prep
  2020-10-02 17:09             ` Jens Axboe
@ 2020-10-02 17:14               ` Pavel Begunkov
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Begunkov @ 2020-10-02 17:14 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-10-02 17:16 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 16:25 [PATCHSET 0/5] io_uring fixes for 5.9 Jens Axboe
2020-09-14 16:25 ` [PATCH 1/5] io_uring: grab any needed state during defer prep Jens Axboe
2020-09-19 15:27   ` Pavel Begunkov
2020-09-19 16:56     ` Pavel Begunkov
2020-10-02 16:14       ` Pavel Begunkov
2020-10-02 16:34         ` Pavel Begunkov
2020-10-02 17:01           ` Jens Axboe
2020-10-02 17:08             ` Pavel Begunkov
2020-10-02 17:09             ` Jens Axboe
2020-10-02 17:14               ` Pavel Begunkov
2020-09-14 16:25 ` [PATCH 2/5] io_uring: drop 'ctx' ref on task work cancelation Jens Axboe
2020-09-14 16:25 ` [PATCH 3/5] io_uring: don't run task work on an exiting task Jens Axboe
2020-09-14 16:25 ` [PATCH 4/5] io_uring: don't re-setup vecs/iter in io_resumit_prep() is already there Jens Axboe
2020-09-14 16:25 ` [PATCH 5/5] io_uring: don't use retry based buffered reads for non-async bdev Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).