On Fri, Dec 17, 2021 at 9:00 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> Just a single fix, fixing an issue with the worker creation change that
> was merged last week.
Hmm. I've pulled, but looking at the result, this is a classic no-no.
You can't just randomly drop and re-take a lock and sat it's "safe".
Because I don't think it's necessarily safe at all.
When you drop the wqe->lock in the middle of io_wqe_dec_running to
create a new worker, it means - for example - that "io_worker_exit()"
can now run immediately on the new worker as far as I can tell.
So one io_worker_exit() m,ay literally race with another one, where
both are inside that io_wqe_dec_running() at the same time. And then
they both end up doing
worker->flags = 0;
current->flags &= ~PF_IO_WORKER;
afterwards in the caller, and not necessarily in the original order.
And then they'll both possible do
kfree_rcu(worker, rcu);
which sounds like a disaster.
Maybe this is all safe and things like the above cannot happen, but it
sure is *not* obviously so. Any time you release a lock in the middle
of holding it, basically *everything* you do afterwards when you
re-take it is suspect.
Don't perpetuate this broken pattern. Because even if it happens to be
safe in this situation, it's _alweays_ broken garbage unless you have
a big and exhaustive comment talking about why re-taking it suddenly
makes everything that follows ok.
The way to do this properly is to either
(a) make the code you ran under the lock ok to run under the lock
(b) make the locked region have a *return value* that the code then
uses to decide what to do after it has actually released the lock
But the whole "release and re-take" pattern is broken, broken, broken.
As mentioned, I've pulled this, but I seriously considered unpulling.
Because I think that fix is wrong.
Linus
