[PATCH 0/3] code clean and nr_worker fixes

[PATCH 0/3] code clean and nr_worker fixes

[Hao Xu]

Hao Xu (3):
  io-wq: clean code of task state setting
  io-wq: fix no lock protection of acct->nr_worker
  io-wq: fix lack of acct->nr_workers < acct->max_workers judgement

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

-- 
2.24.4

[PATCH 1/3] io-wq: clean code of task state setting

[Hao Xu]

We don't need to set task state to TASK_INTERRUPTIBLE at the beginning
of while() in io_wqe_worker(), which causes state resetting to
TASK_RUNNING in other place. Move it to above schedule_timeout() and
remove redundant task state settings.

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

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 50dc93ffc153..cd4fd4d6268f 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -380,7 +380,6 @@ static void io_wait_on_hash(struct io_wqe *wqe, unsigned int hash)
 	if (list_empty(&wqe->wait.entry)) {
 		__add_wait_queue(&wq->hash->wait, &wqe->wait);
 		if (!test_bit(hash, &wq->hash->map)) {
-			__set_current_state(TASK_RUNNING);
 			list_del_init(&wqe->wait.entry);
 		}
 	}
@@ -433,7 +432,6 @@ static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
 static bool io_flush_signals(void)
 {
 	if (unlikely(test_thread_flag(TIF_NOTIFY_SIGNAL))) {
-		__set_current_state(TASK_RUNNING);
 		tracehook_notify_signal();
 		return true;
 	}
@@ -482,7 +480,6 @@ static void io_worker_handle_work(struct io_worker *worker)
 		if (!work)
 			break;
 		io_assign_current_work(worker, work);
-		__set_current_state(TASK_RUNNING);
 
 		/* handle a whole dependent link */
 		do {
@@ -538,7 +535,6 @@ static int io_wqe_worker(void *data)
 	while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) {
 		long ret;
 
-		set_current_state(TASK_INTERRUPTIBLE);
 loop:
 		raw_spin_lock_irq(&wqe->lock);
 		if (io_wqe_run_queue(wqe)) {
@@ -549,6 +545,7 @@ static int io_wqe_worker(void *data)
 		raw_spin_unlock_irq(&wqe->lock);
 		if (io_flush_signals())
 			continue;
+		set_current_state(TASK_INTERRUPTIBLE);
 		ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
 		if (signal_pending(current)) {
 			struct ksignal ksig;
-- 
2.24.4

[PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Hao Xu]

There is an acct->nr_worker visit without lock protection. Think about
the case: two callers call io_wqe_wake_worker(), one is the original
context and the other one is an io-worker(by calling
io_wqe_enqueue(wqe, linked)), on two cpus paralelly, this may cause
nr_worker to be larger than max_worker.
Let's fix it by adding lock for it, and let's do nr_workers++ before
create_io_worker. There may be a edge cause that the first caller fails
to create an io-worker, but the second caller doesn't know it and then
quit creating io-worker as well:

say nr_worker = max_worker - 1
        cpu 0                        cpu 1
   io_wqe_wake_worker()          io_wqe_wake_worker()
      nr_worker < max_worker
      nr_worker++
      create_io_worker()         nr_worker == max_worker
         failed                  return
      return

But the chance of this case is very slim.

Fixes: 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index cd4fd4d6268f..88d0ba7be1fb 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -247,9 +247,14 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
 	ret = io_wqe_activate_free_worker(wqe);
 	rcu_read_unlock();
 
-	if (!ret && acct->nr_workers < acct->max_workers) {
-		atomic_inc(&acct->nr_running);
-		atomic_inc(&wqe->wq->worker_refs);
+	if (!ret) {
+		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++;
+		}
+		raw_spin_unlock_irq(&wqe->lock);
 		create_io_worker(wqe->wq, wqe, acct->index);
 	}
 }
@@ -632,6 +637,9 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		kfree(worker);
 fail:
 		atomic_dec(&acct->nr_running);
+		raw_spin_lock_irq(&wqe->lock);
+		acct->nr_workers--;
+		raw_spin_unlock_irq(&wqe->lock);
 		io_worker_ref_put(wq);
 		return;
 	}
@@ -647,9 +655,8 @@ 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 && (worker->flags & IO_WORKER_F_BOUND))
+	if ((acct->nr_workers == 1) && (worker->flags & IO_WORKER_F_BOUND))
 		worker->flags |= IO_WORKER_F_FIXED;
-	acct->nr_workers++;
 	raw_spin_unlock_irq(&wqe->lock);
 	wake_up_new_task(tsk);
 }
