Skip to content

Commit

Permalink
lib/commit: don't query devino cache for modified files
Browse files Browse the repository at this point in the history 
We can't use the cache if the file we want to commit has been modified
by the client through the file info or xattr modifiers. We would
prematurely look into the cache in `write_dfd_iter_to_mtree_internal`,
regardless of whether any filtering applied.

We remove that path there, and make sure in
`write_directory_content_to_mtree_internal` that we only use the cache
if there were no modifications. We rename the `get_modified_xattrs` to
`get_final_xattrs` to reflect the fact that the xattrs may not be
modified.

One tricky bit that took me some time was that we now need to store the
st_dev & st_ino values in the GFileInfo because the cache lookup relies
on it. I'm guessing we regressed on this at some point.

This patch does slightly change the semantics of the xattr callback.
Previously, returning NULL from the cb meant no xattrs at all. Now, it
means to default to the on-disk state. We might want to consider putting
that behind a flag instead. It seems like a more useful behaviour so
that callers can only override the files they want to without losing
original on-disk state (and if they don't want that, just return an
empty GVariant).

The test I added here will unfortunately be skipped in our CI, though I
did verify that the test passes in an environment that supports xattrs.
We should probably add a non-overlay/non-tmpfs based tester to cover
these paths.

(I also made sure in gdb that we were hitting the cache for content for
which we *didn't* modify neither the file info nor the xattr, though I
wasn't sure offhand how to codify this check in the test.)

Closes: #1165
  • Loading branch information
@jlebon
jlebon committed Sep 13, 2017
1 parent e8b9f1d commit 7d9059c
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 61 deletions.
5 changes: 5 additions & 0 deletions src/libostree/ostree-core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1562,6 +1562,11 @@ _ostree_stbuf_to_gfileinfo (const struct stat *stbuf)
g_file_info_set_attribute_uint32 (ret, "unix::uid", stbuf->st_uid);
g_file_info_set_attribute_uint32 (ret, "unix::gid", stbuf->st_gid);
g_file_info_set_attribute_uint32 (ret, "unix::mode", mode);

/* those aren't stored by ostree, but used by the devino cache */
g_file_info_set_attribute_uint32 (ret, "unix::device", stbuf->st_dev);
g_file_info_set_attribute_uint64 (ret, "unix::inode", stbuf->st_ino);

if (S_ISREG (mode))
g_file_info_set_attribute_uint64 (ret, "standard::size", stbuf->st_size);

Expand Down
124 changes: 65 additions & 59 deletions src/libostree/ostree-repo-commit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2414,58 +2414,67 @@ ptrarray_path_join (GPtrArray *path)
}

static gboolean
get_modified_xattrs (OstreeRepo *self,
OstreeRepoCommitModifier *modifier,
const char *relpath,
GFileInfo *file_info,
GFile *path,
int dfd,
const char *dfd_subpath,
GVariant **out_xattrs,
GCancellable *cancellable,
GError **error)
{
g_autoptr(GVariant) ret_xattrs = NULL;

if (modifier && modifier->xattr_callback)
{
ret_xattrs = modifier->xattr_callback (self, relpath, file_info,
modifier->xattr_user_data);
}
else if (!(modifier && (modifier->flags & (OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS |
OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS)) > 0)
&& !self->disable_xattrs)
get_final_xattrs (OstreeRepo *self,
OstreeRepoCommitModifier *modifier,
const char *relpath,
GFileInfo *file_info,
GFile *path,
int dfd,
const char *dfd_subpath,
GVariant **out_xattrs,
gboolean *out_modified,
GCancellable *cancellable,
GError **error)
{
/* track whether the returned xattrs differ from the file on disk */
gboolean modified = TRUE;

/* fetch on-disk xattrs if needed & not disabled */
g_autoptr(GVariant) original_xattrs = NULL;
if (!(modifier && (modifier->flags & (OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS |
OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS)) > 0)
&& !self->disable_xattrs)
{
if (path && OSTREE_IS_REPO_FILE (path))
{
if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path),
&ret_xattrs,
cancellable,
error))
if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path), &original_xattrs,
cancellable, error))
return FALSE;
}
else if (path)
{
if (!glnx_dfd_name_get_all_xattrs (AT_FDCWD, gs_file_get_path_cached (path),
&ret_xattrs, cancellable, error))
&original_xattrs, cancellable, error))
return FALSE;
}
else if (dfd_subpath == NULL)
{
g_assert (dfd != -1);
if (!glnx_fd_get_all_xattrs (dfd, &ret_xattrs,
cancellable, error))
if (!glnx_fd_get_all_xattrs (dfd, &original_xattrs, cancellable, error))
return FALSE;
}
else
{
g_assert (dfd != -1);
if (!glnx_dfd_name_get_all_xattrs (dfd, dfd_subpath, &ret_xattrs,
cancellable, error))
if (!glnx_dfd_name_get_all_xattrs (dfd, dfd_subpath, &original_xattrs,
cancellable, error))
return FALSE;
}

g_assert (original_xattrs);
}

g_autoptr(GVariant) ret_xattrs = NULL;
if (modifier && modifier->xattr_callback)
{
ret_xattrs = modifier->xattr_callback (self, relpath, file_info,
modifier->xattr_user_data);
}

