bug fix for nr_workers

PATCH In review Hao Xu 2 participants 7 messages Aug 8, 2021 – Aug 9, 2021

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Hao Xu

author

2021-08-08 13:54 UTC #0 lore raw

cover bug fix for nr_workers

The first one is to fix bugs in the previous patches about nr_workers. The second one is to fix the IO_WORKER_F_FIXED logic.

Hao Xu (2):
io-wq: fix bug of creating io-wokers unconditionally
io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker()

 fs/io-wq.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

Signature · 1 line

2.24.4

Hao Xu

author

2021-08-08 13:54 UTC #1 lore raw

PATCH 1/2 io-wq: fix bug of creating io-wokers unconditionally

The former patch to add check between nr_workers and max_workers has a bug, which will cause unconditionally creating io-workers. That's because the result of the check doesn't affect the call of create_io_worker(), fix it by bringing in a boolean value for it.

Fixes 21698274da5b ("io-wq: fix lack of acct->nr_workers < acct->max_workers judgement")

Signed-off-by Hao Xu <[email protected]>

---
 fs/io-wq.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

fs/io-wq.c +14 -5

index 12fc19353bb0..5536b2a008d1 100644
@@ -252,14 +252,15 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 
 		raw_spin_lock_irq(&wqe->lock);
 		if (acct->nr_workers < acct->max_workers) {
-			atomic_inc(&acct->nr_running);
-			atomic_inc(&wqe->wq->worker_refs);
 			acct->nr_workers++;
 			do_create = true;
 		}
 		raw_spin_unlock_irq(&wqe->lock);
-		if (do_create)
+		if (do_create) {
+			atomic_inc(&acct->nr_running);
+			atomic_inc(&wqe->wq->worker_refs);
 			create_io_worker(wqe->wq, wqe, acct->index);
+		}
 	}
 }

@@ -282,16 +283,24 @@ static void create_worker_cb(struct callback_head *cb)
 	struct io_wq *wq;
 	struct io_wqe *wqe;
 	struct io_wqe_acct *acct;
+	bool do_create = false;
 
 	cwd = container_of(cb, struct create_worker_data, work);
 	wqe = cwd->wqe;
 	wq = wqe->wq;
 	acct = &wqe->acct[cwd->index];
 	raw_spin_lock_irq(&wqe->lock);
-	if (acct->nr_workers < acct->max_workers)
+	if (acct->nr_workers < acct->max_workers) {
 		acct->nr_workers++;
+		do_create = true;
+	}
 	raw_spin_unlock_irq(&wqe->lock);
-	create_io_worker(wq, cwd->wqe, cwd->index);
+	if (do_create) {
+		create_io_worker(wq, cwd->wqe, cwd->index);
+	} else {
+		atomic_dec(&acct->nr_running);
+		io_worker_ref_put(wq);
+	}
 	kfree(cwd);
 }

Signature · 1 line

2.24.4

Jens Axboe

maintainer

2021-08-09 14:01 UTC #2 lore raw

Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally

Quoted 22 lines · Hao Xu

The former patch to add check between nr_workers and max_workers has a bug, which will cause unconditionally creating io-workers. That's because the result of the check doesn't affect the call of create_io_worker(), fix it by bringing in a boolean value for it.
Fixes 21698274da5b ("io-wq: fix lack of acct->nr_workers < acct->max_workers judgement")
Signed-off-by Hao Xu <[email protected]>
---
 fs/io-wq.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
fs/io-wq.c +0 -2
index 12fc19353bb0..5536b2a008d1 100644
@@ -252,14 +252,15 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 
 		raw_spin_lock_irq(&wqe->lock);
 		if (acct->nr_workers < acct->max_workers) {
-			atomic_inc(&acct->nr_running);
-			atomic_inc(&wqe->wq->worker_refs);
 			acct->nr_workers++;

fs/io-wq.c static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct) · line 264

Quoted 10 lines

fs/io-wq.c +4 -1

 			do_create = true;
 		}
 		raw_spin_unlock_irq(&wqe->lock);
-		if (do_create)
+		if (do_create) {
+			atomic_inc(&acct->nr_running);
+			atomic_inc(&wqe->wq->worker_refs);
 			create_io_worker(wqe->wq, wqe, acct->index);
+		}
 	}

I don't get this hunk - we already know we're creating a worker, what's the point in moving the incs?

Signature · 1 line

Jens Axboe

Hao Xu

author

2021-08-09 14:08 UTC #3 lore raw

Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally

Quoted 28 lines · Jens Axboe

