From cdaf7cd8383af6f59a70a74d7c1467d8f7b536be Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 29 Mar 2018 17:10:41 +0200 Subject: [PATCH] commit, payload-reflink: do not write to the parent repo reintroduce the feature that was reverted with commit: 28c7bc6d0e153a0b07bdb82d25473a490765067f Differently than the original implementation, now we don't attempt any test for reflinks support on the parent repository, since the test requires write access to the repository. Additionally, also check that the two repositories are on the same device before attempting any reflink. Signed-off-by: Giuseppe Scrivano Closes: #1525 Approved by: cgwalters --- src/libostree/ostree-repo-commit.c | 51 +++++++----- .../nondestructive/itest-payload-link.sh | 83 +++++++++++++++---- 2 files changed, 98 insertions(+), 36 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 5fd97902..c171b3da 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -598,27 +598,26 @@ create_regular_tmpfile_linkable_with_content (OstreeRepo *self, } static gboolean -_check_support_reflink (OstreeRepo *self, gboolean *supported, GError **error) +_check_support_reflink (OstreeRepo *dest, gboolean *supported, GError **error) { - /* We have not checked yet if the file system supports reflinks, do it here */ - if (g_atomic_int_get (&self->fs_support_reflink) == 0) + /* We have not checked yet if the destination file system supports reflinks, do it here */ + if (g_atomic_int_get (&dest->fs_support_reflink) == 0) { - g_auto(GLnxTmpfile) src_tmpf = { 0, }; + glnx_autofd int src_fd = -1; g_auto(GLnxTmpfile) dest_tmpf = { 0, }; - if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_RDWR|O_CLOEXEC, - &src_tmpf, error)) + if (!glnx_openat_rdonly (dest->repo_dir_fd, "config", TRUE, &src_fd, error)) return FALSE; - if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC, + if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (dest), ".", O_WRONLY|O_CLOEXEC, &dest_tmpf, error)) return FALSE; - if (ioctl (dest_tmpf.fd, FICLONE, src_tmpf.fd) == 0) - g_atomic_int_set (&self->fs_support_reflink, 1); + if (ioctl (dest_tmpf.fd, FICLONE, src_fd) == 0) + g_atomic_int_set (&dest->fs_support_reflink, 1); else if (errno == EOPNOTSUPP) /* Ignore other kind of errors as they might be temporary failures */ - g_atomic_int_set (&self->fs_support_reflink, -1); + g_atomic_int_set (&dest->fs_support_reflink, -1); } - *supported = g_atomic_int_get (&self->fs_support_reflink) >= 0; + *supported = g_atomic_int_get (&dest->fs_support_reflink) >= 0; return TRUE; } @@ -631,6 +630,7 @@ _create_payload_link (OstreeRepo *self, GError **error) { gboolean reflinks_supported = FALSE; + if (!_check_support_reflink (self, &reflinks_supported, error)) return FALSE; @@ -664,8 +664,8 @@ _create_payload_link (OstreeRepo *self, } static gboolean -_import_payload_link (OstreeRepo *self, - OstreeRepo *source, +_import_payload_link (OstreeRepo *dest_repo, + OstreeRepo *src_repo, const char *checksum, GCancellable *cancellable, GError **error) @@ -676,20 +676,24 @@ _import_payload_link (OstreeRepo *self, glnx_unref_object OtChecksumInstream *checksum_payload = NULL; g_autoptr(GFileInfo) file_info = NULL; - if (!_check_support_reflink (self, &reflinks_supported, error)) + /* The two repositories are on different devices */ + if (src_repo->device != dest_repo->device) + return TRUE; + + if (!_check_support_reflink (dest_repo, &reflinks_supported, error)) return FALSE; if (!reflinks_supported) return TRUE; - if (!G_IN_SET(self->mode, OSTREE_REPO_MODE_BARE, OSTREE_REPO_MODE_BARE_USER, OSTREE_REPO_MODE_BARE_USER_ONLY)) + if (!G_IN_SET(dest_repo->mode, OSTREE_REPO_MODE_BARE, OSTREE_REPO_MODE_BARE_USER, OSTREE_REPO_MODE_BARE_USER_ONLY)) return TRUE; - if (!ostree_repo_load_file (source, checksum, &is, &file_info, NULL, cancellable, error)) + if (!ostree_repo_load_file (src_repo, checksum, &is, &file_info, NULL, cancellable, error)) return FALSE; if (g_file_info_get_file_type (file_info) != G_FILE_TYPE_REGULAR - || g_file_info_get_size (file_info) < self->payload_link_threshold) + || g_file_info_get_size (file_info) < dest_repo->payload_link_threshold) return TRUE; checksum_payload = ot_checksum_instream_new (is, G_CHECKSUM_SHA256); @@ -706,11 +710,12 @@ _import_payload_link (OstreeRepo *self, } payload_checksum = ot_checksum_instream_get_string (checksum_payload); - return _create_payload_link (self, checksum, payload_checksum, file_info, cancellable, error); + return _create_payload_link (dest_repo, checksum, payload_checksum, file_info, cancellable, error); } static gboolean _try_clone_from_payload_link (OstreeRepo *self, + OstreeRepo *dest_repo, const char *payload_checksum, GFileInfo *file_info, GLnxTmpfile *tmpf, @@ -722,7 +727,11 @@ _try_clone_from_payload_link (OstreeRepo *self, if (self->commit_stagedir.initialized) dfd_searches[0] = self->commit_stagedir.fd; - if (!_check_support_reflink (self, &reflinks_supported, error)) + /* The two repositories are on different devices */ + if (self->device != dest_repo->device) + return TRUE; + + if (!_check_support_reflink (dest_repo, &reflinks_supported, error)) return FALSE; if (!reflinks_supported) @@ -777,6 +786,8 @@ _try_clone_from_payload_link (OstreeRepo *self, return TRUE; } } + if (self->parent_repo) + return _try_clone_from_payload_link (self->parent_repo, dest_repo, payload_checksum, file_info, tmpf, cancellable, error); return TRUE; } @@ -1071,7 +1082,7 @@ write_content_object (OstreeRepo *self, /* Check if a file with the same payload is present in the repository, and in case try to reflink it */ - if (actual_payload_checksum && !_try_clone_from_payload_link (self, actual_payload_checksum, file_info, &tmpf, cancellable, error)) + if (actual_payload_checksum && !_try_clone_from_payload_link (self, self, actual_payload_checksum, file_info, &tmpf, cancellable, error)) return FALSE; /* This path is for regular files */ diff --git a/tests/installed/nondestructive/itest-payload-link.sh b/tests/installed/nondestructive/itest-payload-link.sh index 62b84667..6a6a01d3 100755 --- a/tests/installed/nondestructive/itest-payload-link.sh +++ b/tests/installed/nondestructive/itest-payload-link.sh @@ -30,6 +30,9 @@ date # Use /var/tmp so we have O_TMPFILE etc. prepare_tmpdir /var/tmp trap _tmpdir_cleanup EXIT +# We use this user down below, it needs access too +setfacl -d -m u:bin:rwX . +setfacl -m u:bin:rwX . ostree --repo=repo init --mode=archive echo -e '[archive]\nzlib-level=1\n' >> repo/config @@ -49,23 +52,40 @@ origin=$(cat ${test_tmpdir}/httpd-address) cleanup() { cd ${test_tmpdir} - umount mnt || true - test -n "${blkdev:-}" && losetup -d ${blkdev} || true + for mnt in ${mnts:-}; do + umount ${mnt} || true + done + for blkdev in ${blkdevs:-}; do + losetup -d ${blkdev} || true + done } trap cleanup EXIT -mkdir mnt -truncate -s 2G testblk.img -if ! blkdev=$(losetup --find --show $(pwd)/testblk.img); then - echo "ok # SKIP not run when cannot setup loop device" - exit 0 -fi - +truncate -s 2G testblk1.img +blkdev1=$(losetup --find --show $(pwd)/testblk1.img) +blkdevs="${blkdev1}" # This filesystem must support reflinks -mkfs.xfs -m reflink=1 ${blkdev} +mkfs.xfs -m reflink=1 ${blkdev1} +mkdir mnt1 +mount ${blkdev1} mnt1 +mnts=mnt1 -mount ${blkdev} mnt -cd mnt +truncate -s 2G testblk2.img +blkdev2=$(losetup --find --show $(pwd)/testblk2.img) +blkdevs="${blkdev1} ${blkdev2}" +mkfs.xfs -m reflink=1 ${blkdev2} +mkdir mnt2 +mount ${blkdev2} mnt2 +mnts="mnt1 mnt2" + +cd mnt1 +# See above for setfacl rationale +setfacl -d -m u:bin:rwX . +setfacl -m u:bin:rwX . +ls -al . +runuser -u bin mkdir foo +runuser -u bin touch foo/bar +ls -al foo # Test that reflink is really there (not just --reflink=auto) touch a @@ -78,13 +98,44 @@ ostree --repo=repo pull --disable-static-deltas origin dupobjects find repo -type l -name '*.payload-link' >payload-links.txt assert_streq "$(wc -l < payload-links.txt)" "1" -# Disable logging for inner loop -set +x cat payload-links.txt | while read i; do payload_checksum=$(basename $(dirname $i))$(basename $i .payload-link) payload_checksum_calculated=$(sha256sum $(readlink -f $i) | cut -d ' ' -f 1) assert_streq "${payload_checksum}" "${payload_checksum_calculated}" done -set -x -echo "ok pull creates .payload-link" +echo "ok payload link" + +ostree --repo=repo checkout dupobjects content +# And another object which differs just in metadata +cp --reflink=auto content/bigobject{,3} +chown operator:0 content/bigobject3 +cat >unpriv-child-repo.sh <payload-links.txt +assert_streq "$(wc -l < payload-links.txt)" "0" +rm content -rf + +echo "ok reflink unprivileged with parent repo" + +# We can't reflink across devices though +cd ../mnt2 +ostree --repo=repo init --mode=archive +ostree --repo=repo config set core.parent $(cd ../mnt1/repo && pwd) +ostree --repo=../mnt1/repo checkout dupobjects content +ostree --repo=repo commit -b dupobjects2 --consume --tree=dir=content +ostree --repo=repo pull --disable-static-deltas origin dupobjects +find repo -type l -name '*.payload-link' >payload-links.txt +assert_streq "$(wc -l < payload-links.txt)" "0" + +echo "ok payload link across devices" + date