-- 
2.24.4

[PATCH 3/3] io-wq: fix lack of acct->nr_workers < acct->max_workers judgement

[Hao Xu]

There should be this judgement before we create an io-worker

Fixes: 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io-wq.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 88d0ba7be1fb..b7cc31f96fdb 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -276,9 +276,17 @@ static void create_worker_cb(struct callback_head *cb)
 {
 	struct create_worker_data *cwd;
 	struct io_wq *wq;
+	struct io_wqe *wqe;
+	struct io_wqe_acct *acct;
 
 	cwd = container_of(cb, struct create_worker_data, work);
-	wq = cwd->wqe->wq;
+	wqe = cwd->wqe;
+	wq = wqe->wq;
+	acct = &wqe->acct[cwd->index];
+	raw_spin_lock_irq(&wqe->lock);
+	if (acct->nr_workers < acct->max_workers)
+		acct->nr_workers++;
+	raw_spin_unlock_irq(&wqe->lock);
 	create_io_worker(wq, cwd->wqe, cwd->index);
 	kfree(cwd);
 }
-- 
2.24.4

Re: [PATCH 1/3] io-wq: clean code of task state setting

[Jens Axboe]

On 8/5/21 4:05 AM, Hao Xu wrote:
> We don't need to set task state to TASK_INTERRUPTIBLE at the beginning
> of while() in io_wqe_worker(), which causes state resetting to
> TASK_RUNNING in other place. Move it to above schedule_timeout() and
> remove redundant task state settings.

Not sure that is safe - the reason why the state is manipulated is to
guard from races where we do:

A				B
if (!work_available)
				Work inserted
schedule();

As long as setting the task runnable is part of the work being inserted,
then the above race is fine, as the schedule() turns into a no-op.

-- 
Jens Axboe

Re: [PATCH 0/3] code clean and nr_worker fixes

[Jens Axboe]

On 8/5/21 4:05 AM, Hao Xu wrote:
> 
> Hao Xu (3):
>   io-wq: clean code of task state setting
>   io-wq: fix no lock protection of acct->nr_worker
>   io-wq: fix lack of acct->nr_workers < acct->max_workers judgement
> 
>  fs/io-wq.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)

Applied 2-3, thanks!

-- 
Jens Axboe

Re: [PATCH 1/3] io-wq: clean code of task state setting

[Hao Xu]

在 2021/8/5 下午10:23, Jens Axboe 写道:
> On 8/5/21 4:05 AM, Hao Xu wrote:
>> We don't need to set task state to TASK_INTERRUPTIBLE at the beginning
>> of while() in io_wqe_worker(), which causes state resetting to
>> TASK_RUNNING in other place. Move it to above schedule_timeout() and
>> remove redundant task state settings.
> 
> Not sure that is safe - the reason why the state is manipulated is to
> guard from races where we do:
> 
> A				B
> if (!work_available)
> 				Work inserted
> schedule();
> 
You are right, the patch does bring in races. Though B will then create 
an io-worker in that race condition, but that causes delay. Thanks Jens!
> As long as setting the task runnable is part of the work being inserted,
> then the above race is fine, as the schedule() turns into a no-op.
> 

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Jens Axboe]