Quoted 27 lines · Hao Xu
The former patch to add check between nr_workers and max_workers has a bug, which will cause unconditionally creating io-workers. That's because the result of the check doesn't affect the call of create_io_worker(), fix it by bringing in a boolean value for it.
Fixes 21698274da5b ("io-wq: fix lack of acct->nr_workers < acct->max_workers judgement")
Signed-off-by Hao Xu <[email protected]>
---
  fs/io-wq.c | 19 ++++++++++++++-----
  1 file changed, 14 insertions(+), 5 deletions(-)
fs/io-wq.c +1 -3
index 12fc19353bb0..5536b2a008d1 100644
@@ -252,14 +252,15 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 
  		raw_spin_lock_irq(&wqe->lock);
  		if (acct->nr_workers < acct->max_workers) {
-			atomic_inc(&acct->nr_running);
-			atomic_inc(&wqe->wq->worker_refs);
  			acct->nr_workers++;
  			do_create = true;
  		}
  		raw_spin_unlock_irq(&wqe->lock);
-		if (do_create)
+		if (do_create) {

Quoted 9 lines

fs/io-wq.c static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct) · line 264
Quoted 5 lines
fs/io-wq.c +3 -0
+			atomic_inc(&acct->nr_running);
+			atomic_inc(&wqe->wq->worker_refs);
  			create_io_worker(wqe->wq, wqe, acct->index);
+		}
  	}
I don't get this hunk - we already know we're creating a worker, what's the point in moving the incs?

Actually not much difference, I think we don't need to protect nr_running and worker_refs by wqe->lock, so narrow the range of raw_spin_lock_irq - raw_spin_unlock_irq

Jens Axboe

maintainer

2021-08-09 14:19 UTC #4 lore raw

Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally

Quoted 38 lines · Hao Xu

Quoted 28 lines · Jens Axboe
Quoted 27 lines · Hao Xu
The former patch to add check between nr_workers and max_workers has a bug, which will cause unconditionally creating io-workers. That's because the result of the check doesn't affect the call of create_io_worker(), fix it by bringing in a boolean value for it.
Fixes 21698274da5b ("io-wq: fix lack of acct->nr_workers < acct->max_workers judgement")
Signed-off-by Hao Xu <[email protected]>
---
  fs/io-wq.c | 19 ++++++++++++++-----
  1 file changed, 14 insertions(+), 5 deletions(-)
fs/io-wq.c +1 -3
index 12fc19353bb0..5536b2a008d1 100644
@@ -252,14 +252,15 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 
  		raw_spin_lock_irq(&wqe->lock);
  		if (acct->nr_workers < acct->max_workers) {
-			atomic_inc(&acct->nr_running);
-			atomic_inc(&wqe->wq->worker_refs);
  			acct->nr_workers++;
  			do_create = true;
  		}
  		raw_spin_unlock_irq(&wqe->lock);
-		if (do_create)
+		if (do_create) {
Quoted 9 lines
fs/io-wq.c static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct) · line 264
Quoted 5 lines
fs/io-wq.c +3 -0
+			atomic_inc(&acct->nr_running);
+			atomic_inc(&wqe->wq->worker_refs);
  			create_io_worker(wqe->wq, wqe, acct->index);
+		}
  	}
I don't get this hunk - we already know we're creating a worker, what's the point in moving the incs?

Quoted 3 lines

Actually not much difference, I think we don't need to protect nr_running and worker_refs by wqe->lock, so narrow the range of raw_spin_lock_irq - raw_spin_unlock_irq

Agree, we don't need it, but it's not a fix as such. I'd rather defer that one to a separate cleanup for the next release.

Signature · 1 line

Jens Axboe

Hao Xu

author

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

Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally

Quoted 39 lines · Jens Axboe

Quoted 38 lines · Hao Xu
Quoted 37 lines · Jens Axboe
Quoted 22 lines · Hao Xu
The former patch to add check between nr_workers and max_workers has a bug, which will cause unconditionally creating io-workers. That's because the result of the check doesn't affect the call of create_io_worker(), fix it by bringing in a boolean value for it.
Fixes 21698274da5b ("io-wq: fix lack of acct->nr_workers < acct->max_workers judgement")
Signed-off-by Hao Xu <[email protected]>
---
   fs/io-wq.c | 19 ++++++++++++++-----
   1 file changed, 14 insertions(+), 5 deletions(-)
fs/io-wq.c +0 -2
index 12fc19353bb0..5536b2a008d1 100644
@@ -252,14 +252,15 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 
   		raw_spin_lock_irq(&wqe->lock);
   		if (acct->nr_workers < acct->max_workers) {
-			atomic_inc(&acct->nr_running);
-			atomic_inc(&wqe->wq->worker_refs);
   			acct->nr_workers++;
fs/io-wq.c static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct) · line 264
Quoted 10 lines
fs/io-wq.c +4 -1
   			do_create = true;
   		}
   		raw_spin_unlock_irq(&wqe->lock);
