[PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Jens Axboe]

We have two checks of task->flags & PF_EXITING left:

1) In io_req_task_submit(), which is called in task_work and hence always
   in the context of the original task. That means that
   req->task == current, and hence checking ->flags is totally fine.

2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
   it interfering with cancelation. Here, req->task is not necessarily
   current, and hence the check is racy. Use the ctx refs state instead
   to check if we need to cancel this request or not.

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

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 30edc329d803..ffce959c2370 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2114,6 +2114,7 @@ static void io_req_task_submit(struct io_kiocb *req)
 
 	/* ctx stays valid until unlock, even if we drop all ours ctx->refs */
 	mutex_lock(&ctx->uring_lock);
+	/* req->task == current here, checking PF_EXITING is safe */
 	if (likely(!(req->task->flags & PF_EXITING)))
 		__io_queue_sqe(req);
 	else
@@ -4895,7 +4896,11 @@ static bool io_poll_rewait(struct io_kiocb *req, struct io_poll_iocb *poll)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	if (unlikely(req->task->flags & PF_EXITING))
+	/*
+	 * Pairs with spin_unlock() in percpu_ref_kill()
+	 */
+	smp_rmb();
+	if (unlikely(percpu_ref_is_dying(&ctx->refs)))
 		WRITE_ONCE(poll->canceled, true);
 
 	if (!req->result && !READ_ONCE(poll->canceled)) {

-- 
Jens Axboe

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Hao Xu]

在 2021/8/19 下午11:48, Jens Axboe 写道:
> We have two checks of task->flags & PF_EXITING left:
> 
> 1) In io_req_task_submit(), which is called in task_work and hence always
>     in the context of the original task. That means that
>     req->task == current, and hence checking ->flags is totally fine.
> 
> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>     it interfering with cancelation. Here, req->task is not necessarily
>     current, and hence the check is racy. Use the ctx refs state instead
>     to check if we need to cancel this request or not.
Hi Jens,
I saw cases that io_req_task_submit() and io_poll_rewait() in one
function, why one is safe and the other one not? btw, it seems both two
executes in task_work context..and task_work_add() may fail and then
work goes to system_wq, is that case safe?
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 30edc329d803..ffce959c2370 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2114,6 +2114,7 @@ static void io_req_task_submit(struct io_kiocb *req)
>   
>   	/* ctx stays valid until unlock, even if we drop all ours ctx->refs */
>   	mutex_lock(&ctx->uring_lock);
> +	/* req->task == current here, checking PF_EXITING is safe */
>   	if (likely(!(req->task->flags & PF_EXITING)))
>   		__io_queue_sqe(req);
>   	else
> @@ -4895,7 +4896,11 @@ static bool io_poll_rewait(struct io_kiocb *req, struct io_poll_iocb *poll)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   
> -	if (unlikely(req->task->flags & PF_EXITING))
> +	/*
> +	 * Pairs with spin_unlock() in percpu_ref_kill()
> +	 */
> +	smp_rmb();
> +	if (unlikely(percpu_ref_is_dying(&ctx->refs)))
>   		WRITE_ONCE(poll->canceled, true);
>   
>   	if (!req->result && !READ_ONCE(poll->canceled)) {
> 

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Jens Axboe]

On 8/19/21 11:26 AM, Hao Xu wrote:
> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>> We have two checks of task->flags & PF_EXITING left:
>>
>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>     in the context of the original task. That means that
>>     req->task == current, and hence checking ->flags is totally fine.
>>
>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>     it interfering with cancelation. Here, req->task is not necessarily
>>     current, and hence the check is racy. Use the ctx refs state instead
>>     to check if we need to cancel this request or not.
> Hi Jens,
> I saw cases that io_req_task_submit() and io_poll_rewait() in one
> function, why one is safe and the other one not? btw, it seems both two
> executes in task_work context..and task_work_add() may fail and then
> work goes to system_wq, is that case safe?

io_req_task_submit() is guaranteed to be run in the task that is req->task,
io_poll_rewait() is not. The latter can get called from eg the poll
waitqueue handling, which is not run from the task in question.

-- 
Jens Axboe

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Jens Axboe]

