Cancelation fixes

PATCH In review Jens Axboe 2 participants 10 messages Dec 9, 2021 – Dec 10, 2021

Subsystems (3)

Stable branch VFS io_uring

lore

Share & tools

lore

Jens Axboe

author maintainer

2021-12-09 16:00 UTC #0 lore raw

cover Cancelation fixes

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.

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2021-12-09 16:00 UTC #1 lore raw

PATCH 1/2 io_uring: check tctx->in_idle when decrementing inflight_tracked

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(+)

fs/io_uring.c +2 -0

index c4f217613f56..b4d5b8d168bf 100644
@@ -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);

Signature · 1 line

2.34.1

Hao Xu

2021-12-10 07:21 UTC #2 lore raw

Re: [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked

Quoted 6 lines · 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

Quoted 16 lines

  fs/io_uring.c | 2 ++
  1 file changed, 2 insertions(+)

fs/io_uring.c +2 -0

index c4f217613f56..b4d5b8d168bf 100644
@@ -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);

Jens Axboe

author maintainer

2021-12-10 13:47 UTC #3 lore raw

Re: [PATCH 1/2] io_uring: check tctx->in_idle when decrementing inflight_tracked

Quoted 9 lines · Hao Xu

Quoted 6 lines · 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]>
---

Quoted 7 lines

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.

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2021-12-09 16:00 UTC #4 lore raw

PATCH 2/2 io_uring: ensure task_work gets run as part of cancelations

In reply to 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 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(-)

fs/io_uring.c +5 -2

index b4d5b8d168bf..738076264436 100644
@@ -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

Signature · 1 line

2.34.1

Jens Axboe

author maintainer

2021-12-09 16:16 UTC #5 lore raw

PATCH v2 2/2 io_uring: ensure task_work gets run as part of cancelations

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.

fs/io_uring.c +4 -2

index b4d5b8d168bf..111db33b940e 100644
@@ -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

Signature · 1 line

Jens Axboe

Hao Xu

2021-12-10 03:29 UTC #6 lore raw

Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations

Quoted 8 lines · 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

Quoted 40 lines

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.

fs/io_uring.c +4 -2

index b4d5b8d168bf..111db33b940e 100644
@@ -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

author maintainer

2021-12-10 04:22 UTC #7 lore raw

Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations

Quoted 11 lines · Hao Xu

Quoted 8 lines · 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.

Signature · 1 line

Jens Axboe

Hao Xu

2021-12-10 07:31 UTC #8 lore raw

Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations

Quoted 8 lines · Jens Axboe

Quoted 7 lines · Hao Xu
Quoted 6 lines · 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.

Quoted 5 lines

Quoted 3 lines
Quoted 2 lines
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

Jens Axboe

author maintainer

2021-12-10 13:45 UTC #9 lore raw

Re: [PATCH v2 2/2] io_uring: ensure task_work gets run as part of cancelations

Quoted 10 lines · Hao Xu

Quoted 8 lines · Jens Axboe
Quoted 7 lines · Hao Xu
Quoted 6 lines · 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.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
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.

Signature · 1 line

Jens Axboe