Cache directory entries in the overlay

Currently, the overlay dirCache is only used for a single logical use of
getdents. i.e., it is discard when the FD is closed or seeked back to
the beginning.

But the initial work of getting the directory contents can be quite
expensive (particularly sorting large directories), so we should keep it
as long as possible.

This is very similar to the readdirCache in fs/gofer.

Since the upper filesystem does not have to allow caching readdir
entries, the new CacheReaddir MountSourceOperations method controls this
behavior.

This caching should be trivially movable to all Inodes if desired,
though that adds an additional copy step for non-overlay Inodes.
(Overlay Inodes already do the extra copy).

PiperOrigin-RevId: 255477592

pkg/sentry/fs/BUILD

@@ -69,6 +69,7 @@ go_library(
         "//pkg/state",
         "//pkg/syserror",
         "//pkg/waiter",
+        "//third_party/gvsync",
     ],
 )
 

pkg/sentry/fs/dirent.go

@@ -918,7 +918,7 @@ type DirIterator interface {
 	// calls, and must start with the given offset.
 	//
 	// The caller must ensure that this operation is permitted.
-	IterateDir(ctx context.Context, dirCtx *DirCtx, offset int) (int, error)
+	IterateDir(ctx context.Context, d *Dirent, dirCtx *DirCtx, offset int) (int, error)
 }
 
 // DirentReaddir serializes the directory entries of d including "." and "..".
