[PATCH 0/4] iowq fix

[PATCH 0/4] iowq fix

[Hao Xu]

several iowq fixes, all in theory, haven't been manually triggered.
Hao Xu (4):
  io-wq: tweak return value of io_wqe_create_worker()
  io-wq: code clean of io_wqe_create_worker()
  io-wq: fix worker->refcount when creating worker in work exit
  io-wq: fix potential race of acct->nr_workers

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

-- 
2.24.4

[PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()

[Hao Xu]

The return value of io_wqe_create_worker() should be false if we cannot
create a new worker according to the name of this function.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 382efca4812b..1b102494e970 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -267,7 +267,7 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 		return create_io_worker(wqe->wq, wqe, acct->index);
 	}
 
-	return true;
+	return false;
 }
 
 static void io_wqe_inc_running(struct io_worker *worker)
-- 
2.24.4

Re: [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()

[Jens Axboe]

On 9/11/21 1:40 PM, Hao Xu wrote:
> The return value of io_wqe_create_worker() should be false if we cannot
> create a new worker according to the name of this function.
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>  fs/io-wq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 382efca4812b..1b102494e970 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -267,7 +267,7 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>  		return create_io_worker(wqe->wq, wqe, acct->index);
>  	}
>  
> -	return true;
> +	return false;
>  }

I think this is just a bit confusing. It's not an error case, we just
didn't need to create a worker. So don't return failure, or the caller
will think that we failed while we did not.

-- 
Jens Axboe

Re: [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()

[Hao Xu]

在 2021/9/13 上午2:10, Jens Axboe 写道:
> On 9/11/21 1:40 PM, Hao Xu wrote:
>> The return value of io_wqe_create_worker() should be false if we cannot
>> create a new worker according to the name of this function.
>>
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   fs/io-wq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index 382efca4812b..1b102494e970 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -267,7 +267,7 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>>   		return create_io_worker(wqe->wq, wqe, acct->index);
>>   	}
>>   
>> -	return true;
>> +	return false;
>>   }
> 
> I think this is just a bit confusing. It's not an error case, we just
> didn't need to create a worker. So don't return failure, or the caller
> will think that we failed while we did not.
hmm, I think it is an error case----'we failed to create a new worker
since nr_worker == max_worker'. nr_worker == max_worker doesn't mean
'no need', we may meet situation describled in 4/4: max_worker is 1,
currently 1 worker is running, and we return true here:

           did_create = io_wqe_create_worker(wqe, acct);

              //*******nr_workers changes******//

           if (unlikely(!did_create)) {
                   raw_spin_lock(&wqe->lock);
                   /* fatal condition, failed to create the first worker */
                   if (!acct->nr_workers) {
                           raw_spin_unlock(&wqe->lock);
                           goto run_cancel;
                   }
                   raw_spin_unlock(&wqe->lock);
           }

we will miss the next check, but we have to do the check, since
number of workers may decrease to 0 in //******// place.

or maybe we can see the return value as 'do we create a new worker or
not', and the next code do safe check if it is false.
> 

Re: [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()

[Jens Axboe]

On 9/12/21 1:02 PM, Hao Xu wrote:
> 在 2021/9/13 上午2:10, Jens Axboe 写道:
>> On 9/11/21 1:40 PM, Hao Xu wrote:
>>> The return value of io_wqe_create_worker() should be false if we cannot
>>> create a new worker according to the name of this function.
>>>
>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>> ---
>>>   fs/io-wq.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index 382efca4812b..1b102494e970 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -267,7 +267,7 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>>>   		return create_io_worker(wqe->wq, wqe, acct->index);
>>>   	}
>>>   
>>> -	return true;
>>> +	return false;
>>>   }
>>
>> I think this is just a bit confusing. It's not an error case, we just
>> didn't need to create a worker. So don't return failure, or the caller
>> will think that we failed while we did not.
> hmm, I think it is an error case----'we failed to create a new worker
> since nr_worker == max_worker'. nr_worker == max_worker doesn't mean
> 'no need', we may meet situation describled in 4/4: max_worker is 1,

But that's not an error case in the sense of "uh oh, we need to handle
this as an error". If we're at the max worker count, the work simply has
to wait for another work to be done and process it.

> currently 1 worker is running, and we return true here:
> 
>            did_create = io_wqe_create_worker(wqe, acct);
> 
>               //*******nr_workers changes******//
> 
>            if (unlikely(!did_create)) {
>                    raw_spin_lock(&wqe->lock);
>                    /* fatal condition, failed to create the first worker */
>                    if (!acct->nr_workers) {
>                            raw_spin_unlock(&wqe->lock);
>                            goto run_cancel;
>                    }
>                    raw_spin_unlock(&wqe->lock);
>            }
> 
> we will miss the next check, but we have to do the check, since
> number of workers may decrease to 0 in //******// place.

