lib/repo: Add a DEVINO_CANONICAL commit modifier flag

I was seeing the `Writing OSTree commit...` phase of rpm-ostree
being very slow lately.  This turns out to be more fallout from
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536

Loading the xattrs is slow on my system (F27AW, XFS+LVM, NVMe). I haven't fully
traced through why, but AIUI at least on XFS the xattrs are often stored outside
of the inode so it's a little bit like doing an `open()+read()`. Plus there's
the LSM overhead, etc.

The thing is that for rpm-ostree's package layering use case, we
basically always want to treat the on-disk state as canonical.  (There's
a subtle case here if one does overrides for something that contains
policy but we'll fix that).

Anyways, so we're in a state now where we do the slow but correct thing by
default, which seems sane. But let's allow the app to opt-in to telling us
"really trust devino". The difference between a `stat()` + hash table lookup
versus the full xattr load on my test case of `rpm-ostree install
./tree-1.7.0-10.fc27.x86_64.rpm` is absolutely dramatic; consistently on the
order of 10s without this support, and <1s with (800ms).

Closes: #1357
Approved by: jlebon

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index e2fdf9f4f0f03b111a17dea763fe8be0356d21ef..a286e7adc76abb52c11ef3b40aa6dea6456f7c37 100644 (file)
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -2793,48 +2793,71 @@ write_directory_content_to_mtree_internal (OstreeRepo                  *self,
   g_assert (dir_enum != NULL || dfd_iter != NULL);
 
   GFileType file_type = g_file_info_get_file_type (child_info);
-
   const char *name = g_file_info_get_name (child_info);
-  g_ptr_array_add (path, (char*)name);
 
-  g_autofree char *child_relpath = ptrarray_path_join (path);
-
-  /* See if we have a devino hit; this is used below. Further, for bare-user
-   * repos we'll reload our file info from the object (specifically the
-   * ostreemeta xattr). The on-disk state is not normally what we want to
-   * commit. Basically we're making sure that we pick up "real" uid/gid and any
-   * xattrs there.
+  /* Load flags into boolean constants for ease of readability (we also need to
+   * NULL-check modifier)
+   */
+  const gboolean canonical_permissions = modifier &&
+    (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS);
+  const gboolean devino_canonical = modifier &&
+    (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL);
+  /* We currently only honor the CONSUME flag in the dfd_iter case to avoid even
+   * more complexity in this function, and it'd mostly only be useful when
+   * operating on local filesystems anyways.
    */
+  const gboolean delete_after_commit = dfd_iter && modifier &&
+    (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME);
+
+  /* See if we have a devino hit; this is used below in a few places. */
   const char *loose_checksum = NULL;
-  g_autoptr(GVariant) source_xattrs = NULL;
   if (dfd_iter != NULL && (file_type != G_FILE_TYPE_DIRECTORY))
     {
       guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device");
       guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode");
       loose_checksum = devino_cache_lookup (self, modifier, dev, inode);
-      if (loose_checksum && self->mode == OSTREE_REPO_MODE_BARE_USER)
+      if (loose_checksum && devino_canonical)
         {
-          child_info = NULL;
-          if (!ostree_repo_load_file (self, loose_checksum, NULL, &child_info, &source_xattrs,
-                                      cancellable, error))
+          /* Go directly to checksum, do not pass Go, do not collect $200.
+           * In this mode the app is required to break hardlinks for any
+           * files it wants to modify.
+           */
+          if (!ostree_mutable_tree_replace_file (mtree, name, loose_checksum, error))
             return FALSE;
+          if (delete_after_commit)
+            {
+              if (!glnx_shutil_rm_rf_at (dfd_iter->fd, name, cancellable, error))
+                return FALSE;
+            }
+          return TRUE; /* Early return */
         }
     }
 
+  /* Build the full path which we need for callbacks */
+  g_ptr_array_add (path, (char*)name);
+  g_autofree char *child_relpath = ptrarray_path_join (path);
+
+  /* For bare-user repos we'll reload our file info from the object
+   * (specifically the ostreemeta xattr), if it was checked out that way (via
+   * hardlink). The on-disk state is not normally what we want to commit.
+   * Basically we're making sure that we pick up "real" uid/gid and any xattrs
+   * there.
+   */
+  g_autoptr(GVariant) source_xattrs = NULL;
+  if (loose_checksum && self->mode == OSTREE_REPO_MODE_BARE_USER)
+    {
+      child_info = NULL;
+      if (!ostree_repo_load_file (self, loose_checksum, NULL, &child_info, &source_xattrs,
+                                  cancellable, error))
+        return FALSE;
+    }
+
+  /* Call the filter */
   g_autoptr(GFileInfo) modified_info = NULL;
   OstreeRepoCommitFilterResult filter_result =
     _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info);
   const gboolean child_info_was_modified = !_ostree_gfileinfo_equal (child_info, modified_info);
 
-  /* We currently only honor the CONSUME flag in the dfd_iter case to avoid even
-   * more complexity in this function, and it'd mostly only be useful when
-   * operating on local filesystems anyways.
-   */
-  const gboolean delete_after_commit = dfd_iter && modifier &&
-    (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME);
-  const gboolean canonical_permissions = modifier &&
-    (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS);
-
   if (filter_result != OSTREE_REPO_COMMIT_FILTER_ALLOW)
     {
       g_ptr_array_remove_index (path, path->len - 1);

diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h
index bec43c30ab1b2fbf1d89a66eb9989763db4108d0..db54f022e70516f595d77b38a25101ef939773cd 100644 (file)
--- a/src/libostree/ostree-repo.h
+++ b/src/libostree/ostree-repo.h
@@ -644,6 +644,7 @@ typedef OstreeRepoCommitFilterResult (*OstreeRepoCommitFilter) (OstreeRepo    *r
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions for bare-user-only mode.
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED: Emit an error if configured SELinux policy does not provide a label
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME: Delete added files/directories after commit; Since: 2017.13
+ * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL: If a devino cache hit is found, skip modifier filters (non-directories only); Since: 2017.14
  */
 typedef enum {
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_NONE = 0,
@@ -652,6 +653,7 @@ typedef enum {
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS = (1 << 2),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED = (1 << 3),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME = (1 << 4),
+  OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL = (1 << 5),
 } OstreeRepoCommitModifierFlags;
 
 /**

diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c
index a8eb79aa087439e05d0d84c28aa16661da94e817..c24e06c7c96691b00c186f68e312c6c89755f78e 100644 (file)
--- a/src/ostree/ot-builtin-commit.c
+++ b/src/ostree/ot-builtin-commit.c
@@ -51,6 +51,7 @@ static gboolean opt_no_xattrs;
 static char *opt_selinux_policy;
 static gboolean opt_canonical_permissions;
 static gboolean opt_consume;
+static gboolean opt_devino_canonical;
 static char **opt_trees;
 static gint opt_owner_uid = -1;
 static gint opt_owner_gid = -1;
@@ -98,6 +99,7 @@ static GOptionEntry options[] = {
   { "no-xattrs", 0, 0, G_OPTION_ARG_NONE, &opt_no_xattrs, "Do not import extended attributes", NULL },
   { "selinux-policy", 0, 0, G_OPTION_ARG_FILENAME, &opt_selinux_policy, "Set SELinux labels based on policy in root filesystem PATH (may be /)", "PATH" },
   { "link-checkout-speedup", 0, 0, G_OPTION_ARG_NONE, &opt_link_checkout_speedup, "Optimize for commits of trees composed of hardlinks into the repository", NULL },
+  { "devino-canonical", 'I', 0, G_OPTION_ARG_NONE, &opt_devino_canonical, "Assume hardlinked objects are unmodified.  Implies --link-checkout-speedup", NULL },
   { "tar-autocreate-parents", 0, 0, G_OPTION_ARG_NONE, &opt_tar_autocreate_parents, "When loading tar archives, automatically create parent directories as needed", NULL },
   { "tar-pathname-filter", 0, 0, G_OPTION_ARG_STRING, &opt_tar_pathname_filter, "When loading tar archives, use REGEX,REPLACEMENT against path names", "REGEX,REPLACEMENT" },
   { "skip-if-unchanged", 0, 0, G_OPTION_ARG_NONE, &opt_skip_if_unchanged, "If the contents are unchanged from previous commit, do nothing", NULL },
@@ -480,6 +482,11 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS;
   if (opt_consume)
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME;
+  if (opt_devino_canonical)
+    {
+      opt_link_checkout_speedup = TRUE; /* Imply this */
+      flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL;
+    }
   if (opt_canonical_permissions)
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS;
   if (opt_generate_sizes)

diff --git a/tests/test-basic-user.sh b/tests/test-basic-user.sh
index bc08b65afdac104dad137a5efbd4db32e14925f2..7f970b5c81c3e6966c131ae4615a24f0f327fd87 100755 (executable)
--- a/tests/test-basic-user.sh
+++ b/tests/test-basic-user.sh
@@ -25,7 +25,7 @@ skip_without_user_xattrs
 
 setup_test_repository "bare-user"
 
-extra_basic_tests=5
+extra_basic_tests=6
 . $(dirname $0)/basic-test.sh
 
 # Reset things so we don't inherit a lot of state from earlier tests
@@ -99,3 +99,23 @@ assert_file_has_content ls.txt '^-007.. 0 0 .*/usr/bin/systemd'
 $OSTREE ls rootfs /usr/lib/dbus-daemon-helper >ls.txt
 assert_file_has_content ls.txt '^-007.. 0 81 .*/usr/lib/dbus-daemon-helper'
 echo "ok bare-user link-checkout-speedup maintains uids"
+
+cd ${test_tmpdir}
+rm -rf test2-checkout
+$OSTREE checkout -H -U test2 test2-checkout
+# With --link-checkout-speedup, specifying --owner-uid should "win" by default.
+myid=$(id -u)
+newid=$((${myid} + 1))
+$OSTREE commit ${COMMIT_ARGS} --owner-uid ${newid} --owner-gid ${newid} \
+        --link-checkout-speedup -b test2-linkcheckout-test --tree=dir=test2-checkout
+$OSTREE ls test2-linkcheckout-test /baz/cow > ls.txt
+assert_file_has_content ls.txt "^-006.. ${newid} ${newid} .*/baz/cow"
+
+# But --devino-canonical should override that
+$OSTREE commit ${COMMIT_ARGS} --owner-uid ${newid} --owner-gid ${newid} \
+        -I -b test2-devino-test --tree=dir=test2-checkout
+$OSTREE ls test2-devino-test /baz/cow > ls.txt
+assert_file_has_content ls.txt "^-006.. ${myid} ${myid} .*/baz/cow"
+
+$OSTREE refs --delete test2-{linkcheckout,devino}-test
+echo "ok commit with -I"