GNOME Bugzilla – Bug 616852
Make more use of GFileDescriptorBased and splice()
Last modified: 2018-05-24 12:15:06 UTC
Not completely finished yet, but I wanted to get it off my hard drive. This makes g_output_stream_splice() use splice() with GFileDescriptorBased streams, and makes the remaining file-descriptor-based streams implement the interface. It also adds sendfile() support for when the output stream is a socket. That part in particular can't go in as-is, because FreeBSD also has sendfile, but with a different signature. (Solaris has something similar too.) So that will need more configure checks.
Created attachment 159604 [details] [review] g_cancellable_release_fd: allow NULL cancellable Almost all GCancellable methods silently do nothing if passed NULL for the cancellable. Make g_cancellable_release_fd() do that as well.
Created attachment 159606 [details] [review] Make more use of GFileDescriptorBased and splice() Implement GFileDescriptorBased in GUnix{Input,Output}Stream and GSocket{Input,Output}Stream. Split out the splice()-based implementation of g_file_copy() into g_file_descriptor_based_copy(), add support for using sendfile() when the destination fd is a socket, and make g_output_stream_splice() also use that code when possible. gio/tests/httpd.c was already using g_output_stream_splice() to move data between a GFileInputStream and a GSocketOutputStream. As a result of these changes, that code now results in a zero-copy transfer from the filesystem to the network.
(In reply to comment #0) > Not completely finished yet well, other than the aforementioned sendfile configure issues, the only big problem is that it's not especially tested yet (although gio/tests/httpd.c works)
Comment on attachment 159604 [details] [review] g_cancellable_release_fd: allow NULL cancellable This one is obviously fine.
I was just adding GFileDescriptorBased interfaces to some classes in a branch when i noticed this :-) Awesome Dan! The naming of g_file_descriptor_based_copy() might be problematic for bindings, it's not unthinkable that something implementing GFileDescriptorBased has a copy() function for other reasons I think. Otoh, it matches the calling semantics of g_file_copy(), so maybe that trumps the potential issue.
Review of attachment 159606 [details] [review]: ::: gio/gfiledescriptorbased.c @@ +184,3 @@ + */ + if (pipe (buffer) != 0) + return set_error_from_errno (errno, error); The do_splice() function you removed from gfile.c did not use an extra pipe() to transmit the data. To my knowledge splice() will work on any old fd, so there's really no reason for the extra pipe() here. Maybe you're thinking of vmsplice() which only works on a pipe afaik. @@ +252,3 @@ + +static gboolean +copy_via_sendfile (gint infd, Are you sure that sendfile() is more efficient than splice()? I am not fully up to date, but Linus mentioned ways back that sendfile() should be reimplemented in terms of splice() under the hood (http://kerneltrap.org/node/6505)
(In reply to comment #6) > The do_splice() function you removed from gfile.c did not use an extra pipe() > to transmit the data. To my knowledge splice() will work on any old fd The pipe() in gfile.c is created in splice_stream_with_progress(), and do_splice() is always called with one end of that pipe as one of the two arguments. The code is just reorganized in my patch. from the man page: splice() moves data between two file descriptors without copying between kernel address space and user address space. It transfers up to len bytes of data from the file descriptor fd_in to the file descriptor fd_out, where one of the descriptors must refer to a pipe. > Are you sure that sendfile() is more efficient than splice()? No, but (as seen in the new patches I'm about to attach), sendfile() is more portable and lets us get zero copy on other platforms too. Maybe we always want to use splice() with Linux though.
Created attachment 201057 [details] [review] gio: use GFileDescriptorBased/splice() for g_output_stream_splice() Split out the splice()-based implementation of g_file_copy() into g_file_descriptor_based_copy(), and make g_output_stream_splice() also use that code when possible.
Created attachment 201058 [details] [review] gio: more implementations of GFileDescriptorBased Implement GFileDescriptorBased in GSocket{Input,Output}Stream and GUnix{Input,Output}Stream.
Created attachment 201059 [details] [review] gio/tests/splice: new test of g_output_stream_splice()
Created attachment 201060 [details] [review] Add sendfile support to g_file_descriptor_based_copy() Use sendfile(), where available, when splicing from a file to a socket. gio/tests/httpd.c was already using g_output_stream_splice() to move data between a GFileInputStream and a GSocketOutputStream. As a result of these changes, that code now results in a zero-copy transfer from the filesystem to the network.
(In reply to comment #8) > Created an attachment (id=201057) [details] [review] > gio: use GFileDescriptorBased/splice() for g_output_stream_splice() There's one problem with this patch, which is that if you try to splice something to a GLocalFileOutputStream or GUnixOutputStream referring to a file which is either non-ordinary (eg, "/dev/null") or on a non-spliceable filesystem (eg, fuse), then it will fail. The fallback is slightly tricky because at that point we will have already spliced from the input stream into the temporary pipe, but the splice from the pipe to the output stream will have failed. So we have to fall back to copy_traditionally(), except that first we have to copy the data that's already in the splice pipe out, and then copy the rest of the data from the original input stream. I never got around to implementing this case though, so it's not quite ready to commit.
Comment on attachment 201058 [details] [review] gio: more implementations of GFileDescriptorBased Attachment 201058 [details] pushed as 759c0e9 - gio: more implementations of GFileDescriptorBased
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/287.