On Thu, Oct 1, 2020 at 8:58 AM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> The problem is we're doing the prepare_to_wait, which sets our state
> each time, however we can be woken up either with reads or writes. In
> the case above we race with the WRITER waking us up, and re-set our
> state to INTERRUPTIBLE, and thus never break out of schedule.
Good catch of an interesting problem.
That said, the real problem here is that "pipe_wait()" is some nasty
garbage. I really wanted to convert all users to do a proper
wait-queue usage, but left the old structure alone.
Any normal "prepare_to_wait()" user should always check for the
condition that it's waiting for _after_ the prepare-to-wait call, but
the pipe code was traditionally all run under the pipe mutex, so it
should have no races at all, because it's completely invalid to use
"pipe_wait()" with anything that doesn't hold the mutex (both on the
sleeping and waking side).
So pipe_wait() is kind of legacy and meant for all the silly and
complex UNIX domain connection things that nobody wants to touch.
The IO code was supposed to have been converted away from that pipe
mutex model, but it looks like I punted on splice, without thinking
about this issue.
So I think the *real* fix would be to convert the splice waiting code
to work even without holding the pipe mutex. Because honestly, I think
your patch fixes the problem, but not completely.
In particular, the pipe wakeup can happen from somebody that doesn't
hold the pipe mutex at all (see pipe_read(), and notice how it's doing
the __pipe_unlock() before it does the "if it was full, wake things
up), so this whole sequence is racy:
check if pipe is full
pipe_wait() if it is
because the wakeup can happen in between those things, and
"pipe_wait()" has no way to know what it's waiting for, and the wakeup
event has already happened by the time it's called.
Let me think about this, but I think the right solution is to just get
rid of the splice use of pipe_wait.
Do you have some half-way reliable way to trigger the issue for testing?
Linus