@@ -988,7 +988,7 @@ func direntReaddir(ctx context.Context, d *Dirent, it DirIterator, root *Dirent,
 	// it.IterateDir should be passed an offset that does not include the
 	// initial dot elements.  We will add them back later.
 	offset -= 2
-	newOffset, err := it.IterateDir(ctx, dirCtx, int(offset))
+	newOffset, err := it.IterateDir(ctx, d, dirCtx, int(offset))
 	if int64(newOffset) < offset {
 		panic(fmt.Sprintf("node.Readdir returned offset %v less than input offset %v", newOffset, offset))
 	}

pkg/sentry/fs/file_overlay.go

@@ -85,12 +85,6 @@ type overlayFileOperations struct {
 	// protected by File.mu of the owning file, which is held during
 	// Readdir and Seek calls.
 	dirCursor string
-
-	// dirCacheMu protects dirCache.
-	dirCacheMu sync.RWMutex `state:"nosave"`
-
-	// dirCache is cache of DentAttrs from upper and lower Inodes.
-	dirCache *SortedDentryMap
 }
 
 // Release implements FileOperations.Release.
@@ -171,53 +165,68 @@ func (f *overlayFileOperations) Readdir(ctx context.Context, file *File, seriali
 	if root != nil {
 		defer root.DecRef()
 	}
+
 	dirCtx := &DirCtx{
 		Serializer: serializer,
 		DirCursor:  &f.dirCursor,
 	}
-
-	// If the directory dirent is frozen, then DirentReaddir will calculate
-	// the children based off the frozen dirent tree. There is no need to
-	// call readdir on the upper/lower layers.
-	if file.Dirent.frozen {
-		return DirentReaddir(ctx, file.Dirent, f, root, dirCtx, file.Offset())
-	}
-
-	// Otherwise proceed with usual overlay readdir.
-	o := file.Dirent.Inode.overlay
-
-	// readdirEntries holds o.copyUpMu to ensure that copy-up does not
-	// occur while calculating the readir results.
-	//
-	// However, it is possible for a copy-up to occur after the call to
-	// readdirEntries, but before setting f.dirCache. This is OK, since
-	// copy-up only does not change the children in a way that would affect
-	// the children returned in dirCache. Copy-up only moves
-	// files/directories between layers in the overlay.
-	//
-	// It is also possible for Readdir to race with a Create operation
-	// (which may trigger a copy-up during it's execution). Depending on
-	// whether the Create happens before or after the readdirEntries call,
-	// the newly created file may or may not appear in the readdir results.
-	// But this can only be caused by a real race between readdir and
-	// create syscalls, so it's also OK.
-	dirCache, err := readdirEntries(ctx, o)
-	if err != nil {
-		return file.Offset(), err
-	}
-
-	f.dirCacheMu.Lock()
-	f.dirCache = dirCache
-	f.dirCacheMu.Unlock()
-
 	return DirentReaddir(ctx, file.Dirent, f, root, dirCtx, file.Offset())
 }
 
 // IterateDir implements DirIterator.IterateDir.
-func (f *overlayFileOperations) IterateDir(ctx context.Context, dirCtx *DirCtx, offset int) (int, error) {
-	f.dirCacheMu.RLock()
-	n, err := GenericReaddir(dirCtx, f.dirCache)
-	f.dirCacheMu.RUnlock()
+func (f *overlayFileOperations) IterateDir(ctx context.Context, d *Dirent, dirCtx *DirCtx, offset int) (int, error) {
+	o := d.Inode.overlay
+
+	if !d.Inode.MountSource.CacheReaddir() {
+		// Can't use the dirCache. Simply read the entries.
+		entries, err := readdirEntries(ctx, o)
+		if err != nil {
+			return offset, err
+		}
+		n, err := GenericReaddir(dirCtx, entries)
+		return offset + n, err
+	}
+
+	// Otherwise, use or create cached entries.
+
+	o.dirCacheMu.RLock()
+	if o.dirCache != nil {
+		n, err := GenericReaddir(dirCtx, o.dirCache)
+		o.dirCacheMu.RUnlock()
+		return offset + n, err
+	}
+	o.dirCacheMu.RUnlock()
+
+	// readdirEntries holds o.copyUpMu to ensure that copy-up does not
+	// occur while calculating the readdir results.
+	//
+	// However, it is possible for a copy-up to occur after the call to
+	// readdirEntries, but before setting o.dirCache. This is OK, since
+	// copy-up does not change the children in a way that would affect the
+	// children returned in dirCache. Copy-up only moves files/directories
+	// between layers in the overlay.
+	//
+	// We must hold dirCacheMu around both readdirEntries and setting
+	// o.dirCache to synchronize with dirCache invalidations done by
+	// Create, Remove, Rename.
+	o.dirCacheMu.Lock()
+
+	// We expect dirCache to be nil (we just checked above), but there is a
+	// chance that a racing call managed to just set it, in which case we
+	// can use that new value.
+	if o.dirCache == nil {
+		dirCache, err := readdirEntries(ctx, o)
+		if err != nil {
+			o.dirCacheMu.Unlock()
+			return offset, err
+		}
+		o.dirCache = dirCache
+	}
+
+	o.dirCacheMu.DowngradeLock()
+	n, err := GenericReaddir(dirCtx, o.dirCache)
+	o.dirCacheMu.RUnlock()
+
 	return offset + n, err
 }
 

pkg/sentry/fs/fsutil/file.go

@@ -285,7 +285,7 @@ func NewStaticDirFileOperations(dentries *fs.SortedDentryMap) *StaticDirFileOper
 }
 
 // IterateDir implements DirIterator.IterateDir.
-func (sdfo *StaticDirFileOperations) IterateDir(ctx context.Context, dirCtx *fs.DirCtx, offset int) (int, error) {
+func (sdfo *StaticDirFileOperations) IterateDir(ctx context.Context, d *fs.Dirent, dirCtx *fs.DirCtx, offset int) (int, error) {
 	n, err := fs.GenericReaddir(dirCtx, sdfo.dentryMap)
 	return offset + n, err
 }

pkg/sentry/fs/gofer/file.go

@@ -137,7 +137,7 @@ func (f *fileOperations) Readdir(ctx context.Context, file *fs.File, serializer
 }
 
 // IterateDir implements fs.DirIterator.IterateDir.
-func (f *fileOperations) IterateDir(ctx context.Context, dirCtx *fs.DirCtx, offset int) (int, error) {
+func (f *fileOperations) IterateDir(ctx context.Context, d *fs.Dirent, dirCtx *fs.DirCtx, offset int) (int, error) {
 	f.inodeOperations.readdirMu.Lock()
 	defer f.inodeOperations.readdirMu.Unlock()
 

pkg/sentry/fs/gofer/session.go

@@ -145,16 +145,21 @@ func (s *session) Destroy() {
 	s.client.Close()
 }
 
-// Revalidate implements MountSource.Revalidate.
+// Revalidate implements MountSourceOperations.Revalidate.
 func (s *session) Revalidate(ctx context.Context, name string, parent, child *fs.Inode) bool {
 	return s.cachePolicy.revalidate(ctx, name, parent, child)
 }
 
-// Keep implements MountSource.Keep.
+// Keep implements MountSourceOperations.Keep.
 func (s *session) Keep(d *fs.Dirent) bool {
 	return s.cachePolicy.keep(d)
 }
 
+// CacheReaddir implements MountSourceOperations.CacheReaddir.
+func (s *session) CacheReaddir() bool {
+	return s.cachePolicy.cacheReaddir()
+}
+
 // ResetInodeMappings implements fs.MountSourceOperations.ResetInodeMappings.
 func (s *session) ResetInodeMappings() {
 	s.inodeMappings = make(map[uint64]string)

pkg/sentry/fs/host/file.go

@@ -179,7 +179,7 @@ func (f *fileOperations) Readdir(ctx context.Context, file *fs.File, serializer
 }
 
 // IterateDir implements fs.DirIterator.IterateDir.
-func (f *fileOperations) IterateDir(ctx context.Context, dirCtx *fs.DirCtx, offset int) (int, error) {
+func (f *fileOperations) IterateDir(ctx context.Context, d *fs.Dirent, dirCtx *fs.DirCtx, offset int) (int, error) {
 	if f.dirinfo == nil {
 		f.dirinfo = new(dirInfo)
 		f.dirinfo.buf = make([]byte, usermem.PageSize)

pkg/sentry/fs/host/inode_test.go

@@ -64,12 +64,12 @@ func TestMultipleReaddir(t *testing.T) {
 	defer openFile.DecRef()
 
 	c1 := &fs.DirCtx{DirCursor: new(string)}
-	if _, err := openFile.FileOperations.(*fileOperations).IterateDir(ctx, c1, 0); err != nil {
+	if _, err := openFile.FileOperations.(*fileOperations).IterateDir(ctx, dirent, c1, 0); err != nil {
 		t.Fatalf("First Readdir failed: %v", err)
 	}
 
 	c2 := &fs.DirCtx{DirCursor: new(string)}
-	if _, err := openFile.FileOperations.(*fileOperations).IterateDir(ctx, c2, 0); err != nil {
+	if _, err := openFile.FileOperations.(*fileOperations).IterateDir(ctx, dirent, c2, 0); err != nil {
 		t.Errorf("Second Readdir failed: %v", err)
 	}
 

pkg/sentry/fs/inode_overlay.go

@@ -217,6 +217,9 @@ func overlayCreate(ctx context.Context, o *overlayEntry, parent *Dirent, name st
 		return nil, err
 	}
 
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+
 	// Take another reference on the upper file's inode, which will be
 	// owned by the overlay entry.
 	upperFile.Dirent.Inode.IncRef()
@@ -265,7 +268,12 @@ func overlayCreateDirectory(ctx context.Context, o *overlayEntry, parent *Dirent
 	if err := copyUpLockedForRename(ctx, parent); err != nil {
 		return err
 	}
-	return o.upper.InodeOperations.CreateDirectory(ctx, o.upper, name, perm)
+	if err := o.upper.InodeOperations.CreateDirectory(ctx, o.upper, name, perm); err != nil {
+		return err
+	}
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+	return nil
 }
 
 func overlayCreateLink(ctx context.Context, o *overlayEntry, parent *Dirent, oldname string, newname string) error {
@@ -273,7 +281,12 @@ func overlayCreateLink(ctx context.Context, o *overlayEntry, parent *Dirent, old
 	if err := copyUpLockedForRename(ctx, parent); err != nil {
 		return err
 	}
-	return o.upper.InodeOperations.CreateLink(ctx, o.upper, oldname, newname)
+	if err := o.upper.InodeOperations.CreateLink(ctx, o.upper, oldname, newname); err != nil {
+		return err
+	}
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+	return nil
 }
 
 func overlayCreateHardLink(ctx context.Context, o *overlayEntry, parent *Dirent, target *Dirent, name string) error {
@@ -285,7 +298,12 @@ func overlayCreateHardLink(ctx context.Context, o *overlayEntry, parent *Dirent,
 	if err := copyUpLockedForRename(ctx, target); err != nil {
 		return err
 	}
-	return o.upper.InodeOperations.CreateHardLink(ctx, o.upper, target.Inode.overlay.upper, name)
+	if err := o.upper.InodeOperations.CreateHardLink(ctx, o.upper, target.Inode.overlay.upper, name); err != nil {
+		return err
+	}
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+	return nil
 }
 
 func overlayCreateFifo(ctx context.Context, o *overlayEntry, parent *Dirent, name string, perm FilePermissions) error {
@@ -293,7 +311,12 @@ func overlayCreateFifo(ctx context.Context, o *overlayEntry, parent *Dirent, nam
 	if err := copyUpLockedForRename(ctx, parent); err != nil {
 		return err
 	}
-	return o.upper.InodeOperations.CreateFifo(ctx, o.upper, name, perm)
+	if err := o.upper.InodeOperations.CreateFifo(ctx, o.upper, name, perm); err != nil {
+		return err
+	}
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+	return nil
 }
 
 func overlayRemove(ctx context.Context, o *overlayEntry, parent *Dirent, child *Dirent) error {
@@ -318,6 +341,8 @@ func overlayRemove(ctx context.Context, o *overlayEntry, parent *Dirent, child *
 	if child.Inode.overlay.lowerExists {
 		return overlayCreateWhiteout(o.upper, child.name)
 	}
+	// We've removed from the directory so we must drop the cache.
+	o.markDirectoryDirty()
 	return nil
 }
 
@@ -395,6 +420,8 @@ func overlayRename(ctx context.Context, o *overlayEntry, oldParent *Dirent, rena
 	if renamed.Inode.overlay.lowerExists {
 		return overlayCreateWhiteout(oldParent.Inode.overlay.upper, oldName)
 	}
+	// We've changed the directory so we must drop the cache.
+	o.markDirectoryDirty()
 	return nil
 }
 
@@ -411,6 +438,9 @@ func overlayBind(ctx context.Context, o *overlayEntry, parent *Dirent, name stri
 		return nil, err
 	}
 
+	// We've added to the directory so we must drop the cache.
+	o.markDirectoryDirty()
+
 	// Grab the inode and drop the dirent, we don't need it.
 	inode := d.Inode
 	inode.IncRef()

pkg/sentry/fs/mock.go

@@ -75,6 +75,12 @@ func (n *MockMountSourceOps) Keep(dirent *Dirent) bool {
 	return n.keep
 }
 
+// CacheReaddir implements fs.MountSourceOperations.CacheReaddir.
+func (n *MockMountSourceOps) CacheReaddir() bool {
+	// Common case: cache readdir results if there is a dirent cache.
+	return n.keep
+}
+
 // WriteOut implements fs.InodeOperations.WriteOut.
 func (n *MockInodeOperations) WriteOut(context.Context, *Inode) error {
 	return nil

pkg/sentry/fs/mount.go

@@ -23,8 +23,8 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/context"
 )
 
-// DirentOperations provide file systems greater control over how long a Dirent stays pinned
-// in core. Implementations must not take Dirent.mu.
+// DirentOperations provide file systems greater control over how long a Dirent
+// stays pinned in core. Implementations must not take Dirent.mu.
 type DirentOperations interface {
 	// Revalidate is called during lookup each time we encounter a Dirent
 	// in the cache. Implementations may update stale properties of the
@@ -37,6 +37,12 @@ type DirentOperations interface {
 	// Keep returns true if the Dirent should be kept in memory for as long
 	// as possible beyond any active references.
 	Keep(dirent *Dirent) bool
+
+	// CacheReaddir returns true if directory entries returned by
+	// FileOperations.Readdir may be cached for future use.
+	//
+	// Postconditions: This method must always return the same value.
+	CacheReaddir() bool
 }
 
 // MountSourceOperations contains filesystem specific operations.
@@ -190,25 +196,28 @@ func (msrc *MountSource) SetDirentCacheLimiter(l *DirentCacheLimiter) {
 // aggressively.
 func NewCachingMountSource(ctx context.Context, filesystem Filesystem, flags MountSourceFlags) *MountSource {
 	return NewMountSource(ctx, &SimpleMountSourceOperations{
-		keep:       true,
-		revalidate: false,
+		keep:         true,
+		revalidate:   false,
+		cacheReaddir: true,
 	}, filesystem, flags)
 }
 
 // NewNonCachingMountSource returns a generic mount that will never cache dirents.
 func NewNonCachingMountSource(ctx context.Context, filesystem Filesystem, flags MountSourceFlags) *MountSource {
 	return NewMountSource(ctx, &SimpleMountSourceOperations{
-		keep:       false,
-		revalidate: false,
+		keep:         false,
+		revalidate:   false,
+		cacheReaddir: false,
 	}, filesystem, flags)
 }
 
 // NewRevalidatingMountSource returns a generic mount that will cache dirents,
-// but will revalidate them on each lookup.
+// but will revalidate them on each lookup and always perform uncached readdir.
 func NewRevalidatingMountSource(ctx context.Context, filesystem Filesystem, flags MountSourceFlags) *MountSource {
 	return NewMountSource(ctx, &SimpleMountSourceOperations{
-		keep:       true,
-		revalidate: true,
+		keep:         true,
+		revalidate:   true,
+		cacheReaddir: false,
 	}, filesystem, flags)
 }
 
@@ -216,8 +225,9 @@ func NewRevalidatingMountSource(ctx context.Context, filesystem Filesystem, flag
 // an actual filesystem. It is always non-caching.
 func NewPseudoMountSource(ctx context.Context) *MountSource {
 	return NewMountSource(ctx, &SimpleMountSourceOperations{
-		keep:       false,
-		revalidate: false,
+		keep:         false,
+		revalidate:   false,
+		cacheReaddir: false,
 	}, nil, MountSourceFlags{})
 }
 
@@ -225,8 +235,9 @@ func NewPseudoMountSource(ctx context.Context) *MountSource {
 //
 // +stateify savable
 type SimpleMountSourceOperations struct {
-	keep       bool
-	revalidate bool
+	keep         bool
+	revalidate   bool
+	cacheReaddir bool
 }
 
 // Revalidate implements MountSourceOperations.Revalidate.
@@ -239,6 +250,11 @@ func (smo *SimpleMountSourceOperations) Keep(*Dirent) bool {
 	return smo.keep
 }
 
+// CacheReaddir implements MountSourceOperations.CacheReaddir.
+func (smo *SimpleMountSourceOperations) CacheReaddir() bool {
+	return smo.cacheReaddir
+}
+
 // ResetInodeMappings implements MountSourceOperations.ResetInodeMappings.
 func (*SimpleMountSourceOperations) ResetInodeMappings() {}
 

pkg/sentry/fs/mount_overlay.go

@@ -81,6 +81,17 @@ func (o *overlayMountSourceOperations) Keep(dirent *Dirent) bool {
 	return o.upper.Keep(dirent)
 }
 
+// CacheReaddir implements MountSourceOperations.CacheReaddir for an overlay by
+// performing the logical AND of the upper and lower filesystems' CacheReaddir
+// methods.
+//
+// N.B. This is fs-global instead of inode-specific because it must always
+// return the same value. If it was inode-specific, we couldn't guarantee that
+// property across copy up.
+func (o *overlayMountSourceOperations) CacheReaddir() bool {
+	return o.lower.CacheReaddir() && o.upper.CacheReaddir()
+}
+
 // ResetInodeMappings propagates the call to both upper and lower MountSource.
 func (o *overlayMountSourceOperations) ResetInodeMappings() {
 	o.upper.ResetInodeMappings()

pkg/sentry/fs/overlay.go

@@ -24,6 +24,7 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	"gvisor.dev/gvisor/pkg/sentry/usermem"
 	"gvisor.dev/gvisor/pkg/syserror"
+	"gvisor.dev/gvisor/third_party/gvsync"
 )
 
 // The virtual filesystem implements an overlay configuration. For a high-level
@@ -196,6 +197,12 @@ type overlayEntry struct {
 	// these locks is sufficient to read upper; holding all three for writing
 	// is required to mutate it.
 	upper *Inode
+
+	// dirCacheMu protects dirCache.
+	dirCacheMu gvsync.DowngradableRWMutex `state:"nosave"`
+
+	// dirCache is cache of DentAttrs from upper and lower Inodes.
+	dirCache *SortedDentryMap
 }
 
 // newOverlayEntry returns a new overlayEntry.
@@ -258,6 +265,17 @@ func (o *overlayEntry) isMappableLocked() bool {
 	return o.inodeLocked().Mappable() != nil
 }
 
+// markDirectoryDirty marks any cached data dirty for this directory. This is
+// necessary in order to ensure that this node does not retain stale state
+// throughout its lifetime across multiple open directory handles.
+//
+// Currently this means invalidating any readdir caches.
+func (o *overlayEntry) markDirectoryDirty() {
+	o.dirCacheMu.Lock()
+	o.dirCache = nil
+	o.dirCacheMu.Unlock()
+}
+
 // AddMapping implements memmap.Mappable.AddMapping.
 func (o *overlayEntry) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) error {
 	o.mapsMu.Lock()

pkg/sentry/fs/ramfs/dir.go

@@ -431,7 +431,7 @@ func (dfo *dirFileOperations) Seek(ctx context.Context, file *fs.File, whence fs
 }
 
 // IterateDir implements DirIterator.IterateDir.
-func (dfo *dirFileOperations) IterateDir(ctx context.Context, dirCtx *fs.DirCtx, offset int) (int, error) {
+func (dfo *dirFileOperations) IterateDir(ctx context.Context, d *fs.Dirent, dirCtx *fs.DirCtx, offset int) (int, error) {
 	dfo.dir.mu.Lock()
 	defer dfo.dir.mu.Unlock()
 

pkg/sentry/fs/tty/dir.go

@@ -307,7 +307,7 @@ type dirFileOperations struct {
 var _ fs.FileOperations = (*dirFileOperations)(nil)
 
 // IterateDir implements DirIterator.IterateDir.
-func (df *dirFileOperations) IterateDir(ctx context.Context, dirCtx *fs.DirCtx, offset int) (int, error) {
+func (df *dirFileOperations) IterateDir(ctx context.Context, d *fs.Dirent, dirCtx *fs.DirCtx, offset int) (int, error) {
 	df.di.mu.Lock()
 	defer df.di.mu.Unlock()
 

pkg/sentry/fs/tty/fs.go

@@ -94,6 +94,13 @@ func (superOperations) Keep(*fs.Dirent) bool {
 	return false
 }
 
+// CacheReaddir implements fs.DirentOperations.CacheReaddir.
+//
+// CacheReaddir returns false because entries change on master operations.
+func (superOperations) CacheReaddir() bool {
+	return false
+}
+
 // ResetInodeMappings implements MountSourceOperations.ResetInodeMappings.
 func (superOperations) ResetInodeMappings() {}
 

runsc/boot/fs.go

@@ -239,7 +239,11 @@ func mustFindFilesystem(name string) fs.Filesystem {
 // addSubmountOverlay overlays the inode over a ramfs tree containing the given
 // paths.
 func addSubmountOverlay(ctx context.Context, inode *fs.Inode, submounts []string) (*fs.Inode, error) {
-	msrc := fs.NewPseudoMountSource(ctx)
+	// Construct a ramfs tree of mount points. The contents never
+	// change, so this can be fully caching. There's no real
+	// filesystem backing this tree, so we set the filesystem to
+	// nil.
+	msrc := fs.NewCachingMountSource(ctx, nil, fs.MountSourceFlags{})
 	mountTree, err := ramfs.MakeDirectoryTree(ctx, msrc, submounts)
 	if err != nil {
 		return nil, fmt.Errorf("creating mount tree: %v", err)