Feature #9427

[PATCH] io.c: remove socket check for sendfile

Added by Eric Wong about 1 year ago. Updated about 1 year ago.

[ruby-core:59856]
Status:Closed
Priority:Normal
Assignee:Akira Tanaka

Description

Linux uses splice internally for sendfile since 2.6.23, allowing
sendfile to work for arbitrary destinations.

We gracefully handle EINVAL/ENOSYS from sendfile anyways,
so we will hit the old fallback to read/write if the system
cannot perform sendfile to non-sockets.

Verified using strace on the following one line script:
IO.copy_stream(FILE, "/dev/null")


The following changes since commit 971ef822679dfa6ee63ff83a47b4e4d1aa60d146:

  • ext/socket: Avoid unnecessary ppoll/select on Linux. Patch by Eric Wong. Bug #9039

are available in the git repository at:

git://80x24.org/ruby.git sendfile-anydest

for you to fetch changes up to 01fdf26d720a21820f4f51fade5f8b156948403b:

io.c: remove socket check for sendfile (2014-01-18 22:27:17 +0000)


Eric Wong (1):
io.c: remove socket check for sendfile

io.c | 2 --
1 file changed, 2 deletions(-)

0001-io.c-remove-socket-check-for-sendfile.patch Magnifier (908 Bytes) Eric Wong, 01/18/2014 10:46 PM

Associated revisions

Revision 44747
Added by normal about 1 year ago

io.c: remove socket check

  • io.c (nogvl_copy_stream_sendfile): remove socket check [Feature #9427]

Revision 44747
Added by normal about 1 year ago

io.c: remove socket check

  • io.c (nogvl_copy_stream_sendfile): remove socket check [Feature #9427]

Revision 44750
Added by Nobuyoshi Nakada about 1 year ago

io.c: check socket on other than linux

  • io.c (nogvl_copy_stream_sendfile): check socket on other than linux, as sendfile(2) on non-socket fd works only on linux. [Feature #9427]

Revision 44750
Added by Nobuyoshi Nakada about 1 year ago

io.c: check socket on other than linux

  • io.c (nogvl_copy_stream_sendfile): check socket on other than linux, as sendfile(2) on non-socket fd works only on linux. [Feature #9427]

History

#1 Updated by Akira Tanaka about 1 year ago

I'm afraid that this patch cause a problem on non-Linux platfroms.

#2 Updated by Eric Wong about 1 year ago

akr@fsij.org wrote:

I'm afraid that this patch cause a problem on non-Linux platfroms.

Wouldn't they fail with EINVAL? FreeBSD manpage documents it as such.

Otherwise, can we keep the S_ISSOCK check and wrap with:
#if !defined(linux) ...

I changed the S_ISSOCK check to the macro while I was at it since it's
more readable (and we define it for compatibility anyways).

http://bogomips.org/ruby.git/patch?id=e4746063070539f6d5c7
--

The following changes since commit 971ef822679dfa6ee63ff83a47b4e4d1aa60d146:

  • ext/socket: Avoid unnecessary ppoll/select on Linux. Patch by Eric Wong. Bug #9039

are available in the git repository at:

git://80x24.org/ruby.git sendfile-anydest-linux

for you to fetch changes up to e4746063070539f6d5c7216893cd2cd620d117d4:

io.c: remove socket check for sendfile on Linux (2014-01-19 02:06:46 +0000)


Eric Wong (1):
io.c: remove socket check for sendfile on Linux

io.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

#3 Updated by Akira Tanaka about 1 year ago

I see. We can add a condition later if someone find a problem on non-Linux platforms.

Please commit the patch.

#4 Updated by Anonymous about 1 year ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Applied in changeset r44747.


io.c: remove socket check

  • io.c (nogvl_copy_stream_sendfile): remove socket check [Feature #9427]

#5 Updated by Eric Wong about 1 year ago

Committed as r44747. Thanks for taking a look!

#6 Updated by Akira Tanaka about 1 year ago

  • Status changed from Closed to Feedback

It seems the patch causes problems on CentOS 5.9 (i686)

http://c5632.rubyci.org/~chkbuild/ruby-trunk/log/20140129T110302Z.diff.html.gz

 <n>)
An exception occurred during: before :each
Net::FTP#putbinaryfile when resuming an existing file and the APPE command fails raises a Net::FTPTempError when the response code is 421 ERROR
Errno::EADDRINUSE: Address already in use - bind(2) for "localhost" port 9921
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/fixtures/server.rb:<line_a>:in `initialize'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/fixtures/server.rb:<line_a>:in `new'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/fixtures/server.rb:<line_a>:in `initialize'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/shared/putbinaryfile.rb:<line_a>:in `new'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/shared/putbinaryfile.rb:<line_a>:in `block (2 levels) in <top (required)>'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/putbinaryfile_spec.rb:<line_a>:in `<top (required)>'

 <n>)
An exception occurred during: after :each
Net::FTP#putbinaryfile when resuming an existing file and the APPE command fails raises a Net::FTPTempError when the response code is 421 ERROR
IOError: closed stream
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/fixtures/server.rb:<line_a>:in `close'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/fixtures/server.rb:<line_a>:in `stop'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/shared/putbinaryfile.rb:<line_b>:in `block (2 levels) in <top (required)>'
/home/chkbuild/build/<buildtime>/rubyspec/library/net/ftp/putbinaryfile_spec.rb:<line_a>:in `<top (required)>'

...

Any idea?

#7 Updated by Eric Wong about 1 year ago

Looking into it.

#8 Updated by Eric Wong about 1 year ago

I think the EADDRINUSE was due to other problems, and it looks like the
chkbuild is passing w/o other changes:

http://c5632.rubyci.org/~chkbuild/ruby-trunk/log/20140129T230301Z.log.html.gz

nobu made r44750 which only affects non-Linux, and I just had rubyspec
pass cleanly on an old x86_64 CentOS 5.4. My only test-all failure was
TestException#test_machine_stackoverflow_by_define_method
(infinite recursion not detected)

#9 Updated by Akira Tanaka about 1 year ago

  • Status changed from Feedback to Closed

I see. It seems that the problems are sporadic.

Also available in: Atom PDF