Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checkout/commit: Use glnx_regfile_copy_bytes() if possible #817

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

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.

@cgwalters
Copy link
Member Author

cgwalters commented Apr 27, 2017

Couldn't help writing this after I'd been spending some time staring at strace output for #814 .

Before:

25056 openat(8, "ef/10923868386f564a1b63d0e261a8c3353ab470f69df2fe3b6298286655f09f.file", O_RDONLY|O_CLOEXEC) = 15
25056 lseek(15, 0, SEEK_CUR)            = 0
25056 openat(14, ".", O_WRONLY|O_DIRECTORY|O_CLOEXEC|O_TMPFILE, 0600) = -1 EOPNOTSUPP (Operation not supported)
25056 openat(14, "./tmp.y9lgUn", O_WRONLY|O_CREAT|O_EXCL|O_NOCTTY|O_NOFOLLOW|O_CLOEXEC, 0600) = 16
25056 lseek(16, 0, SEEK_CUR)            = 0
25056 poll([{fd=15, events=POLLIN}], 1, -1) = 1 ([{fd=15, revents=POLLIN}])
25056 read(15, "NAME=TestOS\nVERSION=42\nID=testos"..., 8192) = 71
25056 poll([{fd=16, events=POLLOUT}], 1, -1) = 1 ([{fd=16, revents=POLLOUT}])
25056 write(16, "NAME=TestOS\nVERSION=42\nID=testos"..., 71) = 71
25056 poll([{fd=15, events=POLLIN}], 1, -1) = 1 ([{fd=15, revents=POLLIN}])
25056 read(15, "", 8192)                = 0
25056 fchown(16, 1000, 1000)            = 0
25056 fchmod(16, 0100664)               = 0
25056 renameat2(14, "./tmp.y9lgUn", 14, "os-release", RENAME_NOREPLACE) = 0
25056 close(16)                         = 0
25056 close(15)      

After:

3778  openat(8, "ef/10923868386f564a1b63d0e261a8c3353ab470f69df2fe3b6298286655f09f.file", O_RDONLY|O_CLOEXEC) = 15
3778  lseek(15, 0, SEEK_CUR)            = 0
3778  openat(14, ".", O_WRONLY|O_DIRECTORY|O_CLOEXEC|O_TMPFILE, 0600) = -1 EOPNOTSUPP (Operation not supported)
3778  openat(14, "./tmp.QDjBhn", O_WRONLY|O_CREAT|O_EXCL|O_NOCTTY|O_NOFOLLOW|O_CLOEXEC, 0600) = 16
3778  sendfile(16, 15, NULL, 71)        = 71
3778  fchown(16, 1000, 1000)            = 0
3778  fchmod(16, 0100664)               = 0
3778  renameat2(14, "./tmp.QDjBhn", 14, "os-release", RENAME_NOREPLACE) = 0
3778  close(16)                         = 0
3778  close(15)   

@cgwalters
Copy link
Member Author

bot, retest this please

@cgwalters cgwalters force-pushed the glnx-copy branch 2 times, most recently from 426ea19 to aae7945 Compare May 1, 2017 20:26
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
Copy link
Member Author

Interesting; so the problem was that I think rhci was relying on github's auto-merge commit, rather than doing what homu would do via a rebase. A bit of a messy corner case; rhci would have to know that the repo was homu managed.

But hopefully we can avoid that by relying on a FF-able PR, let's see.

@jlebon
Copy link
Member

jlebon commented May 10, 2017

bot, retest this please

if (g_output_stream_splice (output, input, 0,
cancellable, error) < 0)
return FALSE;
if (G_IS_FILE_DESCRIPTOR_BASED (input))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this condition ever fail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expose as public API:

gboolean      ostree_repo_write_content (OstreeRepo       *self,
                                         const char       *expected_checksum,
                                         GInputStream     *object_input,
                                         guint64           length,
                                         guchar          **out_csum,
                                         GCancellable     *cancellable,
                                         GError          **error);

So we have to support it. In practice using the command line tools, we will go through this code path when pulling from an archive repo to a bare one, since the content objects are zlib-compressed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, that makes sense.

cancellable, error) < 0)
return FALSE;
if (fchmod (temp_fd, 0644) < 0)
if (G_IS_FILE_DESCRIPTOR_BASED (input))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too. Seems like they're always fd-based, no? I'm not very familiar with GIO.

@jlebon
Copy link
Member

jlebon commented May 10, 2017

@rh-atomic-bot r+ 4e09112

@rh-atomic-bot
Copy link

⌛ Testing commit 4e09112 with merge 63497c6...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 63497c6 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants