Make syncableDentries and specialFileFDs linked lists in gofer client.

fs.syncableDentries saves all non-synthetic dentries. This requires a map insert
operation every time a new dentry is created and map removal operation when a
dentry is destroyed. This can be expensive there can be a very large number of
non-synthetic dentries.

Using a map does not provide any additional benefits. We do not require lookup.
Instead use a linked list, as insert and remove are really fast and it allows us
to iterate on the list. It also saves the heap allocations to maintain the map.

Also simplify pkg/state to not use a custom ElementMapper. There is no need to.

PiperOrigin-RevId: 476145150

pkg/sentry/fsimpl/gofer/BUILD

@@ -10,8 +10,20 @@ go_template_instance(
     prefix = "dentry",
     template = "//pkg/ilist:generic_list",
     types = {
-        "Element": "*dentry",
-        "Linker": "*dentry",
+        "Element": "*dentryListElem",
+        "Linker": "*dentryListElem",
+    },
+)
+
+go_template_instance(
+    name = "special_fd_list",
+    out = "special_fd_list.go",
+    package = "gofer",
+    prefix = "specialFD",
+    template = "//pkg/ilist:generic_list",
+    types = {
+        "Element": "*specialFileFD",
+        "Linker": "*specialFileFD",
     },
 )
 