On Thu, Aug 5, 2021 at 4:05 AM Hao Xu <haoxu@linux.alibaba.com> wrote:
>
> There is an acct->nr_worker visit without lock protection. Think about
> the case: two callers call io_wqe_wake_worker(), one is the original
> context and the other one is an io-worker(by calling
> io_wqe_enqueue(wqe, linked)), on two cpus paralelly, this may cause
> nr_worker to be larger than max_worker.
> Let's fix it by adding lock for it, and let's do nr_workers++ before
> create_io_worker. There may be a edge cause that the first caller fails
> to create an io-worker, but the second caller doesn't know it and then
> quit creating io-worker as well:
>
> say nr_worker = max_worker - 1
>         cpu 0                        cpu 1
>    io_wqe_wake_worker()          io_wqe_wake_worker()
>       nr_worker < max_worker
>       nr_worker++
>       create_io_worker()         nr_worker == max_worker
>          failed                  return
>       return
>
> But the chance of this case is very slim.
>
> Fixes: 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>  fs/io-wq.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index cd4fd4d6268f..88d0ba7be1fb 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -247,9 +247,14 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>         ret = io_wqe_activate_free_worker(wqe);
>         rcu_read_unlock();
>
> -       if (!ret && acct->nr_workers < acct->max_workers) {
> -               atomic_inc(&acct->nr_running);
> -               atomic_inc(&wqe->wq->worker_refs);
> +       if (!ret) {
> +               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++;
> +               }
> +               raw_spin_unlock_irq(&wqe->lock);
>                 create_io_worker(wqe->wq, wqe, acct->index);
>         }
>  }

There's a pretty grave bug in this patch, in that you no call
create_io_worker() unconditionally. This causes obvious problems with
misaccounting, and stalls that hit the idle timeout...

-- 
Jens Axboe

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Hao Xu]

在 2021/8/6 下午10:27, Jens Axboe 写道:
> On Thu, Aug 5, 2021 at 4:05 AM Hao Xu <haoxu@linux.alibaba.com> wrote:
>>
>> There is an acct->nr_worker visit without lock protection. Think about
>> the case: two callers call io_wqe_wake_worker(), one is the original
>> context and the other one is an io-worker(by calling
>> io_wqe_enqueue(wqe, linked)), on two cpus paralelly, this may cause
>> nr_worker to be larger than max_worker.
>> Let's fix it by adding lock for it, and let's do nr_workers++ before
>> create_io_worker. There may be a edge cause that the first caller fails
>> to create an io-worker, but the second caller doesn't know it and then
>> quit creating io-worker as well:
>>
>> say nr_worker = max_worker - 1
>>          cpu 0                        cpu 1
>>     io_wqe_wake_worker()          io_wqe_wake_worker()
>>        nr_worker < max_worker
>>        nr_worker++
>>        create_io_worker()         nr_worker == max_worker
>>           failed                  return
>>        return
>>
>> But the chance of this case is very slim.
>>
>> Fixes: 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   fs/io-wq.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index cd4fd4d6268f..88d0ba7be1fb 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -247,9 +247,14 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>>          ret = io_wqe_activate_free_worker(wqe);
>>          rcu_read_unlock();
>>
>> -       if (!ret && acct->nr_workers < acct->max_workers) {
>> -               atomic_inc(&acct->nr_running);
>> -               atomic_inc(&wqe->wq->worker_refs);
>> +       if (!ret) {
>> +               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++;
>> +               }
>> +               raw_spin_unlock_irq(&wqe->lock);
>>                  create_io_worker(wqe->wq, wqe, acct->index);
>>          }
>>   }
> 
> There's a pretty grave bug in this patch, in that you no call
> create_io_worker() unconditionally. This causes obvious problems with
> misaccounting, and stalls that hit the idle timeout...
> 
This is surely a silly mistake, I'll check this patch and the 3/3 again.

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Jens Axboe]

