fdio: Expose glnx_regfile_copy_bytes(), rewrite: GNU style, POSIX errno #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In ostree in a few places we use `g_output_stream_splice()`. I thought this would use `splice()`, but actually it doesn't today. They also, if a cancellable is provided, end up dropping into `poll()` for every read and write. (In addition to copying data to/from userspace). My opinion on this is - for *local files* that's dumb. In the big picture, you really only need cancellation when copying gigabytes. Down the line, we could perhaps add a `glnx_copy_bytes_cancellable()` that only did that check e.g. every gigabyte of copied data. And when we do that we should use `g_cancellable_set_error_if_cancelled()` rather than a `poll()` with the regular file FD, since regular files are *always* readable and writable. For my use case with rpm-ostree though, we don't have gigabyte sized files, and seeing all of the `poll()` calls in strace is annoying. So let's have the non-cancellable file copying API that's modern and uses both reflink and `sendfile()` if available, in that order. My plan at some point once this is tested more is to migrate this code into GLib.
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
Apr 27, 2017
Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example.
|
Thank you for contributing to libglnx! libglnx uses Bugzilla for code review. If you have never contributed to GNOME before make sure you have read the Otherwise please visit |
|
Looks sane to me. Note though that flatpak at least will need to be updated on how it handles the return value of |
|
Good catch on flatpak! I'll submit a PR to update it after this, otherwise we'll forget. |
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
May 1, 2017
Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
May 1, 2017
Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
May 1, 2017
Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx
rh-atomic-bot
pushed a commit
to ostreedev/ostree
that referenced
this pull request
May 10, 2017
Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the #797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx Closes: #817 Approved by: jlebon
mightysai1997
pushed a commit
to mightysai1997/systemd
that referenced
this pull request
Apr 19, 2024
A while ago I copied a version of this code into libglnx, and was recently doing some work on it: GNOME/libglnx#44 While there I noticed that if a maximum size was provided, we could end up doing a zero-sized `sendfile()` call which is obviously pointless. If we have nothing to do at the of the loop, just break out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In ostree in a few places we use
g_output_stream_splice(). Ithought this would use
splice(), but actually it doesn't today.They also, if a cancellable is provided, end up dropping into
poll()for everyread and write. (In addition to copying data to/from userspace).
My opinion on this is - for local files that's dumb. In the big picture, you
really only need cancellation when copying gigabytes. Down the line, we could
perhaps add a
glnx_copy_bytes_cancellable()that only did that check e.g.every gigabyte of copied data. And when we do that we should use
g_cancellable_set_error_if_cancelled()rather than apoll()with the regularfile FD, since regular files are always readable and writable.
For my use case with rpm-ostree though, we don't have gigabyte sized files, and
seeing all of the
poll()calls in strace is annoying. So let's have thenon-cancellable file copying API that's modern and uses both reflink and
sendfile()if available, in that order.My plan at some point once this is tested more is to migrate this code
into GLib.