Skip to content

Commit

Permalink
commit: Try reflinks for local commits by default
Browse files Browse the repository at this point in the history
I think we originally used to do this, but at some point in a
code refactoring, this optimization got lost.

It's a quite important optimization for the case of writing content
generated by an external system into an ostree repository.
  • Loading branch information
cgwalters committed Dec 4, 2023
1 parent 09e32d6 commit 212c4d9
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 35 deletions.
74 changes: 39 additions & 35 deletions src/libostree/ostree-repo-commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -586,11 +586,11 @@ _ostree_repo_bare_content_cleanup (OstreeRepoBareContent *regwrite)
}

/* Allocate an O_TMPFILE, write everything from @input to it, but
* not exceeding @length. Used for every object in archive repos,
* and content objects in all bare-type repos.
* not exceeding @length.
*/
static gboolean
create_regular_tmpfile_linkable_with_content (OstreeRepo *self, guint64 length, GInputStream *input,
create_regular_tmpfile_linkable_with_content (OstreeRepo *self, guint64 length,
GInputStream *original_input, GInputStream *input,
GLnxTmpfile *out_tmpf, GCancellable *cancellable,
GError **error)
{
Expand All @@ -601,40 +601,44 @@ create_regular_tmpfile_linkable_with_content (OstreeRepo *self, guint64 length,
error))
return FALSE;

if (!glnx_try_fallocate (tmpf.fd, 0, length, error))
return FALSE;

if (G_IS_FILE_DESCRIPTOR_BASED (input))
// Try to do a reflink if possible; if we hit this case we're operating on trusted local input.
gboolean did_clone = FALSE;
if (G_IS_FILE_DESCRIPTOR_BASED (original_input))
{
int infd = g_file_descriptor_based_get_fd ((GFileDescriptorBased *)input);
if (glnx_regfile_copy_bytes (infd, tmpf.fd, (off_t)length) < 0)
return glnx_throw_errno_prefix (error, "regfile copy");
int infd = g_file_descriptor_based_get_fd ((GFileDescriptorBased *)original_input);
if (ioctl (tmpf.fd, FICLONE, infd) == 0)
{
did_clone = TRUE;
}
}
else
{
/* We used to do a g_output_stream_splice(), but there are two issues with that:
* - We want to honor the size provided, to avoid malicious content that says it's
* e.g. 10 bytes but is actually gigabytes.
* - Due to GLib bugs that pointlessly calls `poll()` on the output fd for every write
*/
gsize buf_size = MIN (length, 1048576);
g_autofree gchar *buf = g_malloc (buf_size);
guint64 remaining = length;
while (remaining > 0)
{
const gssize bytes_read
= g_input_stream_read (input, buf, MIN (remaining, buf_size), cancellable, error);
if (bytes_read < 0)
return FALSE;
else if (bytes_read == 0)
return glnx_throw (error,
"Unexpected EOF with %" G_GUINT64_FORMAT "/%" G_GUINT64_FORMAT
" bytes remaining",
remaining, length);
if (glnx_loop_write (tmpf.fd, buf, bytes_read) < 0)
return glnx_throw_errno_prefix (error, "write");
remaining -= bytes_read;
}
if (!glnx_try_fallocate (tmpf.fd, 0, length, error))
return FALSE;
}

/* We used to do a g_output_stream_splice(), but there are two issues with that:
* - We want to honor the size provided, to avoid malicious content that says it's
* e.g. 10 bytes but is actually gigabytes.
* - Due to GLib bugs that pointlessly calls `poll()` on the output fd for every write
*/
gsize buf_size = MIN (length, 1048576);
g_autofree gchar *buf = g_malloc (buf_size);
guint64 remaining = length;
while (remaining > 0)
{
const gssize bytes_read
= g_input_stream_read (input, buf, MIN (remaining, buf_size), cancellable, error);
if (bytes_read < 0)
return FALSE;
else if (bytes_read == 0)
return glnx_throw (error,
"Unexpected EOF with %" G_GUINT64_FORMAT "/%" G_GUINT64_FORMAT
" bytes remaining",
remaining, length);
if (!did_clone && glnx_loop_write (tmpf.fd, buf, bytes_read) < 0)
return glnx_throw_errno_prefix (error, "write");
remaining -= bytes_read;
}

if (!glnx_fchmod (tmpf.fd, 0644, error))
Expand Down Expand Up @@ -989,8 +993,8 @@ write_content_object (OstreeRepo *self, const char *expected_checksum, GInputStr
}
else if (repo_mode != OSTREE_REPO_MODE_ARCHIVE)
{
if (!create_regular_tmpfile_linkable_with_content (self, size, file_input, &tmpf, cancellable,
error))
if (!create_regular_tmpfile_linkable_with_content (self, size, input, file_input, &tmpf,
cancellable, error))
return FALSE;
}
else
Expand Down
11 changes: 11 additions & 0 deletions tests/kolainst/destructive/itest-label-selinux.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@ assert_not_has_file "${testbin}"
# Make a test binary that we label as shell_exec_t on disk, but should be
# reset by --selinux-policy back to bin_t
echo 'test foo' > ${testbin}
# Extend it with some real data to help test reflinks
cat < /usr/bin/bash >> "${testbin}"
# And set its label
chcon --reference co/usr/bin/true ${testbin}
# Now at this point, it should not have shared extents
filefrag -v "${testbin}" > out.txt
assert_not_file_has_content out.txt shared
rm -f out.txt
oldcon=$(getfattr --only-values -m security.selinux ${testbin})
chcon --reference co/usr/bin/bash ${testbin}
newcon=$(getfattr --only-values -m security.selinux ${testbin})
Expand All @@ -27,6 +34,10 @@ ostree commit -b testbranch --link-checkout-speedup \
ostree ls -X testbranch /usr/bin/foo-a-generic-binary > ls.txt
assert_file_has_content ls.txt ${oldcon}
ostree fsck
# Additionally, the commit process should have reflinked our input
filefrag -v "${testbin}" > out.txt
assert_file_has_content out.txt shared
rm -f out.txt

ostree refs --delete testbranch
rm co -rf
Expand Down

0 comments on commit 212c4d9

Please sign in to comment.