If that happens, then the work that we have inserted has already been
run. Otherwise how else could we have dropped to zero workers?

-- 
Jens Axboe

Re: [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()

[Hao Xu]

在 2021/9/13 上午5:34, Jens Axboe 写道:
> On 9/12/21 1:02 PM, Hao Xu wrote:
>> 在 2021/9/13 上午2:10, Jens Axboe 写道:
>>> On 9/11/21 1:40 PM, Hao Xu wrote:
>>>> The return value of io_wqe_create_worker() should be false if we cannot
>>>> create a new worker according to the name of this function.
>>>>
>>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>>> ---
>>>>    fs/io-wq.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>>> index 382efca4812b..1b102494e970 100644
>>>> --- a/fs/io-wq.c
>>>> +++ b/fs/io-wq.c
>>>> @@ -267,7 +267,7 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>>>>    		return create_io_worker(wqe->wq, wqe, acct->index);
>>>>    	}
>>>>    
>>>> -	return true;
>>>> +	return false;
>>>>    }
>>>
>>> I think this is just a bit confusing. It's not an error case, we just
>>> didn't need to create a worker. So don't return failure, or the caller
>>> will think that we failed while we did not.
>> hmm, I think it is an error case----'we failed to create a new worker
>> since nr_worker == max_worker'. nr_worker == max_worker doesn't mean
>> 'no need', we may meet situation describled in 4/4: max_worker is 1,
> 
> But that's not an error case in the sense of "uh oh, we need to handle
> this as an error". If we're at the max worker count, the work simply has
> to wait for another work to be done and process it.
> 
>> currently 1 worker is running, and we return true here:
>>
>>             did_create = io_wqe_create_worker(wqe, acct);
>>
>>                //*******nr_workers changes******//
>>
>>             if (unlikely(!did_create)) {
>>                     raw_spin_lock(&wqe->lock);
>>                     /* fatal condition, failed to create the first worker */
>>                     if (!acct->nr_workers) {
>>                             raw_spin_unlock(&wqe->lock);
>>                             goto run_cancel;
>>                     }
>>                     raw_spin_unlock(&wqe->lock);
>>             }
>>
>> we will miss the next check, but we have to do the check, since
>> number of workers may decrease to 0 in //******// place.
> 
> If that happens, then the work that we have inserted has already been
> run. Otherwise how else could we have dropped to zero workers?
> 
Sorry, I see. I forgot the fix moved the place of nr_workers...
There is no problems now. Thanks for explanation, Jens.

  io_wqe_enqueue                   worker1
                                no work there and timeout
                                nr_workers--(after fix)
                                unlock(wqe->lock)
  ->insert work

  ->io_wqe_create_worker

                                ->io_worker_exit
                                  ->nr_workers--(before fix)

[PATCH 2/4] io-wq: code clean of io_wqe_create_worker()

[Hao Xu]

