lib/repo: Don't syncfs or fsync() dirs if fsync opt is disabled

There are use cases for not syncing at all; think build cache repos, etc. Let's
be consistent here and make sure if fsync is disabled we do no sync at all.

I chose this opportunity to add tests using the shiny new strace fault
injection.  I can forsee using this for a lot more things, so I made
the support for detecting things generic.

Related: https://github.com/ostreedev/ostree/issues/1184

Closes: #1186
Approved by: jlebon

diff --git a/ci/build.sh b/ci/build.sh
index 26e2ff37e1b21f1fe3cdf97aa64fef159c77cca9..33ef9503058aba670778676837f60d8671281891 100755 (executable)
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -10,7 +10,7 @@ pkg_upgrade
 pkg_install_builddeps ostree
 # Until this propagates farther
 pkg_install 'pkgconfig(libcurl)' 'pkgconfig(openssl)'
-pkg_install sudo which attr fuse \
+pkg_install sudo which attr fuse strace \
     libubsan libasan libtsan PyYAML redhat-rpm-config \
     elfutils
 if test -n "${CI_PKGS:-}"; then

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index e226d500297ca6dae3f6f013f3ee913dc98e7109..04349ed01e3be72e39ef094d3fb66ce5090c9d26 100644 (file)
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -1187,7 +1187,7 @@ rename_pending_loose_objects (OstreeRepo        *self,
           renamed_some_object = TRUE;
         }
 
-      if (renamed_some_object)
+      if (renamed_some_object && !self->disable_fsync)
         {
           /* Ensure that in the case of a power cut all the directory metadata that
              we want has reached the disk. In particular, we want this before we
@@ -1208,8 +1208,11 @@ rename_pending_loose_objects (OstreeRepo        *self,
     }
 
   /* In case we created any loose object subdirs, make sure they are on disk */
-  if (fsync (self->objects_dir_fd) == -1)
-    return glnx_throw_errno_prefix (error, "fsync");
+  if (!self->disable_fsync)
+    {
+      if (fsync (self->objects_dir_fd) == -1)
+        return glnx_throw_errno_prefix (error, "fsync");
+    }
 
   if (!glnx_tmpdir_delete (&self->commit_stagedir, cancellable, error))
     return FALSE;
@@ -1517,10 +1520,11 @@ ostree_repo_commit_transaction (OstreeRepo                  *self,
   if ((self->test_error_flags & OSTREE_REPO_TEST_ERROR_PRE_COMMIT) > 0)
     return glnx_throw (error, "OSTREE_REPO_TEST_ERROR_PRE_COMMIT specified");
 
-  /* FIXME: Added since valgrind in el7 doesn't know about
-   * `syncfs`...we should delete this later.
+  /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know
+   * about `syncfs`...we should delete this later.
    */
-  if (g_getenv ("OSTREE_SUPPRESS_SYNCFS") == NULL)
+  if (!self->disable_fsync &&
+      g_getenv ("OSTREE_SUPPRESS_SYNCFS") == NULL)
     {
       if (syncfs (self->tmp_dir_fd) < 0)
         return glnx_throw_errno_prefix (error, "syncfs");

diff --git a/tests/basic-test.sh b/tests/basic-test.sh
index 5058af1d2598c8ef4f29e4573211a4f4c0d74faa..218bc31c38e3563b72c3f448d1ca162feaaa6e2f 100644 (file)
--- a/tests/basic-test.sh
+++ b/tests/basic-test.sh
@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..$((72 + ${extra_basic_tests:-0}))"
+echo "1..$((73 + ${extra_basic_tests:-0}))"
 
 $CMD_PREFIX ostree --version > version.yaml
 python -c 'import yaml; yaml.safe_load(open("version.yaml"))'
@@ -809,8 +809,18 @@ cd ${test_tmpdir}
 rm -rf test2-checkout
 mkdir -p test2-checkout
 cd test2-checkout
-touch should-not-be-fsynced
-$OSTREE commit ${COMMIT_ARGS} -b test2 -s "Unfsynced commit" --fsync=false
+echo 'should not be fsynced' > should-not-be-fsynced
+if ! skip_one_without_strace_fault_injection; then
+    # Test that --fsync=false doesn't fsync
+    fsync_inject_error_ostree="strace -o /dev/null -f -e inject=syncfs,fsync,sync:error=EPERM ostree"
+    ${fsync_inject_error_ostree} --repo=${test_tmpdir}/repo commit ${COMMIT_ARGS} -b test2-no-fsync --fsync=false
+    # And test that we get EPERM if we inject an error
+    if ${fsync_inject_error_ostree} --repo=${test_tmpdir}/repo commit ${COMMIT_ARGS} -b test2-no-fsync 2>err.txt; then
+        fatal "fsync error injection failed"
+    fi
+    assert_file_has_content err.txt 'sync.*Operation not permitted'
+    echo "ok fsync disabled"
+fi
 
 # Run this test only as non-root user.  When run as root, the chmod
 # won't have any effect.

diff --git a/tests/libtest.sh b/tests/libtest.sh
index 5017abea47fcec2f4b822a62006b17963031c4a0..2b30e654596220e704e4ec263b47ca9033134a95 100755 (executable)
--- a/tests/libtest.sh
+++ b/tests/libtest.sh
@@ -543,6 +543,29 @@ skip_without_user_xattrs () {
     fi
 }
 
+# https://brokenpi.pe/tools/strace-fault-injection
+_have_strace_fault_injection=''
+have_strace_fault_injection() {
+    if test "${_have_strace_fault_injection}" = ''; then
+        if strace -P ${test_srcdir}/libtest-core.sh -e inject=read:retval=0 cat ${test_srcdir}/libtest-core.sh >out.txt &&
+           test '!' -s out.txt; then
+            _have_strace_fault_injection=yes
+        else
+            _have_strace_fault_injection=no
+        fi
+        rm -f out.txt
+    fi
+    test ${_have_strace_fault_injection} = yes
+}
+
+skip_one_without_strace_fault_injection() {
+    if ! have_strace_fault_injection; then
+        echo "ok # SKIP this test requires strace fault injection"
+        return 0
+    fi
+    return 1
+}
+
 skip_without_fuse () {
     fusermount --version >/dev/null 2>&1 || skip "no fusermount"