[PATCH 0/2] bug fix for nr_workers

[PATCH 0/2] bug fix for nr_workers

[Hao Xu]

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

-- 
2.24.4

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

[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 <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 12fc19353bb0..5536b2a008d1 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -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);
 }
 
-- 
2.24.4

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

[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 <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 5536b2a008d1..660625ac02d7 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -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);
-- 
2.24.4

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

[Jens Axboe]

On 8/8/21 7:54 AM, Hao Xu wrote:
> 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 <haoxu@linux.alibaba.com>
> ---
>  fs/io-wq.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 12fc19353bb0..5536b2a008d1 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -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);
> +		}
>  	}

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

-- 
Jens Axboe

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

[Hao Xu]

在 2021/8/9 下午10:01, Jens Axboe 写道:
> On 8/8/21 7:54 AM, Hao Xu wrote:
>> 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 <haoxu@linux.alibaba.com>
>> ---
>>   fs/io-wq.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index 12fc19353bb0..5536b2a008d1 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -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);
>> +		}
>>   	}
> 
> 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

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

[Jens Axboe]

On 8/9/21 8:08 AM, Hao Xu wrote:
> 在 2021/8/9 下午10:01, Jens Axboe 写道:
>> On 8/8/21 7:54 AM, Hao Xu wrote:
>>> 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 <haoxu@linux.alibaba.com>
>>> ---
>>>   fs/io-wq.c | 19 ++++++++++++++-----
>>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index 12fc19353bb0..5536b2a008d1 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -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);
>>> +		}
>>>   	}
>>
>> 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

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.

-- 
Jens Axboe

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

[Hao Xu]

在 2021/8/9 下午10:18, Jens Axboe 写道:
> On 8/9/21 8:08 AM, Hao Xu wrote:
>> 在 2021/8/9 下午10:01, Jens Axboe 写道:
>>> On 8/8/21 7:54 AM, Hao Xu wrote:
>>>> 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 <haoxu@linux.alibaba.com>
>>>> ---
>>>>    fs/io-wq.c | 19 ++++++++++++++-----
>>>>    1 file changed, 14 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>>> index 12fc19353bb0..5536b2a008d1 100644
>>>> --- a/fs/io-wq.c
>>>> +++ b/fs/io-wq.c
>>>> @@ -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);
>>>> +		}
>>>>    	}
>>>
>>> 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
> 
> 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.
>