diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 9c44f0506..d1af32204 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -1918,7 +1918,6 @@ func (fs *filesystem) MountOptions() string { {moptDfltGID, fs.opts.dfltgid}, {moptMsize, fs.opts.msize}, {moptVersion, fs.opts.version}, - {moptDentryCacheLimit, fs.opts.maxCachedDentries}, } switch fs.opts.interop { diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index fc72d8602..8eb7f1374 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -20,7 +20,7 @@ // regularFileFD/directoryFD.mu // filesystem.renameMu // dentry.cachingMu -// filesystem.cacheMu +// dentryCache.mu // dentry.dirMu // filesystem.syncMu // dentry.metadataMu @@ -81,7 +81,6 @@ const ( moptDfltGID = "dfltgid" moptMsize = "msize" moptVersion = "version" - moptDentryCacheLimit = "dentry_cache_limit" moptCache = "cache" moptForcePageCache = "force_page_cache" moptLimitHostFDTranslation = "limit_host_fd_translation" @@ -97,6 +96,21 @@ const ( cacheRemoteRevalidating = "remote_revalidating" ) +const defaultMaxCachedDentries = 1000 + +// +stateify savable +type dentryCache struct { + // mu protects the below fields. + mu sync.Mutex `state:"nosave"` + // dentries contains all dentries with 0 references. Due to race conditions, + // it may also contain dentries with non-zero references. + dentries dentryList + // dentriesLen is the number of dentries in dentries. + dentriesLen uint64 + // maxCachedDentries is the maximum number of cachable dentries. + maxCachedDentries uint64 +} + // Valid values for "trans" mount option. const transportModeFD = "fd" @@ -147,13 +161,7 @@ type filesystem struct { // it is reachable from its parent). renameMu sync.RWMutex `state:"nosave"` - // cachedDentries contains all dentries with 0 references. (Due to race - // conditions, it may also contain dentries with non-zero references.) - // cachedDentriesLen is the number of dentries in cachedDentries. These fields - // are protected by cacheMu. - cacheMu sync.Mutex `state:"nosave"` - cachedDentries dentryList - cachedDentriesLen uint64 + dentryCache *dentryCache // syncableDentries contains all non-synthetic dentries. specialFileFDs // contains all open specialFileFDs. These fields are protected by syncMu. @@ -197,9 +205,6 @@ type filesystemOptions struct { msize uint32 version string - // maxCachedDentries is the maximum size of filesystem.cachedDentries. - maxCachedDentries uint64 - // If forcePageCache is true, host FDs may not be used for application // memory mappings even if available; instead, the client must perform its // own caching of regular file pages. This is primarily useful for testing. @@ -423,18 +428,6 @@ func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt fsopts.version = version } - // Parse the dentry cache limit. - fsopts.maxCachedDentries = 1000 - if str, ok := mopts[moptDentryCacheLimit]; ok { - delete(mopts, moptDentryCacheLimit) - maxCachedDentries, err := strconv.ParseUint(str, 10, 64) - if err != nil { - ctx.Warningf("gofer.FilesystemType.GetFilesystem: invalid dentry cache limit: %s=%s", moptDentryCacheLimit, str) - return nil, nil, linuxerr.EINVAL - } - fsopts.maxCachedDentries = maxCachedDentries - } - // Handle simple flags. if _, ok := mopts[moptForcePageCache]; ok { delete(mopts, moptForcePageCache) @@ -488,6 +481,7 @@ func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt specialFileFDs: make(map[*specialFileFD]struct{}), inoByQIDPath: make(map[uint64]uint64), inoByKey: make(map[inoKey]uint64), + dentryCache: &dentryCache{maxCachedDentries: defaultMaxCachedDentries}, } fs.vfsfs.Init(vfsObj, &fstype, fs) @@ -824,7 +818,7 @@ type dentry struct { cachingMu sync.Mutex `state:"nosave"` // If cached is true, dentryEntry links dentry into - // filesystem.cachedDentries. cached and dentryEntry are protected by + // filesystem.dentryCache.dentries. cached and dentryEntry are protected by // cachingMu. cached bool dentryEntry @@ -1811,11 +1805,12 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo return } if refs > 0 { - // fs.cachedDentries is permitted to contain dentries with non-zero refs, - // which are skipped by fs.evictCachedDentryLocked() upon reaching the end - // of the LRU. But it is still beneficial to remove d from the cache as we - // are already holding d.cachingMu. Keeping a cleaner cache also reduces - // the number of evictions (which is expensive as it acquires fs.renameMu). + // fs.dentryCache.dentries is permitted to contain dentries with non-zero + // refs, which are skipped by fs.evictCachedDentryLocked() upon reaching + // the end of the LRU. But it is still beneficial to remove d from the + // cache as we are already holding d.cachingMu. Keeping a cleaner cache + // also reduces the number of evictions (which is expensive as it acquires + // fs.renameMu). d.removeFromCacheLocked() d.cachingMu.Unlock() return @@ -1872,22 +1867,22 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo return } - d.fs.cacheMu.Lock() + d.fs.dentryCache.mu.Lock() // If d is already cached, just move it to the front of the LRU. if d.cached { - d.fs.cachedDentries.Remove(d) - d.fs.cachedDentries.PushFront(d) - d.fs.cacheMu.Unlock() + d.fs.dentryCache.dentries.Remove(d) + d.fs.dentryCache.dentries.PushFront(d) + d.fs.dentryCache.mu.Unlock() d.cachingMu.Unlock() return } // Cache the dentry, then evict the least recently used cached dentry if // the cache becomes over-full. - d.fs.cachedDentries.PushFront(d) - d.fs.cachedDentriesLen++ + d.fs.dentryCache.dentries.PushFront(d) + d.fs.dentryCache.dentriesLen++ d.cached = true - shouldEvict := d.fs.cachedDentriesLen > d.fs.opts.maxCachedDentries - d.fs.cacheMu.Unlock() + shouldEvict := d.fs.dentryCache.dentriesLen > d.fs.dentryCache.maxCachedDentries + d.fs.dentryCache.mu.Unlock() d.cachingMu.Unlock() if shouldEvict { @@ -1904,10 +1899,10 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo // Preconditions: d.cachingMu must be locked. func (d *dentry) removeFromCacheLocked() { if d.cached { - d.fs.cacheMu.Lock() - d.fs.cachedDentries.Remove(d) - d.fs.cachedDentriesLen-- - d.fs.cacheMu.Unlock() + d.fs.dentryCache.mu.Lock() + d.fs.dentryCache.dentries.Remove(d) + d.fs.dentryCache.dentriesLen-- + d.fs.dentryCache.mu.Unlock() d.cached = false } } @@ -1916,7 +1911,7 @@ func (d *dentry) removeFromCacheLocked() { // unlocked. // +checklocks:fs.renameMu func (fs *filesystem) evictAllCachedDentriesLocked(ctx context.Context) { - for fs.cachedDentriesLen != 0 { + for fs.dentryCache.dentriesLen != 0 { fs.evictCachedDentryLocked(ctx) } } @@ -1926,42 +1921,69 @@ func (fs *filesystem) evictAllCachedDentriesLocked(ctx context.Context) { // // +checklocks:fs.renameMu func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) { - fs.cacheMu.Lock() - victim := fs.cachedDentries.Back() - fs.cacheMu.Unlock() + fs.dentryCache.mu.Lock() + victim := fs.dentryCache.dentries.Back() + fs.dentryCache.mu.Unlock() if victim == nil { - // fs.cachedDentries may have become empty between when it was checked and - // when we locked fs.cacheMu. + // fs.dentryCache.dentries may have become empty between when it was + // checked and when we locked fs.dentryCache.mu. return } - victim.cachingMu.Lock() - victim.removeFromCacheLocked() - // victim.refs or victim.watches.Size() may have become non-zero from an - // earlier path resolution since it was inserted into fs.cachedDentries. - if victim.refs.Load() != 0 || victim.watches.Size() != 0 { - victim.cachingMu.Unlock() + if victim.fs == fs { + victim.evictLocked(ctx) // +checklocksforce: owned as precondition, victim.fs == fs return } - if victim.parent != nil { - victim.parent.dirMu.Lock() - if !victim.vfsd.IsDead() { - // 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.vfsd) - delete(victim.parent.children, victim.name) + + // The dentry cache is shared between all gofer filesystems and the victim is + // from another filesystem. Have that filesystem do the work. We unlock + // fs.renameMu to prevent deadlock: two filesystems could otherwise wait on + // each others' renameMu. + fs.renameMu.Unlock() + defer fs.renameMu.Lock() + victim.evict(ctx) +} + +// Preconditions: +// - d.fs.renameMu must not be locked for writing. +func (d *dentry) evict(ctx context.Context) { + d.fs.renameMu.Lock() + defer d.fs.renameMu.Unlock() + d.evictLocked(ctx) +} + +// Preconditions: +// - d.fs.renameMu must be locked for writing; it may be temporarily unlocked. +// +// +checklocks:d.fs.renameMu +func (d *dentry) evictLocked(ctx context.Context) { + d.cachingMu.Lock() + d.removeFromCacheLocked() + // d.refs or d.watches.Size() may have become non-zero from an earlier path + // resolution since it was inserted into fs.dentryCache.dentries. + if d.refs.Load() != 0 || d.watches.Size() != 0 { + d.cachingMu.Unlock() + return + } + if d.parent != nil { + d.parent.dirMu.Lock() + if !d.vfsd.IsDead() { + // Note that d can't be a mount point (in any mount namespace), since VFS + // holds references on mount points. + d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, &d.vfsd) + delete(d.parent.children, d.name) // We're only deleting the dentry, not the file it // represents, so we don't need to update - // victimParent.dirents etc. + // victim parent.dirents etc. } - victim.parent.dirMu.Unlock() + d.parent.dirMu.Unlock() } - // Safe to unlock cachingMu now that victim.vfsd.IsDead(). Henceforth any - // concurrent caching attempts on victim will attempt to destroy it and so - // will try to acquire fs.renameMu (which we have already acquired). Hence, + // Safe to unlock cachingMu now that d.vfsd.IsDead(). Henceforth any + // concurrent caching attempts on d will attempt to destroy it and so will + // try to acquire fs.renameMu (which we have already acquiredd). Hence, // fs.renameMu will synchronize the destroy attempts. - victim.cachingMu.Unlock() - victim.destroyLocked(ctx) // +checklocksforce: owned as precondition, victim.fs == fs. + d.cachingMu.Unlock() + d.destroyLocked(ctx) // +checklocksforce: owned as precondition. } // destroyLocked destroys the dentry. diff --git a/pkg/sentry/fsimpl/gofer/gofer_test.go b/pkg/sentry/fsimpl/gofer/gofer_test.go index 0384683f9..4a518b504 100644 --- a/pkg/sentry/fsimpl/gofer/gofer_test.go +++ b/pkg/sentry/fsimpl/gofer/gofer_test.go @@ -25,14 +25,12 @@ import ( func TestDestroyIdempotent(t *testing.T) { ctx := contexttest.Context(t) fs := filesystem{ - mfp: pgalloc.MemoryFileProviderFromContext(ctx), - opts: filesystemOptions{ - // Test relies on no dentry being held in the cache. - maxCachedDentries: 0, - }, + mfp: pgalloc.MemoryFileProviderFromContext(ctx), syncableDentries: make(map[*dentry]struct{}), inoByQIDPath: make(map[uint64]uint64), inoByKey: make(map[inoKey]uint64), + // Test relies on no dentry being held in the cache. + dentryCache: &dentryCache{maxCachedDentries: 0}, } attr := &p9.Attr{ diff --git a/test/e2e/integration_test.go b/test/e2e/integration_test.go index 788c36ae6..81f520a9f 100644 --- a/test/e2e/integration_test.go +++ b/test/e2e/integration_test.go @@ -1003,7 +1003,7 @@ func TestRlimitNoFile(t *testing.T) { d := dockerutil.MakeContainerWithRuntime(ctx, t, "-fdlimit") defer d.CleanUp(ctx) - // We have to create a directory with a bunch of files. + // Create a directory with a bunch of files. const nfiles = 5000 tmpDir := testutil.TmpDir() for i := 0; i < nfiles; i++ {