Remove do_create to save a local variable.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 1b102494e970..0e1288a549eb 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -246,8 +246,6 @@ static bool io_wqe_activate_free_worker(struct io_wqe *wqe,
  */
 static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 {
-	bool do_create = false;
-
 	/*
 	 * Most likely an attempt to queue unbounded work on an io_wq that
 	 * wasn't setup with any unbounded workers.
@@ -256,18 +254,15 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 		pr_warn_once("io-wq is not configured for unbound workers");
 
 	raw_spin_lock(&wqe->lock);
-	if (acct->nr_workers < acct->max_workers) {
-		acct->nr_workers++;
-		do_create = true;
+	if (acct->nr_workers == acct->max_workers) {
+		raw_spin_unlock(&wqe->lock);
+		return false;
 	}
+	acct->nr_workers++;
 	raw_spin_unlock(&wqe->lock);
-	if (do_create) {
-		atomic_inc(&acct->nr_running);
-		atomic_inc(&wqe->wq->worker_refs);
-		return create_io_worker(wqe->wq, wqe, acct->index);
-	}
-
-	return false;
+	atomic_inc(&acct->nr_running);
+	atomic_inc(&wqe->wq->worker_refs);
+	return create_io_worker(wqe->wq, wqe, acct->index);
 }
 
 static void io_wqe_inc_running(struct io_worker *worker)
-- 
2.24.4

Re: [PATCH 2/4] io-wq: code clean of io_wqe_create_worker()

[Jens Axboe]

On 9/11/21 1:40 PM, Hao Xu wrote:
> Remove do_create to save a local variable.

This one looks good, it's easier to follow as well. Applied.

-- 
Jens Axboe

Re: [PATCH 2/4] io-wq: code clean of io_wqe_create_worker()

[Hao Xu]

在 2021/9/12 上午3:40, Hao Xu 写道:
> Remove do_create to save a local variable.
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>   fs/io-wq.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 1b102494e970..0e1288a549eb 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -246,8 +246,6 @@ static bool io_wqe_activate_free_worker(struct io_wqe *wqe,
>    */
>   static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>   {
> -	bool do_create = false;
> -
>   	/*
>   	 * Most likely an attempt to queue unbounded work on an io_wq that
>   	 * wasn't setup with any unbounded workers.
> @@ -256,18 +254,15 @@ static bool io_wqe_create_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>   		pr_warn_once("io-wq is not configured for unbound workers");
>   
>   	raw_spin_lock(&wqe->lock);
> -	if (acct->nr_workers < acct->max_workers) {
> -		acct->nr_workers++;
> -		do_create = true;
> +	if (acct->nr_workers == acct->max_workers) {
> +		raw_spin_unlock(&wqe->lock);
> +		return false;
Hi Jens, would you like to tweak it to return true or would like me to
send a fix.
>   	}
> +	acct->nr_workers++;
>   	raw_spin_unlock(&wqe->lock);
> -	if (do_create) {
> -		atomic_inc(&acct->nr_running);
> -		atomic_inc(&wqe->wq->worker_refs);
> -		return create_io_worker(wqe->wq, wqe, acct->index);
> -	}
> -
> -	return false;
> +	atomic_inc(&acct->nr_running);
> +	atomic_inc(&wqe->wq->worker_refs);
> +	return create_io_worker(wqe->wq, wqe, acct->index);
>   }
>   
>   static void io_wqe_inc_running(struct io_worker *worker)
> 

[PATCH 3/4] io-wq: fix worker->refcount when creating worker in work exit

[Hao Xu]

We may enter the worker creation path from io_worker_exit(), and
refcount is already zero, which causes definite failure of worker
creation.
io_worker_exit
                              ref = 0
->io_wqe_dec_running
  ->io_queue_worker_create
    ->io_worker_get           failure since ref is 0

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 0e1288a549eb..75e79571bdfd 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -188,7 +188,9 @@ static void io_worker_exit(struct io_worker *worker)
 	list_del_rcu(&worker->all_list);
 	acct->nr_workers--;
 	preempt_disable();
+	refcount_set(&worker->ref, 1);
 	io_wqe_dec_running(worker);
+	refcount_set(&worker->ref, 0);
 	worker->flags = 0;
 	current->flags &= ~PF_IO_WORKER;
 	preempt_enable();
-- 
2.24.4

Re: [PATCH 3/4] io-wq: fix worker->refcount when creating worker in work exit

[Jens Axboe]

On 9/11/21 1:40 PM, Hao Xu wrote:
> We may enter the worker creation path from io_worker_exit(), and
> refcount is already zero, which causes definite failure of worker
> creation.
> io_worker_exit
>                               ref = 0
> ->io_wqe_dec_running
>   ->io_queue_worker_create
>     ->io_worker_get           failure since ref is 0
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>  fs/io-wq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 0e1288a549eb..75e79571bdfd 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -188,7 +188,9 @@ static void io_worker_exit(struct io_worker *worker)
>  	list_del_rcu(&worker->all_list);
>  	acct->nr_workers--;
>  	preempt_disable();
> +	refcount_set(&worker->ref, 1);
>  	io_wqe_dec_running(worker);
> +	refcount_set(&worker->ref, 0);

That kind of refcount manipulation is highly suspect. And in fact it
should not be needed, io_worker_exit() clears ->flags before going on
with worker teardown. Hence you can't hit worker creation from
io_wqe_dec_running().

-- 
Jens Axboe

Re: [PATCH 3/4] io-wq: fix worker->refcount when creating worker in work exit

[Hao Xu]

在 2021/9/12 上午6:13, Jens Axboe 写道:
> On 9/11/21 1:40 PM, Hao Xu wrote:
>> We may enter the worker creation path from io_worker_exit(), and
>> refcount is already zero, which causes definite failure of worker
>> creation.
>> io_worker_exit
>>                                ref = 0
>> ->io_wqe_dec_running
>>    ->io_queue_worker_create
>>      ->io_worker_get           failure since ref is 0
>>
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   fs/io-wq.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index 0e1288a549eb..75e79571bdfd 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -188,7 +188,9 @@ static void io_worker_exit(struct io_worker *worker)
>>   	list_del_rcu(&worker->all_list);
>>   	acct->nr_workers--;
>>   	preempt_disable();
>> +	refcount_set(&worker->ref, 1);
>>   	io_wqe_dec_running(worker);
>> +	refcount_set(&worker->ref, 0);
> 
> That kind of refcount manipulation is highly suspect. And in fact it
> should not be needed, io_worker_exit() clears ->flags before going on
> with worker teardown. Hence you can't hit worker creation from
> io_wqe_dec_running().
Doesn't see the relationship between worker->flags and the creation.
But yes, the creation path does io_worker_get() which causes failure
if it's from io_worker_exit(), Now I understand it is more like a
feature, isn't it? Anyway, the issue in 4/4 seems still there.

Regards,
Hao
> 

Re: [PATCH 3/4] io-wq: fix worker->refcount when creating worker in work exit

[Jens Axboe]

On 9/12/21 3:04 AM, Hao Xu wrote:
> 在 2021/9/12 上午6:13, Jens Axboe 写道:
>> On 9/11/21 1:40 PM, Hao Xu wrote:
>>> We may enter the worker creation path from io_worker_exit(), and
>>> refcount is already zero, which causes definite failure of worker
>>> creation.
>>> io_worker_exit
>>>                                ref = 0
>>> ->io_wqe_dec_running
>>>    ->io_queue_worker_create
>>>      ->io_worker_get           failure since ref is 0
>>>
>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>> ---
>>>   fs/io-wq.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index 0e1288a549eb..75e79571bdfd 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -188,7 +188,9 @@ static void io_worker_exit(struct io_worker *worker)
>>>   	list_del_rcu(&worker->all_list);
>>>   	acct->nr_workers--;
>>>   	preempt_disable();
>>> +	refcount_set(&worker->ref, 1);
>>>   	io_wqe_dec_running(worker);
>>> +	refcount_set(&worker->ref, 0);
>>
>> That kind of refcount manipulation is highly suspect. And in fact it
>> should not be needed, io_worker_exit() clears ->flags before going on
>> with worker teardown. Hence you can't hit worker creation from
>> io_wqe_dec_running().
> Doesn't see the relationship between worker->flags and the creation.
> But yes, the creation path does io_worker_get() which causes failure
> if it's from io_worker_exit(), Now I understand it is more like a
> feature, isn't it? Anyway, the issue in 4/4 seems still there.

Right, that's on purpose. In any case, the above would fail miserably
if it raced with someone trying to get a reference on the worker:

A				B
refcount_set(ref, 1)
				io_worker_get(), succeeds now 2
refcount_set(ref, 0)
oops...

-- 
Jens Axboe

[PATCH 4/4] io-wq: fix potential race of acct->nr_workers

[Hao Xu]

Given max_worker is 1, and we currently have 1 running and it is
exiting. There may be race like:
 io_wqe_enqueue                   worker1
                               no work there and timeout
                               unlock(wqe->lock)
 ->insert work
                               -->io_worker_exit
 lock(wqe->lock)
 ->if(!nr_workers) //it's still 1
 unlock(wqe->lock)
    goto run_cancel
                                  lock(wqe->lock)
                                  nr_workers--
                                  ->dec_running
                                    ->worker creation fails
                                  unlock(wqe->lock)

We enqueued one work but there is no workers, causes hung.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 75e79571bdfd..b84dc8df6c68 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -176,7 +176,6 @@ static void io_worker_ref_put(struct io_wq *wq)
 static void io_worker_exit(struct io_worker *worker)
 {
 	struct io_wqe *wqe = worker->wqe;
-	struct io_wqe_acct *acct = io_wqe_get_acct(worker);
 
 	if (refcount_dec_and_test(&worker->ref))
 		complete(&worker->ref_done);
@@ -186,7 +185,6 @@ static void io_worker_exit(struct io_worker *worker)
 	if (worker->flags & IO_WORKER_F_FREE)
 		hlist_nulls_del_rcu(&worker->nulls_node);
 	list_del_rcu(&worker->all_list);
-	acct->nr_workers--;
 	preempt_disable();
 	refcount_set(&worker->ref, 1);
 	io_wqe_dec_running(worker);
@@ -571,6 +569,7 @@ static int io_wqe_worker(void *data)
 		}
 		/* timed out, exit unless we're the last worker */
 		if (last_timeout && acct->nr_workers > 1) {
+			acct->nr_workers--;
 			raw_spin_unlock(&wqe->lock);
 			__set_current_state(TASK_RUNNING);
 			break;
-- 
2.24.4

Re: [PATCH 4/4] io-wq: fix potential race of acct->nr_workers

[Jens Axboe]

On 9/11/21 1:40 PM, Hao Xu wrote:
> Given max_worker is 1, and we currently have 1 running and it is
> exiting. There may be race like:
>  io_wqe_enqueue                   worker1
>                                no work there and timeout
>                                unlock(wqe->lock)
>  ->insert work
>                                -->io_worker_exit
>  lock(wqe->lock)
>  ->if(!nr_workers) //it's still 1
>  unlock(wqe->lock)
>     goto run_cancel
>                                   lock(wqe->lock)
>                                   nr_workers--
>                                   ->dec_running
>                                     ->worker creation fails
>                                   unlock(wqe->lock)
> 
> We enqueued one work but there is no workers, causes hung.

This one also looks good, applied.

-- 
Jens Axboe