From 5ebf3246df152dd83a6de84b899c6bc95762dfeb Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Thu, 22 Sep 2022 11:00:20 -0700 Subject: [PATCH] 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 | 17 +++++- pkg/sentry/fsimpl/gofer/directory.go | 2 + pkg/sentry/fsimpl/gofer/filesystem.go | 10 ++-- pkg/sentry/fsimpl/gofer/gofer.go | 69 +++++++++++++++---------- pkg/sentry/fsimpl/gofer/gofer_test.go | 9 ++-- pkg/sentry/fsimpl/gofer/save_restore.go | 10 ++-- pkg/sentry/fsimpl/gofer/special_file.go | 5 +- pkg/state/BUILD | 3 +- pkg/state/encode.go | 5 -- 9 files changed, 77 insertions(+), 53 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/BUILD b/pkg/sentry/fsimpl/gofer/BUILD index 36111538c..d2134db5a 100644 --- a/pkg/sentry/fsimpl/gofer/BUILD +++ b/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", diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index 6156f4990..eba8f641e 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/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) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 9ef905d2e..aecd652bb 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/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() diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index ee8430f2d..22da7c121 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/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() } diff --git a/pkg/sentry/fsimpl/gofer/gofer_test.go b/pkg/sentry/fsimpl/gofer/gofer_test.go index addd0c451..58a06e9fa 100644 --- a/pkg/sentry/fsimpl/gofer/gofer_test.go +++ b/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}, } diff --git a/pkg/sentry/fsimpl/gofer/save_restore.go b/pkg/sentry/fsimpl/gofer/save_restore.go index 0b152b130..5b42fddef 100644 --- a/pkg/sentry/fsimpl/gofer/save_restore.go +++ b/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 { diff --git a/pkg/sentry/fsimpl/gofer/special_file.go b/pkg/sentry/fsimpl/gofer/special_file.go index 06b3e235c..7336eaf11 100644 --- a/pkg/sentry/fsimpl/gofer/special_file.go +++ b/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() } diff --git a/pkg/state/BUILD b/pkg/state/BUILD index 92c51879b..b8a675970 100644 --- a/pkg/state/BUILD +++ b/pkg/state/BUILD @@ -11,8 +11,7 @@ go_template_instance( template = "//pkg/ilist:generic_list", types = { "Element": "*objectEncodeState", - "ElementMapper": "deferredMapper", - "Linker": "*deferredEntry", + "Linker": "*objectEncodeState", }, ) diff --git a/pkg/state/encode.go b/pkg/state/encode.go index 157ac0dd2..9f15c3c2e 100644 --- a/pkg/state/encode.go +++ b/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{}