* [PATCH 0/2] bug fix for nr_workers @ 2021-08-08 13:54 Hao Xu 2021-08-08 13:54 ` [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally Hao Xu 2021-08-08 13:54 ` [PATCH 2/2] io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker() Hao Xu 0 siblings, 2 replies; 7+ messages in thread From: Hao Xu @ 2021-08-08 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally 2021-08-08 13:54 [PATCH 0/2] bug fix for nr_workers Hao Xu @ 2021-08-08 13:54 ` Hao Xu 2021-08-09 14:01 ` Jens Axboe 2021-08-08 13:54 ` [PATCH 2/2] io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker() Hao Xu 1 sibling, 1 reply; 7+ messages in thread From: Hao Xu @ 2021-08-08 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally 2021-08-08 13:54 ` [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally Hao Xu @ 2021-08-09 14:01 ` Jens Axboe 2021-08-09 14:08 ` Hao Xu 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2021-08-09 14:01 UTC (permalink / raw) To: Hao Xu; +Cc: io-uring, Pavel Begunkov, Joseph Qi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally 2021-08-09 14:01 ` Jens Axboe @ 2021-08-09 14:08 ` Hao Xu 2021-08-09 14:18 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Hao Xu @ 2021-08-09 14:08 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 在 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally 2021-08-09 14:08 ` Hao Xu @ 2021-08-09 14:18 ` Jens Axboe 2021-08-09 16:12 ` Hao Xu 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2021-08-09 14:18 UTC (permalink / raw) To: Hao Xu; +Cc: io-uring, Pavel Begunkov, Joseph Qi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally 2021-08-09 14:18 ` Jens Axboe @ 2021-08-09 16:12 ` Hao Xu 0 siblings, 0 replies; 7+ messages in thread From: Hao Xu @ 2021-08-09 16:12 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 在 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker() 2021-08-08 13:54 [PATCH 0/2] bug fix for nr_workers Hao Xu 2021-08-08 13:54 ` [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally Hao Xu @ 2021-08-08 13:54 ` Hao Xu 1 sibling, 0 replies; 7+ messages in thread From: Hao Xu @ 2021-08-08 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-09 16:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-08 13:54 [PATCH 0/2] bug fix for nr_workers Hao Xu 2021-08-08 13:54 ` [PATCH 1/2] io-wq: fix bug of creating io-wokers unconditionally Hao Xu 2021-08-09 14:01 ` Jens Axboe 2021-08-09 14:08 ` Hao Xu 2021-08-09 14:18 ` Jens Axboe 2021-08-09 16:12 ` Hao Xu 2021-08-08 13:54 ` [PATCH 2/2] io-wq: fix IO_WORKER_F_FIXED issue in create_io_worker() Hao Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.