Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sun, Feb 1, 2015 at 2:14 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> It's ugly, but it actually is revealing a bug.

If so, that patch is *still* not the right thing to do. Leave the
warning. It's better than horrible crap code that doesn't warn, but
also doesn't make any *sense*.

Because code like this is just crap:

+       int running = current->state == TASK_RUNNING;

really. It's just crazy.

It makes no sense. It's all just avoiding the warning, it's not making
the code any better. In fact, it's actively making it worse, because
now the code is literally designed to give random different behavior
depending on timing.


For all I know, there might be multiple concurrent waiters etc that
got woken up on the same wait-queue, afaik, so the "am I runnable"
question is fundamentally nonsensical, since it isn't necessarily the
same as testing the state of the ring that we're waiting for anyway.
And if it *is* the same thing (because maybe the locking does end up
guaranteeing that "running" here ends up always matching some
particular state of the aio ring), then it should still be rewritten
in terms of that ring state, not some random "am I runnable" question.

So *clearly* the code has explicitly been written for the debug test,
not to make sense. Seriously. It's the only possible reason for why it
would test "current->state".

If the code relies on some 1:1 relationship with the wakeups
(*despite* having comments currently explicitly saying that it
doesn't), then use the new "wait_woken()" interface instead, which
actually does exactly that. That one doesn't test some random state,
it tests "was I explicitly woken". Then the whole "oh, but the mutex
changed the task state" doesn't even *matter*.

Which may in the end be the same thing in practice, but at least the
code makes *sense*. In ways that that "current->state == TASK_RUNNING"
test does not.

                         Linus