On 8/7/21 3:56 AM, Hao Xu wrote:
> 在 2021/8/6 下午10:27, Jens Axboe 写道:
>> On Thu, Aug 5, 2021 at 4:05 AM Hao Xu <haoxu@linux.alibaba.com> wrote:
>>>
>>> There is an acct->nr_worker visit without lock protection. Think about
>>> the case: two callers call io_wqe_wake_worker(), one is the original
>>> context and the other one is an io-worker(by calling
>>> io_wqe_enqueue(wqe, linked)), on two cpus paralelly, this may cause
>>> nr_worker to be larger than max_worker.
>>> Let's fix it by adding lock for it, and let's do nr_workers++ before
>>> create_io_worker. There may be a edge cause that the first caller fails
>>> to create an io-worker, but the second caller doesn't know it and then
>>> quit creating io-worker as well:
>>>
>>> say nr_worker = max_worker - 1
>>>          cpu 0                        cpu 1
>>>     io_wqe_wake_worker()          io_wqe_wake_worker()
>>>        nr_worker < max_worker
>>>        nr_worker++
>>>        create_io_worker()         nr_worker == max_worker
>>>           failed                  return
>>>        return
>>>
>>> But the chance of this case is very slim.
>>>
>>> Fixes: 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")
>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>> ---
>>>   fs/io-wq.c | 17 ++++++++++++-----
>>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index cd4fd4d6268f..88d0ba7be1fb 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -247,9 +247,14 @@ static void io_wqe_wake_worker(struct io_wqe *wqe, struct io_wqe_acct *acct)
>>>          ret = io_wqe_activate_free_worker(wqe);
>>>          rcu_read_unlock();
>>>
>>> -       if (!ret && acct->nr_workers < acct->max_workers) {
>>> -               atomic_inc(&acct->nr_running);
>>> -               atomic_inc(&wqe->wq->worker_refs);
>>> +       if (!ret) {
>>> +               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++;
>>> +               }
>>> +               raw_spin_unlock_irq(&wqe->lock);
>>>                  create_io_worker(wqe->wq, wqe, acct->index);
>>>          }
>>>   }
>>
>> There's a pretty grave bug in this patch, in that you no call
>> create_io_worker() unconditionally. This causes obvious problems with
>> misaccounting, and stalls that hit the idle timeout...
>>
> This is surely a silly mistake, I'll check this patch and the 3/3 again.

Please do - and please always run the full set of tests before sending
out changes like this, you would have seen the slower runs and/or
timeouts from the regression suite. I ended up wasting time on this
thinking it was a change I made that broke it, before then debugging
this one.

-- 
Jens Axboe

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Olivier Langlois]

On Sat, 2021-08-07 at 07:51 -0600, Jens Axboe wrote:
> 
> Please do - and please always run the full set of tests before
> sending
> out changes like this, you would have seen the slower runs and/or
> timeouts from the regression suite. I ended up wasting time on this
> thinking it was a change I made that broke it, before then debugging
> this one.
> 
Jens,

for my personal info, where is the regression suite?

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Pavel Begunkov]

On 8/9/21 9:19 PM, Olivier Langlois wrote:
> On Sat, 2021-08-07 at 07:51 -0600, Jens Axboe wrote:
>>
>> Please do - and please always run the full set of tests before
>> sending
>> out changes like this, you would have seen the slower runs and/or
>> timeouts from the regression suite. I ended up wasting time on this
>> thinking it was a change I made that broke it, before then debugging
>> this one.
>>
> Jens,
> 
> for my personal info, where is the regression suite?

liburing/tests.

There are scripts for convenience, e.g. you can run all tests once with

`make runtests`

or even better to leave them for a while executing again and again:

`make runtests-loop`

-- 
Pavel Begunkov

Re: [PATCH 2/3] io-wq: fix no lock protection of acct->nr_worker

[Jens Axboe]

On 8/9/21 2:19 PM, Olivier Langlois wrote:
> On Sat, 2021-08-07 at 07:51 -0600, Jens Axboe wrote:
>>
>> Please do - and please always run the full set of tests before
>> sending
>> out changes like this, you would have seen the slower runs and/or
>> timeouts from the regression suite. I ended up wasting time on this
>> thinking it was a change I made that broke it, before then debugging
>> this one.
>>
> Jens,
> 
> for my personal info, where is the regression suite?

It's the test/ portion of liburing, it's actually (by far) the biggest
part of that repo. For my personal use cases, I've got a bunch of
different devices (and files on various file systems) setup in the
configuration, and I run make runtests for all changes to test for known
cases that were broken at some point.

-- 
Jens Axboe