From 39cebc67e3300c220eec49da4b6327d93c0e473e Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 22 Jun 2012 19:35:47 -0700 Subject: [PATCH] io.c: avoid rb_thread_wait_fd() if we may call rb_io_wait_readable() Blindly calling rb_thread_wait_fd() is an extra, unnecessary system call in some cases. Since we already call rb_io_wait_readable() when encountering EAGAIN, there is no user-visible change in behavior and a small potential for speedup by avoiding a system call (especially on regular filesystem that should never return EAGAIN/EINTR). This also helps avoid triggering a bugs on some buggy (ancient) Linux kernels. I encountered this issue[1] on a CentOS 5.7 machine running the 2.6.18-274.7.1.el5 kernel. Upgrading to the 2.6.18-308.8.2.el5 kernel fixes the issue. This Linux kernel regression I encountered was introduced with commit 786d7e1612f0b0adb6046f19b906609e4fe8b1ba (Jul 15 2007) and fixed in commit dd23aae4f5edf4e1dbd8f7f8013a754ba3253f48 (Sep 11 2007) I've verified the dd23aae4f5edf4e1dbd8f7f8013a754ba3253f48 change is present in the 2.6.18-308.8.2.el5 (working) kernel but not in the 2.6.18-274.7.1.el5 (broken) kernel. I realize this is a 5 year-old (fixed) bug in Linux, but I also believe Ruby is is wrong to call select()/ppoll() before encountering EAGAIN. [1] http://mid.gmane.org/20120621004402.GA7450@dcvr.yhbt.net --- io.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/io.c b/io.c index daa3aa2..74aad1b 100644 --- a/io.c +++ b/io.c @@ -1907,7 +1907,6 @@ io_bufread(char *ptr, long len, rb_io_t *fptr) } offset += c; if ((n -= c) <= 0) break; - rb_thread_wait_fd(fptr->fd); } return len - n; } @@ -1918,7 +1917,6 @@ io_bufread(char *ptr, long len, rb_io_t *fptr) offset += c; if ((n -= c) <= 0) break; } - rb_thread_wait_fd(fptr->fd); rb_io_check_closed(fptr); if (io_fillbuf(fptr) < 0) { break; @@ -4333,7 +4331,16 @@ rb_io_sysread(int argc, VALUE *argv, VALUE io) } n = fptr->fd; + + /* + * FIXME: removing rb_thread_wait_fd() here changes sysread semantics + * on non-blocking IOs. However, it's still currently possible + * for sysread to raise Errno::EAGAIN if another thread read()s + * the IO after we return from rb_thread_wait_fd() but before + * we call read() + */ rb_thread_wait_fd(fptr->fd); + rb_io_check_closed(fptr); io_setstrbuf(&str, ilen); @@ -9797,7 +9804,6 @@ copy_stream_fallback_body(VALUE arg) } else { ssize_t ss; - rb_thread_wait_fd(stp->src_fd); rb_str_resize(buf, buflen); ss = maygvl_copy_stream_read(1, stp, RSTRING_PTR(buf), l, off); if (ss == -1)