commit, payload-reflink: do not write to the parent repo

reintroduce the feature that was reverted with commit:

28c7bc6d0e

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 <gscrivan@redhat.com>

Closes: #1525
Approved by: cgwalters

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 */
+  /* We have not checked yet if the destination file system supports reflinks, do it here */
-  if (g_atomic_int_get (&self->fs_support_reflink) == 0)
+  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,
+      if (!glnx_openat_rdonly (dest->repo_dir_fd, "config", TRUE, &src_fd, error))
-                                          &src_tmpf, 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)
+      if (ioctl (dest_tmpf.fd, FICLONE, src_fd) == 0)
-        g_atomic_int_set (&self->fs_support_reflink, 1);
+        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,
+_import_payload_link (OstreeRepo   *dest_repo,
-                      OstreeRepo   *source,
+                      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 */

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
+    for mnt in ${mnts:-}; do
-    test -n "${blkdev:-}" && losetup -d ${blkdev} || true
+        umount ${mnt} || true
+    done
+    for blkdev in ${blkdevs:-}; do
+        losetup -d ${blkdev} || true
+    done
 }
 trap cleanup EXIT
 
-mkdir mnt
+truncate -s 2G testblk1.img
-truncate -s 2G testblk.img
+blkdev1=$(losetup --find --show $(pwd)/testblk1.img)
-if ! blkdev=$(losetup --find --show $(pwd)/testblk.img); then
+blkdevs="${blkdev1}"
-    echo "ok # SKIP not run when cannot setup loop device"
-    exit 0
-fi
-
 # 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
+truncate -s 2G testblk2.img
-cd mnt
+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 payload link"
-echo "ok pull creates .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 <<EOF
+#!/bin/bash
+# Check that commit to an user repo that has a parent still works
+set -xeuo pipefail
+ostree --repo=child-repo init --mode=bare-user
+ostree --repo=child-repo remote add origin --set=gpg-verify=false ${origin}
+ostree --repo=child-repo config set core.parent $(pwd)/repo
+ostree --repo=child-repo commit -b test content
+EOF
+chmod a+x unpriv-child-repo.sh
+runuser -u bin ./unpriv-child-repo.sh
+find child-repo -type l -name '*.payload-link' >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