From ee37961d4a83a1244175339fc526ede1a6a13817 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Thu, 8 Jun 2023 10:24:33 -0700 Subject: [PATCH] Mark newly created directories in merged parents in overlay as opaque. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an optimization that will prevent needless lookups on lower layers for directories. This is analogous to the Linux commit 97c684cc9110 ("ovl: create directories inside merged parent opaque"). If mkdir(2) does succeed, then none of the lower layers have a file at that position, otherwise mkdir(2) would have failed with EEXIST. Changes on the lower layer underneath a new upper layer directory are not visible. See the following for example: ``` $ sudo mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=workdir none overlay $ mkdir -p overlay/dir/dir1 $ mkdir -p lower/dir/dir2 $ ls overlay/dir/ dir1 ``` Benchmarking shows that this change cuts ABSL build time by 9.5%! ``` goos: linux goarch: amd64 cpu: Intel(R) Xeon(R) CPU @ 2.20GHz │ /tmp/benchout.runsc │ /tmp/benchout.runsc-opt │ │ sec/op │ sec/op vs base │ BuildABSL/page_cache.clean/filesystem.bindfs-8 71.16 ± 12% 64.40 ± 3% -9.50% (p=0.003 n=8) BuildGRPC/page_cache.clean/filesystem.bindfs-8 418.2 ± 1% 419.9 ± 1% ~ (p=0.721 n=8) RubySpecTest/page_cache.clean/filesystem.bindfs-8 76.67 ± 5% 75.17 ± 1% ~ (p=0.382 n=8) geomean 131.6 126.7 -3.78% │ /tmp/benchout.runsc │ /tmp/benchout.runsc-opt │ │ load.sec │ load.sec vs base │ RubySpecTest/page_cache.clean/filesystem.bindfs-8 12.95 ± 6% 12.80 ± 5% ~ (p=0.625 n=8) ``` PiperOrigin-RevId: 538823897 --- pkg/sentry/fsimpl/overlay/filesystem.go | 18 +++++++++++++++--- test/syscalls/linux/xattr.cc | 7 ++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 771ce6f94..5e52e71cb 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -749,9 +749,10 @@ func (fs *filesystem) MkdirAt(ctx context.Context, rp *vfs.ResolvingPath, opts v return err } if haveUpperWhiteout { - // There may be directories on lower layers (previously hidden by - // the whiteout) that the new directory should not be merged with. - // Mark it opaque to prevent merging. + // A whiteout is being replaced with this new directory. There may be + // directories on lower layers (previously hidden by the whiteout) that + // the new directory should not be merged with, so mark as opaque. + // See fs/overlayfs/dir.c:ovl_create_over_whiteout() -> ovl_set_opaque(). if err := vfsObj.SetXattrAt(ctx, fs.creds, &pop, &vfs.SetXattrOptions{ Name: _OVL_XATTR_OPAQUE, Value: "y", @@ -763,6 +764,17 @@ func (fs *filesystem) MkdirAt(ctx context.Context, rp *vfs.ResolvingPath, opts v } return err } + } else if len(parent.lowerVDs) > 0 { + // If haveUpperWhiteout is false and the parent is merged, then we should + // apply an optimization. We know that nothing exists on the parent's + // lower layers. Otherwise doCreateAt() would have failed with EEXIST. + // Mark the new directory opaque to avoid unnecessary lower lookups in + // fs.lookupLocked(). Allow it to fail since this is an optimization. + // See fs/overlayfs/dir.c:ovl_create_upper() -> ovl_set_opaque(). + _ = vfsObj.SetXattrAt(ctx, fs.creds, &pop, &vfs.SetXattrOptions{ + Name: _OVL_XATTR_OPAQUE, + Value: "y", + }) } return nil }) diff --git a/test/syscalls/linux/xattr.cc b/test/syscalls/linux/xattr.cc index 114de0040..1b80f9721 100644 --- a/test/syscalls/linux/xattr.cc +++ b/test/syscalls/linux/xattr.cc @@ -185,7 +185,12 @@ TEST_F(XattrTest, XattrOnDirectory) { EXPECT_THAT(getxattr(dir.path().c_str(), name, nullptr, 0), SyscallSucceedsWithValue(0)); - char list[sizeof(name)]; + // Overlay may have private attributes. Even though it is not returned to + // userspace, it is counted against the `size` argument in listxattr(2). + // See fs/overlayfs/inode.c:ovl_listxattr(). Notice that `size` is not + // extended to accommodate for private attributes that are filtered later. + // So use a large enough buffer for xattr list. + char list[64]; EXPECT_THAT(listxattr(dir.path().c_str(), list, sizeof(list)), SyscallSucceedsWithValue(sizeof(name))); EXPECT_STREQ(list, name);