On 8/19/21 11:29 AM, Jens Axboe wrote:
> On 8/19/21 11:26 AM, Hao Xu wrote:
>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>> We have two checks of task->flags & PF_EXITING left:
>>>
>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>     in the context of the original task. That means that
>>>     req->task == current, and hence checking ->flags is totally fine.
>>>
>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>     it interfering with cancelation. Here, req->task is not necessarily
>>>     current, and hence the check is racy. Use the ctx refs state instead
>>>     to check if we need to cancel this request or not.
>> Hi Jens,
>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>> function, why one is safe and the other one not? btw, it seems both two
>> executes in task_work context..and task_work_add() may fail and then
>> work goes to system_wq, is that case safe?
> 
> io_req_task_submit() is guaranteed to be run in the task that is req->task,
> io_poll_rewait() is not. The latter can get called from eg the poll
> waitqueue handling, which is not run from the task in question.

Pavel nudged me, and in the 5.15 branch we actually only do run rewait
from the task itself. So this patch isn't needed, we can ignore it!
Might just augment it with a comment, like it was done for submit.

-- 
Jens Axboe

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Hao Xu]

在 2021/8/20 上午1:29, Jens Axboe 写道:
> On 8/19/21 11:26 AM, Hao Xu wrote:
>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>> We have two checks of task->flags & PF_EXITING left:
>>>
>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>      in the context of the original task. That means that
>>>      req->task == current, and hence checking ->flags is totally fine.
>>>
>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>      to check if we need to cancel this request or not.
>> Hi Jens,
>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>> function, why one is safe and the other one not? btw, it seems both two
>> executes in task_work context..and task_work_add() may fail and then
>> work goes to system_wq, is that case safe?
I've got answer for the second question..
> 
> io_req_task_submit() is guaranteed to be run in the task that is req->task,
> io_poll_rewait() is not. The latter can get called from eg the poll
> waitqueue handling, which is not run from the task in question.
I only found io_poll_rewait() call in io_async_task_func() and
io_poll_task_func(), both are in task_work
> 

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Jens Axboe]

On 8/19/21 11:36 AM, Hao Xu wrote:
> 在 2021/8/20 上午1:29, Jens Axboe 写道:
>> On 8/19/21 11:26 AM, Hao Xu wrote:
>>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>>> We have two checks of task->flags & PF_EXITING left:
>>>>
>>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>>      in the context of the original task. That means that
>>>>      req->task == current, and hence checking ->flags is totally fine.
>>>>
>>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>>      to check if we need to cancel this request or not.
>>> Hi Jens,
>>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>>> function, why one is safe and the other one not? btw, it seems both two
>>> executes in task_work context..and task_work_add() may fail and then
>>> work goes to system_wq, is that case safe?
> I've got answer for the second question..
>>
>> io_req_task_submit() is guaranteed to be run in the task that is req->task,
>> io_poll_rewait() is not. The latter can get called from eg the poll
>> waitqueue handling, which is not run from the task in question.
> I only found io_poll_rewait() call in io_async_task_func() and
> io_poll_task_func(), both are in task_work

Yeah see followup, my information was outdated, we only do rewait from the
right context at this point. Hence the PF_EXITING check is actually fine.

-- 
Jens Axboe

Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()

[Hao Xu]

在 2021/8/20 上午1:36, Jens Axboe 写道:
> On 8/19/21 11:29 AM, Jens Axboe wrote:
>> On 8/19/21 11:26 AM, Hao Xu wrote:
>>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>>> We have two checks of task->flags & PF_EXITING left:
>>>>
>>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>>      in the context of the original task. That means that
>>>>      req->task == current, and hence checking ->flags is totally fine.
>>>>
>>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>>      to check if we need to cancel this request or not.
>>> Hi Jens,
>>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>>> function, why one is safe and the other one not? btw, it seems both two
>>> executes in task_work context..and task_work_add() may fail and then
>>> work goes to system_wq, is that case safe?
>>
>> io_req_task_submit() is guaranteed to be run in the task that is req->task,
>> io_poll_rewait() is not. The latter can get called from eg the poll
>> waitqueue handling, which is not run from the task in question.
> 
> Pavel nudged me, and in the 5.15 branch we actually only do run rewait
> from the task itself. So this patch isn't needed, we can ignore it!
> Might just augment it with a comment, like it was done for submit.
saw this after my second email. cool!
>