-		if (do_create)
+		if (do_create) {
+			atomic_inc(&acct->nr_running);
+			atomic_inc(&wqe->wq->worker_refs);
   			create_io_worker(wqe->wq, wqe, acct->index);
+		}
   	}
I don't get this hunk - we already know we're creating a worker, what's the point in moving the incs?

Quoted 6 lines

Quoted 3 lines
Actually not much difference, I think we don't need to protect nr_running and worker_refs by wqe->lock, so narrow the range of raw_spin_lock_irq - raw_spin_unlock_irq
Agree, we don't need it, but it's not a fix as such. I'd rather defer that one to a separate cleanup for the next release.

I'll send it later.

Quoted 1 line

Hao Xu

author

2021-08-08 13:54 UTC #6 lore raw

PATCH 2/2 io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker()

In reply to Hao Xu

There may be cases like:
A B
spin_lock(wqe->lock)
nr_workers is 0
nr_workers++
spin_unlock(wqe->lock)

                                     spin_lock(wqe->lock)
                                     nr_wokers is 1
                                     nr_workers++
                                     spin_unlock(wqe->lock)
create_io_worker()
  acct->worker is 1
                                     create_io_worker()
                                       acct->worker is 1

There should be one worker marked IO_WORKER_F_FIXED, but no one is. Fix this by introduce a new agrument for create_io_worker() to indicate if it is the first worker.

Fixes 3d4e4face9c1 ("io-wq: fix no lock protection of acct->nr_worker")

Signed-off-by Hao Xu <[email protected]>

---
 fs/io-wq.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

fs/io-wq.c +11 -7

index 5536b2a008d1..660625ac02d7 100644
@@ -129,7 +129,7 @@ struct io_cb_cancel_data {
 	bool cancel_all;
 };
 
-static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index);
+static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index, bool first);
 static void io_wqe_dec_running(struct io_worker *worker);
 
 static bool io_worker_get(struct io_worker *worker)

@@ -248,10 +248,12 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 	rcu_read_unlock();
 
 	if (!ret) {
-		bool do_create = false;
+		bool do_create = false, first = false;
 
 		raw_spin_lock_irq(&wqe->lock);
 		if (acct->nr_workers < acct->max_workers) {
+			if (!acct->nr_workers)
+				first = true;
 			acct->nr_workers++;
 			do_create = true;
 		}

@@ -259,7 +261,7 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 		if (do_create) {
 			atomic_inc(&acct->nr_running);
 			atomic_inc(&wqe->wq->worker_refs);
-			create_io_worker(wqe->wq, wqe, acct->index);
+			create_io_worker(wqe->wq, wqe, acct->index, first);
 		}
 	}
 }

@@ -283,7 +285,7 @@ static void create_worker_cb(struct callback_head *cb)
 	struct io_wq *wq;
 	struct io_wqe *wqe;
 	struct io_wqe_acct *acct;
-	bool do_create = false;
+	bool do_create = false, first = false;
 
 	cwd = container_of(cb, struct create_worker_data, work);
 	wqe = cwd->wqe;

@@ -291,12 +293,14 @@ static void create_worker_cb(struct callback_head *cb)
 	acct = &wqe->acct[cwd->index];
 	raw_spin_lock_irq(&wqe->lock);
 	if (acct->nr_workers < acct->max_workers) {
+		if (!acct->nr_workers)
+			first = true;
 		acct->nr_workers++;
 		do_create = true;
 	}
 	raw_spin_unlock_irq(&wqe->lock);
 	if (do_create) {
-		create_io_worker(wq, cwd->wqe, cwd->index);
+		create_io_worker(wq, wqe, cwd->index, first);
 	} else {
 		atomic_dec(&acct->nr_running);
 		io_worker_ref_put(wq);

@@ -638,7 +642,7 @@ void io_wq_worker_sleeping(struct task_struct *tsk)
 	raw_spin_unlock_irq(&worker->wqe->lock);
 }
 
-static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
+static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index, bool first)
 {
 	struct io_wqe_acct *acct = &wqe->acct[index];
 	struct io_worker *worker;

@@ -679,7 +683,7 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 	worker->flags |= IO_WORKER_F_FREE;
 	if (index == IO_WQ_ACCT_BOUND)
 		worker->flags |= IO_WORKER_F_BOUND;
-	if ((acct->nr_workers == 1) && (worker->flags & IO_WORKER_F_BOUND))
+	if (first && (worker->flags & IO_WORKER_F_BOUND))
 		worker->flags |= IO_WORKER_F_FIXED;
 	raw_spin_unlock_irq(&wqe->lock);
 	wake_up_new_task(tsk);

Signature · 1 line

2.24.4