/* if callback returned NULL or didn't exist, default to on-disk state */
if (!ret_xattrs && original_xattrs)
ret_xattrs = g_variant_ref (original_xattrs);

if (modifier && modifier->sepolicy)
{
g_autofree char *label = NULL;
Expand Down Expand Up @@ -2509,8 +2518,13 @@ get_modified_xattrs (OstreeRepo *self,
}
}

if (original_xattrs && ret_xattrs && g_variant_equal (original_xattrs, ret_xattrs))
modified = FALSE;

if (out_xattrs)
*out_xattrs = g_steal_pointer (&ret_xattrs);
if (out_modified)
*out_modified = modified;
return TRUE;
}

Expand Down Expand Up @@ -2557,6 +2571,7 @@ write_directory_content_to_mtree_internal (OstreeRepo *self,
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 = (child_info != modified_info);

if (filter_result != OSTREE_REPO_COMMIT_FILTER_ALLOW)
{
Expand Down Expand Up @@ -2618,16 +2633,27 @@ write_directory_content_to_mtree_internal (OstreeRepo *self,
else
{
guint64 file_obj_length;
const char *loose_checksum;
g_autoptr(GInputStream) file_input = NULL;
g_autoptr(GVariant) xattrs = NULL;
g_autoptr(GInputStream) file_object_input = NULL;
g_autofree guchar *child_file_csum = NULL;
g_autofree char *tmp_checksum = NULL;

loose_checksum = devino_cache_lookup (self, modifier,
g_file_info_get_attribute_uint32 (child_info, "unix::device"),
g_file_info_get_attribute_uint64 (child_info, "unix::inode"));
g_autoptr(GVariant) xattrs = NULL;
gboolean xattrs_were_modified;
if (!get_final_xattrs (self, modifier, child_relpath, child_info, child,
dfd_iter != NULL ? dfd_iter->fd : -1,
name, &xattrs, &xattrs_were_modified,
cancellable, error))
return FALSE;

/* only check the devino cache if the file info & xattrs were not modified */
const char *loose_checksum = NULL;
if (!child_info_was_modified && !xattrs_were_modified)
{
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)
{
Expand All @@ -2653,12 +2679,6 @@ write_directory_content_to_mtree_internal (OstreeRepo *self,
}
}

if (!get_modified_xattrs (self, modifier,
child_relpath, child_info, child, dfd_iter != NULL ? dfd_iter->fd : -1, name,
&xattrs,
cancellable, error))
return FALSE;

if (!ostree_raw_file_to_content_stream (file_input,
modified_info, xattrs,
&file_object_input, &file_obj_length,
Expand Down Expand Up @@ -2732,10 +2752,8 @@ write_directory_to_mtree_internal (OstreeRepo *self,

if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW)
{
if (!get_modified_xattrs (self, modifier, relpath, child_info,
dir, -1, NULL,
&xattrs,
cancellable, error))
if (!get_final_xattrs (self, modifier, relpath, child_info, dir, -1, NULL,
&xattrs, NULL, cancellable, error))
return FALSE;

g_autofree guchar *child_file_csum = NULL;
Expand Down Expand Up @@ -2819,10 +2837,8 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self,

if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW)
{
if (!get_modified_xattrs (self, modifier, relpath, modified_info,
NULL, src_dfd_iter->fd, NULL,
&xattrs,
cancellable, error))
if (!get_final_xattrs (self, modifier, relpath, modified_info, NULL, src_dfd_iter->fd,
NULL, &xattrs, NULL, cancellable, error))
return FALSE;

if (!_ostree_repo_write_directory_meta (self, modified_info, xattrs, &child_file_csum,
Expand Down Expand Up @@ -2852,16 +2868,6 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self,
if (!glnx_fstatat (src_dfd_iter->fd, dent->d_name, &stbuf, AT_SYMLINK_NOFOLLOW, error))
return FALSE;

const char *loose_checksum = devino_cache_lookup (self, modifier, stbuf.st_dev, stbuf.st_ino);
if (loose_checksum)
{
if (!ostree_mutable_tree_replace_file (mtree, dent->d_name, loose_checksum,
error))
return FALSE;

continue;
}

g_autoptr(GFileInfo) child_info = _ostree_stbuf_to_gfileinfo (&stbuf);
g_file_info_set_name (child_info, dent->d_name);

Expand Down
14 changes: 13 additions & 1 deletion tests/basic-test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))'
Expand Down Expand Up @@ -574,6 +574,18 @@ $OSTREE checkout test2 test2-checkout
(cd test2-checkout && $OSTREE commit ${COMMIT_ARGS} --link-checkout-speedup -b test2 -s "tmp")
echo "ok commit with link speedup"

cd ${test_tmpdir}
rm -rf test2-checkout
$OSTREE checkout test2 test2-checkout
cat > statoverride.txt <<EOF
=384 /baz/cow
EOF
$OSTREE commit ${COMMIT_ARGS} --statoverride=statoverride.txt --link-checkout-speedup -b test2-tmp test2-checkout
$OSTREE diff test2 test2-tmp > diff-test2
assert_file_has_content diff-test2 'M */baz/cow$'
assert_not_file_has_content diff-test2 'baz/saucer'
echo "ok commit with link speedup and modifier"

cd ${test_tmpdir}
$OSTREE ls test2
echo "ok ls with no argument"
Expand Down
Loading

0 comments on commit 7d9059c

Please sign in to comment.