From 2fe0fe3102bbe174f3a850696586ecd081e881b4 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Mon, 11 Jul 2022 16:25:38 -0700 Subject: [PATCH] Add vfs.Dentry.Evictable support. This change does 3 things: - Adds `IsEvictable() bool` and `MarkEvictable()` methods to vfs.Dentry. A dentry's "evictability" is just a hint to filesystem implementations that this dentry will not be accessed again (from the caller of Dentry.MarkEvictable()). So filesystem implementation can choose to release resources for such evictable dentries. For example, caching such dentries is no longer useful. - In VFS2, only 2 filesystem implementations have a LRU dentry cache: kernfs and gofer. Dentries with 0 refs are cached here. Updated both these filesystem implementations to evict dentries that have 0 refs and are evictable. - Updated overlayfs to mark lower layer dentries for non-directory files as evictable when either a dentry is copied up or a dentry is deleted. In both cases if there is a dentry in the lower layer, it is no longer visible from the overlayfs. Note that the file "replaced" by rename(2) is also considered deleted. Motivation: We were observing elevated memory usage in VFS2 when using overlayfs because VFS2 gofer client has a dentry cache and was holding on to lower layer dentries which were anyways not accessible from the overlayfs. The file mappings for these stale dentries were responsible for holding up so much sentry memory. PiperOrigin-RevId: 460325860 --- pkg/sentry/fsimpl/gofer/gofer.go | 10 +++++++ pkg/sentry/fsimpl/kernfs/kernfs.go | 39 ++++++++++++++++--------- pkg/sentry/fsimpl/overlay/copy_up.go | 7 +++++ pkg/sentry/fsimpl/overlay/filesystem.go | 14 +++++++++ pkg/sentry/vfs/dentry.go | 20 +++++++++++++ 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index b08d759dc..ecc962260 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -1857,6 +1857,16 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo d.destroyLocked(ctx) // +checklocksforce: renameMu must be acquired at this point. return } + if d.vfsd.IsEvictable() { + d.cachingMu.Unlock() + // Attempt to evict. + if renameMuWriteLocked { + d.evictLocked(ctx) // +checklocksforce: renameMu is locked in this case. + return + } + d.evict(ctx) + return + } // If d still has inotify watches and it is not deleted or invalidated, it // can't be evicted. Otherwise, we will lose its watches, even if a new // dentry is created for the same file in the future. Note that the size of diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index 4c0a954c9..43af31123 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/pkg/sentry/fsimpl/kernfs/kernfs.go @@ -342,6 +342,10 @@ func (d *Dentry) cacheLocked(ctx context.Context) { d.destroyLocked(ctx) return } + if d.VFSDentry().IsEvictable() { + d.evictLocked(ctx) + return + } // If d is already cached, just move it to the front of the LRU. if d.cached { d.fs.cachedDentries.Remove(d) @@ -363,29 +367,36 @@ func (d *Dentry) cacheLocked(ctx context.Context) { // Preconditions: // - fs.mu must be locked for writing. -// - fs.cachedDentriesLen != 0. func (fs *Filesystem) evictCachedDentryLocked(ctx context.Context) { // Evict the least recently used dentry because cache size is greater than // max cache size (configured on mount). - victim := fs.cachedDentries.Back() - fs.cachedDentries.Remove(victim) - fs.cachedDentriesLen-- - victim.cached = false + fs.cachedDentries.Back().evictLocked(ctx) +} + +// Preconditions: +// - d.fs.mu must be locked for writing. +func (d *Dentry) evictLocked(ctx context.Context) { + if d == nil { + return + } + if d.cached { + d.fs.cachedDentries.Remove(d) + d.fs.cachedDentriesLen-- + d.cached = false + } // victim.refs may have become non-zero from an earlier path resolution // after it was inserted into fs.cachedDentries. - if victim.refs.Load() == 0 { - if !victim.vfsd.IsDead() { - victim.parent.dirMu.Lock() + if d.refs.Load() == 0 { + if !d.vfsd.IsDead() { + d.parent.dirMu.Lock() // Note that victim can't be a mount point (in any mount // namespace), since VFS holds references on mount points. - fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, victim.VFSDentry()) - delete(victim.parent.children, victim.name) - victim.parent.dirMu.Unlock() + d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + delete(d.parent.children, d.name) + d.parent.dirMu.Unlock() } - victim.destroyLocked(ctx) + d.destroyLocked(ctx) } - // Whether or not victim was destroyed, we brought fs.cachedDentriesLen - // back down to fs.MaxCachedDentries, so we don't loop. } // destroyLocked destroys the dentry. diff --git a/pkg/sentry/fsimpl/overlay/copy_up.go b/pkg/sentry/fsimpl/overlay/copy_up.go index 4cfc3f6fd..240319646 100644 --- a/pkg/sentry/fsimpl/overlay/copy_up.go +++ b/pkg/sentry/fsimpl/overlay/copy_up.go @@ -287,6 +287,13 @@ func (d *dentry) copyUpMaybeSyntheticMountpointLocked(ctx context.Context, forSy d.devMajor.Store(upperStat.DevMajor) d.devMinor.Store(upperStat.DevMinor) d.ino.Store(upperStat.Ino) + + // Lower level dentries for non-directories are no longer accessible from + // the overlayfs anymore after copyup. Ask filesystems to release their + // resources whenever possible. + for _, lowerDentry := range d.lowerVDs { + lowerDentry.Dentry().MarkEvictable() + } } if mmapOpts != nil && mmapOpts.Mappable != nil { diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 14b32d2cf..799e5c7e2 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -1276,6 +1276,12 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) delete(oldParent.children, oldName) if replaced != nil { + // Lower dentries of replaced are not reachable from the overlay anymore. + // NOTE(b/237573779): Ask lower filesystem to release resources for this + // dentry whenever possible to reduce resource usage. + for _, replaceLower := range replaced.lowerVDs { + replaceLower.Dentry().MarkEvictable() + } ds = appendDentry(ds, replaced) } if oldParent != newParent { @@ -1639,6 +1645,14 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error vfsObj.CommitDeleteDentry(ctx, &child.vfsd) delete(parent.children, name) + if !child.isDir() { + // Once a whiteout is created, non-directory dentries on the lower layers + // are no longer reachable from the overlayfs. Ask filesystems to release + // their resources whenever possible. + for _, lowerDentry := range child.lowerVDs { + lowerDentry.Dentry().MarkEvictable() + } + } ds = appendDentry(ds, child) vfs.InotifyRemoveChild(ctx, &child.watches, &parent.watches, name) parent.dirents = nil diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go index 79ed28dbd..4edb5f7c3 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -66,6 +66,12 @@ type Dentry struct { // InvalidateDentry). dead is protected by mu. dead bool + // evictable is set by the VFS layer or filesystems like overlayfs as a hint + // that this dentry will not be accessed hence forth. So filesystems that + // cache dentries locally can use this hint to release the dentry when all + // references are dropped. evictable is protected by mu. + evictable bool + // mounts is the number of Mounts for which this Dentry is Mount.point. mounts atomicbitops.Uint32 @@ -163,6 +169,20 @@ func (d *Dentry) IsDead() bool { return d.dead } +// IsEvictable returns true if d is evictable from filesystem dentry cache. +func (d *Dentry) IsEvictable() bool { + d.mu.Lock() + defer d.mu.Unlock() + return d.evictable +} + +// MarkEvictable marks d as evictable. +func (d *Dentry) MarkEvictable() { + d.mu.Lock() + defer d.mu.Unlock() + d.evictable = true +} + func (d *Dentry) isMounted() bool { return d.mounts.Load() != 0 }