io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Hao Xu <haoxu@linux.alibaba.com>
Cc: io-uring@vger.kernel.org, Pavel Begunkov <asml.silence@gmail.com>,
	Joseph Qi <joseph.qi@linux.alibaba.com>
Subject: Re: [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker()
Date: Sun, 12 Sep 2021 15:34:46 -0600	[thread overview]
Message-ID: <06e27618-8b47-f926-5c7e-5346423006ea@kernel.dk> (raw)
In-Reply-To: <1175a5b4-5c95-ff84-22cd-355590946e87@linux.alibaba.com>

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


  reply	other threads:[~2021-09-12 21:34 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-11 19:40 [PATCH 0/4] iowq fix Hao Xu
2021-09-11 19:40 ` [PATCH 1/4] io-wq: tweak return value of io_wqe_create_worker() Hao Xu
2021-09-12 18:10   ` Jens Axboe
2021-09-12 19:02     ` Hao Xu
2021-09-12 21:34       ` Jens Axboe [this message]
2021-09-13  6:37         ` Hao Xu
2021-09-11 19:40 ` [PATCH 2/4] io-wq: code clean " Hao Xu
2021-09-12 18:18   ` Jens Axboe
2021-09-13  8:30   ` Hao Xu
2021-09-11 19:40 ` [PATCH 3/4] io-wq: fix worker->refcount when creating worker in work exit Hao Xu
2021-09-11 22:13   ` Jens Axboe
2021-09-12  9:04     ` Hao Xu
2021-09-12 18:07       ` Jens Axboe
2021-09-11 19:40 ` [PATCH 4/4] io-wq: fix potential race of acct->nr_workers Hao Xu
2021-09-12 18:23   ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=06e27618-8b47-f926-5c7e-5346423006ea@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=asml.silence@gmail.com \
    --cc=haoxu@linux.alibaba.com \
    --cc=io-uring@vger.kernel.org \
    --cc=joseph.qi@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).