public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCHSET 0/2] Cancelation fixes
@ 2021-12-09 15:59 Jens Axboe
  2021-12-09 15:59 ` [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked Jens Axboe
  2021-12-09 15:59 ` [PATCH 2/2] io_uring: ensure task_work gets run as part of cancelations Jens Axboe
  0 siblings, 2 replies; 10+ messages in thread
From: Jens Axboe @ 2021-12-09 15:59 UTC (permalink / raw)
  To: io-uring

Hi,

#1 fixes a missing wakeup on decrement of a value that is used with
the tctx->wait and in_idle tracking, and #2 ensures that we properly
process task_work off the cancelation path.

-- 
Jens Axboe



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

* [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked
  2021-12-09 15:59 [PATCHSET 0/2] Cancelation fixes Jens Axboe
@ 2021-12-09 15:59 ` Jens Axboe
  2021-12-10  7:21   ` Hao Xu
  2021-12-09 15:59 ` [PATCH 2/2] io_uring: ensure task_work gets run as part of cancelations Jens Axboe
  1 sibling, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-12-09 15:59 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, stable

If we have someone potentially waiting for tracked requests to finish,
ensure that we check in_idle and wake them up appropriately.

Cc: [email protected]
Signed-off-by: Jens Axboe <[email protected]>
---
 fs/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c4f217613f56..b4d5b8d168bf 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6640,6 +6640,8 @@ static void io_clean_op(struct io_kiocb *req)
 		struct io_uring_task *tctx = req->task->io_uring;
 
 		atomic_dec(&tctx->inflight_tracked);
+		if (unlikely(atomic_read(&tctx->in_idle)))
+			wake_up(&tctx->wait);
 	}
 	if (req->flags & REQ_F_CREDS)
 		put_cred(req->creds);
-- 
2.34.1


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

* [PATCH 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-09 15:59 [PATCHSET 0/2] Cancelation fixes Jens Axboe
  2021-12-09 15:59 ` [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked Jens Axboe
@ 2021-12-09 15:59 ` Jens Axboe
  2021-12-09 16:16   ` [PATCH v2 " Jens Axboe
  1 sibling, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-12-09 15:59 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, syzbot+21e6887c0be14181206d, stable

If we successfully cancel a work item but that work item needs to be
processed through task_work, then we can be sleeping uninterruptibly
in io_uring_cancel_generic() and never process it. Hence we don't
make forward progress and we end up with an uninterruptible sleep
warning.

Add the waitqueue earlier to ensure that any wakeups from cancelations
are seen, and switch to using uninterruptible sleep so that postponed
task_work additions get seen and processed.

While in there, correct a comment that should be IFF, not IIF.

Reported-by: [email protected]
Cc: [email protected]
Signed-off-by: Jens Axboe <[email protected]>
---
 fs/io_uring.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b4d5b8d168bf..738076264436 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9826,7 +9826,7 @@ static __cold void io_uring_drop_tctx_refs(struct task_struct *task)
 
 /*
  * Find any io_uring ctx that this task has registered or done IO on, and cancel
- * requests. @sqd should be not-null IIF it's an SQPOLL thread cancellation.
+ * requests. @sqd should be not-null IFF it's an SQPOLL thread cancellation.
  */
 static __cold void io_uring_cancel_generic(bool cancel_all,
 					   struct io_sq_data *sqd)
@@ -9851,6 +9851,8 @@ static __cold void io_uring_cancel_generic(bool cancel_all,
 		if (!inflight)
 			break;
 
+		prepare_to_wait(&tctx->wait, &wait, TASK_INTERRUPTIBLE);
+
 		if (!sqd) {
 			struct io_tctx_node *node;
 			unsigned long index;
@@ -9868,8 +9870,9 @@ static __cold void io_uring_cancel_generic(bool cancel_all,
 							     cancel_all);
 		}
 
-		prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
+		io_run_task_work();
 		io_uring_drop_tctx_refs(current);
+
 		/*
 		 * If we've seen completions, retry without waiting. This
 		 * avoids a race where a completion comes in before we did
-- 
2.34.1


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

* [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-09 15:59 ` [PATCH 2/2] io_uring: ensure task_work gets run as part of cancelations Jens Axboe
@ 2021-12-09 16:16   ` Jens Axboe
  2021-12-10  3:29     ` Hao Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-12-09 16:16 UTC (permalink / raw)
  To: io-uring; +Cc: syzbot+21e6887c0be14181206d, stable

If we successfully cancel a work item but that work item needs to be
processed through task_work, then we can be sleeping uninterruptibly
in io_uring_cancel_generic() and never process it. Hence we don't
make forward progress and we end up with an uninterruptible sleep
warning.

Add the waitqueue earlier to ensure that any wakeups from cancelations
are seen, and switch to using uninterruptible sleep so that postponed
task_work additions get seen and processed.

While in there, correct a comment that should be IFF, not IIF.

Reported-by: [email protected]
Cc: [email protected]
Signed-off-by: Jens Axboe <[email protected]>

---

v2 - don't move prepare_to_wait(), it'll run into issues with locking
     etc, and we don't need to as the inflight tracking guards against
     missing a wakeup for a completion.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b4d5b8d168bf..111db33b940e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9826,7 +9826,7 @@ static __cold void io_uring_drop_tctx_refs(struct task_struct *task)
 
 /*
  * Find any io_uring ctx that this task has registered or done IO on, and cancel
- * requests. @sqd should be not-null IIF it's an SQPOLL thread cancellation.
+ * requests. @sqd should be not-null IFF it's an SQPOLL thread cancellation.
  */
 static __cold void io_uring_cancel_generic(bool cancel_all,
 					   struct io_sq_data *sqd)
@@ -9868,8 +9868,10 @@ static __cold void io_uring_cancel_generic(bool cancel_all,
 							     cancel_all);
 		}
 
-		prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
+		prepare_to_wait(&tctx->wait, &wait, TASK_INTERRUPTIBLE);
+		io_run_task_work();
 		io_uring_drop_tctx_refs(current);
+
 		/*
 		 * If we've seen completions, retry without waiting. This
 		 * avoids a race where a completion comes in before we did

-- 
Jens Axboe


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

* Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-09 16:16   ` [PATCH v2 " Jens Axboe
@ 2021-12-10  3:29     ` Hao Xu
  2021-12-10  4:22       ` Jens Axboe
  0 siblings, 1 reply; 10+ messages in thread
From: Hao Xu @ 2021-12-10  3:29 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: syzbot+21e6887c0be14181206d, stable


在 2021/12/10 上午12:16, Jens Axboe 写道:
> If we successfully cancel a work item but that work item needs to be
> processed through task_work, then we can be sleeping uninterruptibly
> in io_uring_cancel_generic() and never process it. Hence we don't
> make forward progress and we end up with an uninterruptible sleep
> warning.
>
> Add the waitqueue earlier to ensure that any wakeups from cancelations
> are seen, and switch to using uninterruptible sleep so that postponed
^ typo
> task_work additions get seen and processed.
>
> While in there, correct a comment that should be IFF, not IIF.
>
> Reported-by: [email protected]
> Cc: [email protected]
> Signed-off-by: Jens Axboe <[email protected]>
>
> ---
>
> v2 - don't move prepare_to_wait(), it'll run into issues with locking
>       etc, and we don't need to as the inflight tracking guards against
>       missing a wakeup for a completion.
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b4d5b8d168bf..111db33b940e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -9826,7 +9826,7 @@ static __cold void io_uring_drop_tctx_refs(struct task_struct *task)
>   
>   /*
>    * Find any io_uring ctx that this task has registered or done IO on, and cancel
> - * requests. @sqd should be not-null IIF it's an SQPOLL thread cancellation.
> + * requests. @sqd should be not-null IFF it's an SQPOLL thread cancellation.
>    */
>   static __cold void io_uring_cancel_generic(bool cancel_all,
>   					   struct io_sq_data *sqd)
> @@ -9868,8 +9868,10 @@ static __cold void io_uring_cancel_generic(bool cancel_all,
>   							     cancel_all);
>   		}
>   
> -		prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
> +		prepare_to_wait(&tctx->wait, &wait, TASK_INTERRUPTIBLE);
> +		io_run_task_work();
>   		io_uring_drop_tctx_refs(current);
> +
>   		/*
>   		 * If we've seen completions, retry without waiting. This
>   		 * avoids a race where a completion comes in before we did
>

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

* Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-10  3:29     ` Hao Xu
@ 2021-12-10  4:22       ` Jens Axboe
  2021-12-10  7:31         ` Hao Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-12-10  4:22 UTC (permalink / raw)
  To: Hao Xu, io-uring; +Cc: syzbot+21e6887c0be14181206d, stable

On 12/9/21 8:29 PM, Hao Xu wrote:
> 
> 在 2021/12/10 上午12:16, Jens Axboe 写道:
>> If we successfully cancel a work item but that work item needs to be
>> processed through task_work, then we can be sleeping uninterruptibly
>> in io_uring_cancel_generic() and never process it. Hence we don't
>> make forward progress and we end up with an uninterruptible sleep
>> warning.
>>
>> Add the waitqueue earlier to ensure that any wakeups from cancelations
>> are seen, and switch to using uninterruptible sleep so that postponed
> ^ typo

Not really a typo, but should be killed from v2 for sure. I'll do that.

-- 
Jens Axboe


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

* Re: [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked
  2021-12-09 15:59 ` [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked Jens Axboe
@ 2021-12-10  7:21   ` Hao Xu
  2021-12-10 13:47     ` Jens Axboe
  0 siblings, 1 reply; 10+ messages in thread
From: Hao Xu @ 2021-12-10  7:21 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: stable


在 2021/12/9 下午11:59, Jens Axboe 写道:
> If we have someone potentially waiting for tracked requests to finish,
> ensure that we check in_idle and wake them up appropriately.
>
> Cc: [email protected]
> Signed-off-by: Jens Axboe <[email protected]>
> ---

Hi Jens,

I saw every/several( in batching cases)  io_clean_op() followed by an 
io_put_task() which does the same thing

as this patch, so it seems this one is not neccessary? Correct me if I'm 
wrong since I haven't touch this code for

a long time.


Regards,
Hao

>   fs/io_uring.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c4f217613f56..b4d5b8d168bf 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6640,6 +6640,8 @@ static void io_clean_op(struct io_kiocb *req)
>   		struct io_uring_task *tctx = req->task->io_uring;
>   
>   		atomic_dec(&tctx->inflight_tracked);
> +		if (unlikely(atomic_read(&tctx->in_idle)))
> +			wake_up(&tctx->wait);
>   	}
>   	if (req->flags & REQ_F_CREDS)
>   		put_cred(req->creds);

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

* Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-10  4:22       ` Jens Axboe
@ 2021-12-10  7:31         ` Hao Xu
  2021-12-10 13:45           ` Jens Axboe
  0 siblings, 1 reply; 10+ messages in thread
From: Hao Xu @ 2021-12-10  7:31 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: syzbot+21e6887c0be14181206d, stable


在 2021/12/10 下午12:22, Jens Axboe 写道:
> On 12/9/21 8:29 PM, Hao Xu wrote:
>> 在 2021/12/10 上午12:16, Jens Axboe 写道:
>>> If we successfully cancel a work item but that work item needs to be
>>> processed through task_work, then we can be sleeping uninterruptibly
>>> in io_uring_cancel_generic() and never process it. Hence we don't
>>> make forward progress and we end up with an uninterruptible sleep
>>> warning.
>>>
>>> Add the waitqueue earlier to ensure that any wakeups from cancelations
>>> are seen, and switch to using uninterruptible sleep so that postponed
>> ^ typo
> Not really a typo, but should be killed from v2 for sure. I'll do that.
>
Don't know why the ^ char doesn't align with 'uninterruptible' ... here 
I mean 'uninterruptible' is a typo

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

* Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations
  2021-12-10  7:31         ` Hao Xu
@ 2021-12-10 13:45           ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2021-12-10 13:45 UTC (permalink / raw)
  To: Hao Xu, io-uring; +Cc: syzbot+21e6887c0be14181206d, stable

On 12/10/21 12:31 AM, Hao Xu wrote:
> 
> 在 2021/12/10 下午12:22, Jens Axboe 写道:
>> On 12/9/21 8:29 PM, Hao Xu wrote:
>>> 在 2021/12/10 上午12:16, Jens Axboe 写道:
>>>> If we successfully cancel a work item but that work item needs to be
>>>> processed through task_work, then we can be sleeping uninterruptibly
>>>> in io_uring_cancel_generic() and never process it. Hence we don't
>>>> make forward progress and we end up with an uninterruptible sleep
>>>> warning.
>>>>
>>>> Add the waitqueue earlier to ensure that any wakeups from cancelations
>>>> are seen, and switch to using uninterruptible sleep so that postponed
>>> ^ typo
>> Not really a typo, but should be killed from v2 for sure. I'll do that.
>>
> Don't know why the ^ char doesn't align with 'uninterruptible' ... here 
> I mean 'uninterruptible' is a typo

Gotcha, I guess the end result is the same as I killed the section on
moving the sleep.

-- 
Jens Axboe


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

* Re: [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked
  2021-12-10  7:21   ` Hao Xu
@ 2021-12-10 13:47     ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2021-12-10 13:47 UTC (permalink / raw)
  To: Hao Xu, io-uring; +Cc: stable

On 12/10/21 12:21 AM, Hao Xu wrote:
> 
> 在 2021/12/9 下午11:59, Jens Axboe 写道:
>> If we have someone potentially waiting for tracked requests to finish,
>> ensure that we check in_idle and wake them up appropriately.
>>
>> Cc: [email protected]
>> Signed-off-by: Jens Axboe <[email protected]>
>> ---
> 
> Hi Jens,
> 
> I saw every/several( in batching cases)  io_clean_op() followed by an 
> io_put_task() which does the same thing
> 
> as this patch, so it seems this one is not neccessary? Correct me if I'm 
> wrong since I haven't touch this code for

Hard to deduce as it also depends on whether it's the task itself or not.
Making it explicit is better imho.

-- 
Jens Axboe


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

end of thread, other threads:[~2021-12-10 13:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-09 15:59 [PATCHSET 0/2] Cancelation fixes Jens Axboe
2021-12-09 15:59 ` [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked Jens Axboe
2021-12-10  7:21   ` Hao Xu
2021-12-10 13:47     ` Jens Axboe
2021-12-09 15:59 ` [PATCH 2/2] io_uring: ensure task_work gets run as part of cancelations Jens Axboe
2021-12-09 16:16   ` [PATCH v2 " Jens Axboe
2021-12-10  3:29     ` Hao Xu
2021-12-10  4:22       ` Jens Axboe
2021-12-10  7:31         ` Hao Xu
2021-12-10 13:45           ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox