Messages in this thread | | | Date | Tue, 26 Jun 2012 15:20:09 -0500 | From | Jonathan Nieder <> | Subject | Re: [PATCH] fifo: Do not restart open() if it already found a partner |
| |
Hi,
Anders Kaseorg wrote:
> The following test demonstrates the EINTR that was wrongly thrown from > the parent’s open(). Change .sa_flags = 0 to .sa_flags = SA_RESTART > to see a deadlock instead, in which the restarted open() waits for a > second reader that will never come.
Nice.
To recap, reading a fifo without a writer (resp. when writing a fifo without a reader), fifo_open() without O_NONBLOCK waits for the other end to be opened:
if (!pipe->writers) { if ((filp->f_flags & O_NONBLOCK)) { ... } else { wait_for_partner(inode, &pipe->w_counter);
The wait_for_partner() function waits for the pipe to be opened. It is interruptible. Inlining a little for clarity[*]:
int cur = pipe->w_counter;
while (cur == pipe->w_counter) { DEFINE_WAIT(wait);
prepare_to_wait(&pipe->wait, &wait, TASK_INTERRUPTIBLE); pipe_unlock(pipe); schedule(); finish_wait(&pipe->wait, &wait); pipe_lock(pipe);
if (signal_pending(current)) break; }
In the window while i_mutex is unlocked, it is possible for the fifo to be opened for writing and closed again. That's fine --- open() should succeed as long as someone has successfully opened the pipe for writing. And similarly, if a writer opens the pipe and a signal arrives, we should let open() succeed. The rest of fifo_open() is quick and the signal will still be promptly delivered afterwards. So this looks like a good patch.
Two small details:
1. I wasn't able to get your testcase to reliably fail (on 3.0.36). Sometimes it would produce EINTR quickly and sometimes it prefers to spew out dots. Perhaps a note would help avoid confusing people that want to see if their kernel is affected.
2. The return value convention surprised me a little:
-static void wait_for_partner(struct inode* inode, unsigned int *cnt) +static bool wait_for_partner(struct inode* inode, unsigned int *cnt)
It would be easier to guess the sense at a glance if it returned an int (e.g., 0 or -ERESTARTSYS) so the caller could look like
if (wait_for_partner(inode, &pipe->w_counter)) /* wait failed */ ...
Documentation/CodingStyle says more about that in chapter 16.
Except for those two details, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for a pleasant read.
[*] This is almost the same as
int dummy; wait_event_interruptible(&pipe->wait, pipe->w_counter != cur, dummy);
except that we keep i_mutex held when placing the current task on the waitqueue. There's probably some simplification possible, but that's a story for another day. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |