Skip to content

Commit

Permalink
lib/repo: Add a DEVINO_CANONICAL commit modifier flag
Browse files Browse the repository at this point in the history
I was seeing the `Writing OSTree commit...` phase of rpm-ostree
being very slow lately.  This turns out to be more fallout from
ostreedev#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).

No tests yet.
  • Loading branch information
cgwalters committed Dec 1, 2017
1 parent 17308e2 commit c44f01a
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 23 deletions.
69 changes: 46 additions & 23 deletions src/libostree/ostree-repo-commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -2766,48 +2766,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);
Expand Down
2 changes: 2 additions & 0 deletions src/libostree/ostree-repo.h
Original file line number Diff line number Diff line change
Expand Up @@ -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, do not compare metadata/flags, directly consume object; Since: 2017.14
*/
typedef enum {
OSTREE_REPO_COMMIT_MODIFIER_FLAGS_NONE = 0,
Expand All @@ -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;

/**
Expand Down

0 comments on commit c44f01a

Please sign in to comment.