@@ -41,6 +53,7 @@ go_library(
         "revalidate.go",
         "save_restore.go",
         "socket.go",
+        "special_fd_list.go",
         "special_file.go",
         "symlink.go",
         "time.go",

pkg/sentry/fsimpl/gofer/directory.go

@@ -140,6 +140,8 @@ func (d *dentry) createSyntheticChildLocked(opts *createSyntheticOpts) {
 		panic(fmt.Sprintf("failed to create synthetic file of unrecognized type: %v", opts.mode.FileType()))
 	}
 	child.pf.dentry = child
+	child.cacheEntry.d = child
+	child.syncableListEntry.d = child
 	child.vfsd.Init(child)
 
 	d.cacheNewChildLocked(child, opts.name)

pkg/sentry/fsimpl/gofer/filesystem.go

@@ -41,12 +41,12 @@ import (
 func (fs *filesystem) Sync(ctx context.Context) error {
 	// Snapshot current syncable dentries and special file FDs.
 	fs.syncMu.Lock()
-	ds := make([]*dentry, 0, len(fs.syncableDentries))
-	for d := range fs.syncableDentries {
-		ds = append(ds, d)
+	ds := make([]*dentry, 0, fs.syncableDentries.Len())
+	for elem := fs.syncableDentries.Front(); elem != nil; elem = elem.Next() {
+		ds = append(ds, elem.d)
 	}
-	sffds := make([]*specialFileFD, 0, len(fs.specialFileFDs))
-	for sffd := range fs.specialFileFDs {
+	sffds := make([]*specialFileFD, 0, fs.specialFileFDs.Len())
+	for sffd := fs.specialFileFDs.Front(); sffd != nil; sffd = sffd.Next() {
 		sffds = append(sffds, sffd)
 	}
 	fs.syncMu.Unlock()

pkg/sentry/fsimpl/gofer/gofer.go

@@ -181,8 +181,8 @@ type filesystem struct {
 	// syncableDentries contains all non-synthetic dentries. specialFileFDs
 	// contains all open specialFileFDs. These fields are protected by syncMu.
 	syncMu           sync.Mutex `state:"nosave"`
-	syncableDentries map[*dentry]struct{}
-	specialFileFDs   map[*specialFileFD]struct{}
+	syncableDentries dentryList
+	specialFileFDs   specialFDList
 
 	// inoByQIDPath maps previously-observed QID.Paths to inode numbers
 	// assigned to those paths. inoByQIDPath is not preserved across
@@ -487,15 +487,13 @@ func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt
 		return nil, nil, err
 	}
 	fs := &filesystem{
-		mfp:              mfp,
-		opts:             fsopts,
-		iopts:            iopts,
-		clock:            ktime.RealtimeClockFromContext(ctx),
-		devMinor:         devMinor,
-		syncableDentries: make(map[*dentry]struct{}),
-		specialFileFDs:   make(map[*specialFileFD]struct{}),
-		inoByQIDPath:     make(map[uint64]uint64),
-		inoByKey:         make(map[inoKey]uint64),
+		mfp:          mfp,
+		opts:         fsopts,
+		iopts:        iopts,
+		clock:        ktime.RealtimeClockFromContext(ctx),
+		devMinor:     devMinor,
+		inoByQIDPath: make(map[uint64]uint64),
+		inoByKey:     make(map[inoKey]uint64),
 	}
 
 	// Did the user configure a global dentry cache?
@@ -681,7 +679,8 @@ func (fs *filesystem) Release(ctx context.Context) {
 
 	mf := fs.mfp.MemoryFile()
 	fs.syncMu.Lock()
-	for d := range fs.syncableDentries {
+	for elem := fs.syncableDentries.Front(); elem != nil; elem = elem.Next() {
+		d := elem.d
 		d.handleMu.Lock()
 		d.dataMu.Lock()
 		if h := d.writeHandleLocked(); h.isOpen() {
@@ -839,11 +838,17 @@ type dentry struct {
 	// this dentry.
 	cachingMu sync.Mutex `state:"nosave"`
 
-	// If cached is true, dentryEntry links dentry into
-	// filesystem.dentryCache.dentries. cached and dentryEntry are protected by
-	// cachingMu.
+	// If cached is true, this dentry is part of filesystem.dentryCache. cached
+	// is protected by cachingMu.
 	cached bool
-	dentryEntry
+
+	// cacheEntry links dentry into filesystem.dentryCache.dentries. It is
+	// protected by filesystem.dentryCache.mu.
+	cacheEntry dentryListElem
+
+	// syncableListEntry links dentry into filesystem.syncableDentries. It is
+	// protected by filesystem.syncMu.
+	syncableListEntry dentryListElem
 
 	dirMu sync.Mutex `state:"nosave"`
 
@@ -995,6 +1000,13 @@ type dentry struct {
 	watches vfs.Watches
 }
 
+// +stateify savable
+type dentryListElem struct {
+	// d is the dentry that this elem represents.
+	d *dentry
+	dentryEntry
+}
+
 // dentryAttrMask returns a p9.AttrMask enabling all attributes used by the
 // gofer client.
 func dentryAttrMask() p9.AttrMask {
@@ -1040,6 +1052,8 @@ func (fs *filesystem) newDentry(ctx context.Context, file p9file, qid p9.QID, ma
 		mmapFD:    atomicbitops.FromInt32(-1),
 	}
 	d.pf.dentry = d
+	d.cacheEntry.d = d
+	d.syncableListEntry.d = d
 	if mask.UID {
 		d.uid = atomicbitops.FromUint32(dentryUIDFromP9UID(attr.UID))
 	}
@@ -1083,7 +1097,7 @@ func (fs *filesystem) newDentry(ctx context.Context, file p9file, qid p9.QID, ma
 	d.vfsd.Init(d)
 	refsvfs2.Register(d)
 	fs.syncMu.Lock()
-	fs.syncableDentries[d] = struct{}{}
+	fs.syncableDentries.PushBack(&d.syncableListEntry)
 	fs.syncMu.Unlock()
 	return d, nil
 }
@@ -1112,8 +1126,9 @@ func (fs *filesystem) newDentryLisa(ctx context.Context, ino *lisafs.Inode) (*de
 		mmapFD:        atomicbitops.FromInt32(-1),
 		controlFDLisa: fs.clientLisa.NewFD(ino.ControlFD),
 	}
-
 	d.pf.dentry = d
+	d.cacheEntry.d = d
+	d.syncableListEntry.d = d
 	if ino.Stat.Mask&linux.STATX_UID != 0 {
 		d.uid = atomicbitops.FromUint32(dentryUIDFromLisaUID(lisafs.UID(ino.Stat.UID)))
 	}
@@ -1157,7 +1172,7 @@ func (fs *filesystem) newDentryLisa(ctx context.Context, ino *lisafs.Inode) (*de
 	d.vfsd.Init(d)
 	refsvfs2.Register(d)
 	fs.syncMu.Lock()
-	fs.syncableDentries[d] = struct{}{}
+	fs.syncableDentries.PushBack(&d.syncableListEntry)
 	fs.syncMu.Unlock()
 	return d, nil
 }
@@ -1928,15 +1943,15 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo
 	d.fs.dentryCache.mu.Lock()
 	// If d is already cached, just move it to the front of the LRU.
 	if d.cached {
-		d.fs.dentryCache.dentries.Remove(d)
-		d.fs.dentryCache.dentries.PushFront(d)
+		d.fs.dentryCache.dentries.Remove(&d.cacheEntry)
+		d.fs.dentryCache.dentries.PushFront(&d.cacheEntry)
 		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.dentryCache.dentries.PushFront(d)
+	d.fs.dentryCache.dentries.PushFront(&d.cacheEntry)
 	d.fs.dentryCache.dentriesLen++
 	d.cached = true
 	shouldEvict := d.fs.dentryCache.dentriesLen > d.fs.dentryCache.maxCachedDentries
@@ -1958,7 +1973,7 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo
 func (d *dentry) removeFromCacheLocked() {
 	if d.cached {
 		d.fs.dentryCache.mu.Lock()
-		d.fs.dentryCache.dentries.Remove(d)
+		d.fs.dentryCache.dentries.Remove(&d.cacheEntry)
 		d.fs.dentryCache.dentriesLen--
 		d.fs.dentryCache.mu.Unlock()
 		d.cached = false
@@ -1988,8 +2003,8 @@ func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) {
 		return
 	}
 
-	if victim.fs == fs {
-		victim.evictLocked(ctx) // +checklocksforce: owned as precondition, victim.fs == fs
+	if victim.d.fs == fs {
+		victim.d.evictLocked(ctx) // +checklocksforce: owned as precondition, victim.fs == fs
 		return
 	}
 
@@ -1999,7 +2014,7 @@ func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) {
 	// each others' renameMu.
 	fs.renameMu.Unlock()
 	defer fs.renameMu.Lock()
-	victim.evict(ctx)
+	victim.d.evict(ctx)
 }
 
 // Preconditions:
@@ -2140,7 +2155,7 @@ func (d *dentry) destroyLocked(ctx context.Context) {
 
 		// Remove d from the set of syncable dentries.
 		d.fs.syncMu.Lock()
-		delete(d.fs.syncableDentries, d)
+		d.fs.syncableDentries.Remove(&d.syncableListEntry)
 		d.fs.syncMu.Unlock()
 	}
 

pkg/sentry/fsimpl/gofer/gofer_test.go

@@ -26,11 +26,10 @@ import (
 func TestDestroyIdempotent(t *testing.T) {
 	ctx := contexttest.Context(t)
 	fs := filesystem{
-		mfp:              pgalloc.MemoryFileProviderFromContext(ctx),
-		syncableDentries: make(map[*dentry]struct{}),
-		inoByQIDPath:     make(map[uint64]uint64),
-		inoByKey:         make(map[inoKey]uint64),
-		clock:            time.RealtimeClockFromContext(ctx),
+		mfp:          pgalloc.MemoryFileProviderFromContext(ctx),
+		inoByQIDPath: make(map[uint64]uint64),
+		inoByKey:     make(map[inoKey]uint64),
+		clock:        time.RealtimeClockFromContext(ctx),
 		// Test relies on no dentry being held in the cache.
 		dentryCache: &dentryCache{maxCachedDentries: 0},
 	}

pkg/sentry/fsimpl/gofer/save_restore.go

@@ -61,7 +61,7 @@ func (fs *filesystem) PrepareSave(ctx context.Context) error {
 	// Buffer pipe data so that it's available for reading after restore. (This
 	// is a legacy VFS1 feature.)
 	fs.syncMu.Lock()
-	for sffd := range fs.specialFileFDs {
+	for sffd := fs.specialFileFDs.Front(); sffd != nil; sffd = sffd.Next() {
 		if sffd.dentry().fileType() == linux.S_IFIFO && sffd.vfsfd.IsReadable() {
 			if err := sffd.savePipeData(ctx); err != nil {
 				fs.syncMu.Unlock()
@@ -231,7 +231,7 @@ func (fs *filesystem) CompleteRestore(ctx context.Context, opts vfs.CompleteRest
 	// ENXIO if another specialFileFD represents the read end of the same pipe.
 	// This is consistent with VFS1.
 	haveWriteOnlyPipes := false
-	for fd := range fs.specialFileFDs {
+	for fd := fs.specialFileFDs.Front(); fd != nil; fd = fd.Next() {
 		if fd.dentry().fileType() == linux.S_IFIFO && !fd.vfsfd.IsReadable() {
 			haveWriteOnlyPipes = true
 			continue
@@ -241,7 +241,7 @@ func (fs *filesystem) CompleteRestore(ctx context.Context, opts vfs.CompleteRest
 		}
 	}
 	if haveWriteOnlyPipes {
-		for fd := range fs.specialFileFDs {
+		for fd := fs.specialFileFDs.Front(); fd != nil; fd = fd.Next() {
 			if fd.dentry().fileType() == linux.S_IFIFO && !fd.vfsfd.IsReadable() {
 				if err := fd.completeRestore(ctx); err != nil {
 					return err
@@ -360,8 +360,8 @@ func (d *dentry) restoreDescendantsRecursive(ctx context.Context, opts *vfs.Comp
 		if child == nil {
 			continue
 		}
-		if _, ok := d.fs.syncableDentries[child]; !ok {
-			// child is synthetic.
+		// child is synthetic if it does not exist in fs.syncableDentries.
+		if child.syncableListEntry.Next() == nil && child.syncableListEntry.Prev() == nil && d.fs.syncableDentries.Front() != &child.syncableListEntry {
 			continue
 		}
 		if err := child.restoreRecursive(ctx, opts); err != nil {

pkg/sentry/fsimpl/gofer/special_file.go

@@ -44,6 +44,7 @@ import (
 // +stateify savable
 type specialFileFD struct {
 	fileDescription
+	specialFDEntry
 
 	// releaseMu synchronizes the closing of fd.handle with fd.sync(). It's safe
 	// to access fd.handle without locking for operations that require a ref to
@@ -116,7 +117,7 @@ func newSpecialFileFD(h handle, mnt *vfs.Mount, d *dentry, flags uint32) (*speci
 		return nil, err
 	}
 	d.fs.syncMu.Lock()
-	d.fs.specialFileFDs[fd] = struct{}{}
+	d.fs.specialFileFDs.PushBack(fd)
 	d.fs.syncMu.Unlock()
 	if fd.vfsfd.IsWritable() && (d.mode.Load()&0111 != 0) {
 		metric.SuspiciousOperationsMetric.Increment("opened_write_execute_file")
@@ -140,7 +141,7 @@ func (fd *specialFileFD) Release(ctx context.Context) {
 
 	fs := fd.vfsfd.Mount().Filesystem().Impl().(*filesystem)
 	fs.syncMu.Lock()
-	delete(fs.specialFileFDs, fd)
+	fs.specialFileFDs.Remove(fd)
 	fs.syncMu.Unlock()
 }
 

pkg/state/BUILD

@@ -11,8 +11,7 @@ go_template_instance(
     template = "//pkg/ilist:generic_list",
     types = {
         "Element": "*objectEncodeState",
-        "ElementMapper": "deferredMapper",
-        "Linker": "*deferredEntry",
+        "Linker": "*objectEncodeState",
     },
 )
 

pkg/state/encode.go

@@ -839,11 +839,6 @@ func WriteHeader(w wire.Writer, length uint64, object bool) error {
 	})
 }
 
-// deferredMapper is for the deferred list.
-type deferredMapper struct{}
-
-func (deferredMapper) linkerFor(oes *objectEncodeState) *deferredEntry { return &oes.deferredEntry }
-
 // addrSetFunctions is used by addrSet.
 type addrSetFunctions struct{}