[PATCH 0/2] io-wq: cancelation race fix and small cleanup in io-wq

[PATCH 0/2] io-wq: cancelation race fix and small cleanup in io-wq

[Gabriel Krisman Bertazi]

Hi Jens,

Two small fixes to the wq path, closing a small race of cancelation with
wq work removal for execution.

Thank you,

Gabriel Krisman Bertazi (2):
  io-wq: write next_work before dropping acct_lock
  io-wq: Drop intermediate step between pending list and active work

 io_uring/io-wq.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.44.0

[PATCH 1/2] io-wq: write next_work before dropping acct_lock

[Gabriel Krisman Bertazi]

Commit 361aee450c6e ("io-wq: add intermediate work step between pending
list and active work") closed a race between a cancellation and the work
being removed from the wq for execution.  To ensure the request is
always reachable by the cancellation, we need to move it within the wq
lock, which also synchronizes the cancellation.  But commit
42abc95f05bf ("io-wq: decouple work_list protection from the big
wqe->lock") replaced the wq lock here and accidentally reintroduced the
race by releasing the acct_lock too early.

In other words:

        worker                |     cancellation
work = io_get_next_work()     |
raw_spin_unlock(&acct->lock); |
			      |
                              | io_acct_cancel_pending_work
                              | io_wq_worker_cancel()
worker->next_work = work

Using acct_lock is still enough since we synchronize on it on
io_acct_cancel_pending_work.

Fixes: 42abc95f05bf ("io-wq: decouple work_list protection from the big wqe->lock")
Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 io_uring/io-wq.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 522196dfb0ff..318ed067dbf6 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -564,10 +564,7 @@ static void io_worker_handle_work(struct io_wq_acct *acct,
 		 * clear the stalled flag.
 		 */
 		work = io_get_next_work(acct, worker);
-		raw_spin_unlock(&acct->lock);
 		if (work) {
-			__io_worker_busy(wq, worker);
-
 			/*
 			 * Make sure cancelation can find this, even before
 			 * it becomes the active work. That avoids a window
@@ -578,9 +575,15 @@ static void io_worker_handle_work(struct io_wq_acct *acct,
 			raw_spin_lock(&worker->lock);
 			worker->next_work = work;
 			raw_spin_unlock(&worker->lock);
-		} else {
-			break;
 		}
+
+		raw_spin_unlock(&acct->lock);
+
+		if (!work)
+			break;
+
+		__io_worker_busy(wq, worker);
+
 		io_assign_current_work(worker, work);
 		__set_current_state(TASK_RUNNING);
 
-- 
2.44.0

[PATCH 2/2] io-wq: Drop intermediate step between pending list and active work

[Gabriel Krisman Bertazi]

next_work is only used to make the work visible for
cancellation. Instead, we can just directly write to cur_work before
dropping the acct_lock and avoid the extra hop.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 io_uring/io-wq.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 318ed067dbf6..d7fc6f6d4477 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -51,7 +51,6 @@ struct io_worker {
 	struct io_wq *wq;
 
 	struct io_wq_work *cur_work;
-	struct io_wq_work *next_work;
 	raw_spinlock_t lock;
 
 	struct completion ref_done;
@@ -539,7 +538,6 @@ static void io_assign_current_work(struct io_worker *worker,
 
 	raw_spin_lock(&worker->lock);
 	worker->cur_work = work;
-	worker->next_work = NULL;
 	raw_spin_unlock(&worker->lock);
 }
 
@@ -573,7 +571,7 @@ static void io_worker_handle_work(struct io_wq_acct *acct,
 			 * current work item for this worker.
 			 */
 			raw_spin_lock(&worker->lock);
-			worker->next_work = work;
+			worker->cur_work = work;
 			raw_spin_unlock(&worker->lock);
 		}
 
@@ -1008,8 +1006,7 @@ static bool io_wq_worker_cancel(struct io_worker *worker, void *data)
 	 * may dereference the passed in work.
 	 */
 	raw_spin_lock(&worker->lock);
-	if (__io_wq_worker_cancel(worker, match, worker->cur_work) ||
-	    __io_wq_worker_cancel(worker, match, worker->next_work))
+	if (__io_wq_worker_cancel(worker, match, worker->cur_work))
 		match->nr_running++;
 	raw_spin_unlock(&worker->lock);
 
-- 
2.44.0

Re: [PATCH 0/2] io-wq: cancelation race fix and small cleanup in io-wq

[Jens Axboe]

On 4/15/24 8:10 PM, Gabriel Krisman Bertazi wrote:
> Hi Jens,
> 
> Two small fixes to the wq path, closing a small race of cancelation with
> wq work removal for execution.

Looks good, and cleans it up nicely. Thanks!

-- 
Jens Axboe

Re: [PATCH 0/2] io-wq: cancelation race fix and small cleanup in io-wq

[Jens Axboe]

On Mon, 15 Apr 2024 22:10:52 -0400, Gabriel Krisman Bertazi wrote:
> Two small fixes to the wq path, closing a small race of cancelation with
> wq work removal for execution.
> 
> Thank you,
> 
> Gabriel Krisman Bertazi (2):
>   io-wq: write next_work before dropping acct_lock
>   io-wq: Drop intermediate step between pending list and active work
> 
> [...]

Applied, thanks!

[1/2] io-wq: write next_work before dropping acct_lock
      commit: 068c27e32e51e94e4a9eb30ae85f4097a3602980
[2/2] io-wq: Drop intermediate step between pending list and active work
      commit: 24c3fc5c75c5b9d471783b4a4958748243828613

Best regards,
-- 
Jens Axboe