From 8342c57ae8705630f16d62583cba424e5d76d0b5 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Thu, 26 Jan 2023 14:53:31 -0800 Subject: [PATCH] Split gofer.dentry.dirMu into two mutexes: * gofer.dentry.opMu is an RW mutex that serializes high-level operations on the dentry. Operations that mutate the dentry must hold this lock for writing. * gofer.dentry.childrenMu protects cached children state. With these two mutexes, we can avoid serialing lookup operations in a directory in the common case where the children exist. PiperOrigin-RevId: 504949043 --- pkg/sentry/fsimpl/gofer/dentry_impl.go | 6 +- pkg/sentry/fsimpl/gofer/directory.go | 48 +++++-- pkg/sentry/fsimpl/gofer/filesystem.go | 174 ++++++++++++++++++----- pkg/sentry/fsimpl/gofer/gofer.go | 123 ++++++++++------ pkg/sentry/fsimpl/gofer/gofer_test.go | 8 +- pkg/sentry/fsimpl/gofer/lisafs_dentry.go | 48 +++++-- pkg/sentry/fsimpl/gofer/revalidate.go | 16 ++- pkg/sentry/fsimpl/gofer/save_restore.go | 6 +- 8 files changed, 309 insertions(+), 120 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/dentry_impl.go b/pkg/sentry/fsimpl/gofer/dentry_impl.go index 0ee85b248..eaeee86da 100644 --- a/pkg/sentry/fsimpl/gofer/dentry_impl.go +++ b/pkg/sentry/fsimpl/gofer/dentry_impl.go @@ -188,6 +188,8 @@ func (d *dentry) destroyImpl(ctx context.Context) { } // Postcondition: Caller must do dentry caching appropriately. +// +// +checklocksread:d.opMu func (d *dentry) getRemoteChild(ctx context.Context, name string) (*dentry, error) { switch dt := d.impl.(type) { case *lisafsDentry: @@ -199,12 +201,14 @@ func (d *dentry) getRemoteChild(ctx context.Context, name string) (*dentry, erro // Preconditions: // - fs.renameMu must be locked. -// - parent.dirMu must be locked. +// - parent.opMu must be locked for reading. // - parent.isDir(). // - !rp.Done() && rp.Component() is not "." or "..". // - dentry at name must not already exist in dentry tree. // // Postcondition: The returned dentry is already cached appropriately. +// +// +checklocksread:d.opMu func (d *dentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { switch dt := d.impl.(type) { case *lisafsDentry: diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index 7c87d3d72..1324ccb30 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/pkg/sentry/fsimpl/gofer/directory.go @@ -33,28 +33,46 @@ func (d *dentry) isDir() bool { return d.fileType() == linux.S_IFDIR } +// cacheNewChildLocked will cache the new child dentry, and will panic if a +// non-negative child is already cached. It is the caller's responsibility to +// check that the child does not exist before calling this method. +// // Preconditions: // - filesystem.renameMu must be locked. -// - d.dirMu must be locked. +// - If the addition to the dentry tree is due to a read-only operation (like +// Walk), then d.opMu must be held for reading. Otherwise d.opMu must be +// held for writing. +// - d.childrenMu must be locked. // - d.isDir(). // - child must be a newly-created dentry that has never had a parent. +// - d.children[name] must be unset or nil (a "negative child") +// +// +checklocksread:d.opMu +// +checklocks:d.childrenMu func (d *dentry) cacheNewChildLocked(child *dentry, name string) { d.IncRef() // reference held by child on its parent child.parent = d child.name = name if d.children == nil { d.children = make(map[string]*dentry) - } else if c, ok := d.children[name]; ok && c == nil { - // This child will not be negative, decrease count of negativeChildren. + } else if c, ok := d.children[name]; ok { + if c != nil { + panic(fmt.Sprintf("cacheNewChildLocked collision; child with name=%q already cached", name)) + } + + // Cached child is negative. OK to cache over, but we must + // update the count of negative children. d.negativeChildren-- } d.children[name] = child } // Preconditions: -// - d.dirMu must be locked. +// - d.childrenMu must be locked. // - d.isDir(). // - name is not already a negative entry. +// +// +checklocks:d.childrenMu func (d *dentry) cacheNegativeLookupLocked(name string) { // Don't cache negative lookups if InteropModeShared is in effect (since // this makes remote lookup unavoidable), or if d.isSynthetic() (in which @@ -139,7 +157,9 @@ func (fs *filesystem) newSyntheticDentry(opts *createSyntheticOpts) *dentry { } // Preconditions: -// - d.dirMu must be locked. +// - d.childrenMu must be locked. +// +// +checklocks:d.childrenMu func (d *dentry) clearDirentsLocked() { d.dirents = nil d.childrenSet = nil @@ -194,7 +214,7 @@ func (d *dentry) getDirents(ctx context.Context) ([]vfs.Dirent, error) { // presence of concurrent mutation of an iterated directory, so // implementations may duplicate or omit entries in this case, which // violates POSIX semantics. Thus we read all directory entries while - // holding d.dirMu to exclude directory mutations. (Note that it is + // holding d.opMu to exclude directory mutations. (Note that it is // impossible for the client to exclude concurrent mutation from other // remote filesystem users. Since there is no way to detect if the server // has incorrectly omitted directory entries, we simply assume that the @@ -204,11 +224,20 @@ func (d *dentry) getDirents(ctx context.Context) ([]vfs.Dirent, error) { // to readdir RPCs), but is consistent with VFS1. // filesystem.renameMu is needed for d.parent, and must be locked before - // dentry.dirMu. + // d.opMu. d.fs.renameMu.RLock() defer d.fs.renameMu.RUnlock() - d.dirMu.Lock() - defer d.dirMu.Unlock() + d.opMu.RLock() + defer d.opMu.RUnlock() + + // d.childrenMu must be locked after d.opMu and held for the entire + // function. This synchronizes concurrent getDirents() attempts. + // getdents(2) advances the file offset. To get complete results from + // multiple getdents(2) calls, the directory FD's offset needs to be + // protected. + d.childrenMu.Lock() + defer d.childrenMu.Unlock() + if d.dirents != nil { return d.dirents, nil } @@ -261,6 +290,7 @@ func (d *dentry) getDirents(ctx context.Context) ([]vfs.Dirent, error) { return nil, err } } + // Emit entries for synthetic children. if d.syntheticChildren != 0 { for _, child := range d.children { diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 74c24f0a1..0400b33f3 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -171,10 +171,12 @@ func (fs *filesystem) renameMuUnlockAndCheckCaching(ctx context.Context, ds **[] // // Preconditions: // - fs.renameMu must be locked. -// - d.dirMu must be locked. +// - d.opMu must be locked for reading. // - !rp.Done(). // - If !d.cachedMetadataAuthoritative(), then d and all children that are // part of rp must have been revalidated. +// +// +checklocksread:d.opMu func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, bool, error) { if !d.isDir() { return nil, false, linuxerr.ENOTDIR @@ -224,28 +226,62 @@ func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d * // // Preconditions: // - fs.renameMu must be locked. -// - parent.dirMu must be locked. +// - parent.opMu must be locked. // - parent.isDir(). // - name is not "." or "..". // - parent and the dentry at name have been revalidated. +// +// +checklocks:parent.opMu func (fs *filesystem) getChildLocked(ctx context.Context, parent *dentry, name string, ds **[]*dentry) (*dentry, error) { if child, err := parent.getCachedChildLocked(name); child != nil || err != nil { return child, err } - return fs.getRemoteChildLocked(ctx, parent, name, ds) + // We don't need to check for race here because parent.opMu is held for + // writing. + return fs.getRemoteChildLocked(ctx, parent, name, false /* checkForRace */, ds) } // getRemoteChildLocked is similar to getChildLocked, with the additional // precondition that the child identified by name does not exist in cache. -func (fs *filesystem) getRemoteChildLocked(ctx context.Context, parent *dentry, name string, ds **[]*dentry) (*dentry, error) { +// +// If checkForRace argument is true, then this method will check to see if the +// call has raced with another getRemoteChild call, and will handle the race if +// so. +// +// Preconditions: +// - If checkForRace is false, then parent.opMu must be held for writing. +// - Otherwise, parent.opMu must be held for reading. +// +// +checklocksread:parent.opMu +func (fs *filesystem) getRemoteChildLocked(ctx context.Context, parent *dentry, name string, checkForRace bool, ds **[]*dentry) (*dentry, error) { child, err := parent.getRemoteChild(ctx, name) // Cache the result appropriately in the dentry tree. if err != nil { if linuxerr.Equals(linuxerr.ENOENT, err) { + parent.childrenMu.Lock() + defer parent.childrenMu.Unlock() parent.cacheNegativeLookupLocked(name) } return nil, err } + + parent.childrenMu.Lock() + defer parent.childrenMu.Unlock() + + if checkForRace { + // See if we raced with anoter getRemoteChild call that added + // to the cache. + if cachedChild, ok := parent.children[name]; ok && cachedChild != nil { + // We raced. Destroy our child and return the cached + // one. This child has no handles, no data, and has not + // been cached, so destruction is quick and painless. + child.destroyDisconnected(ctx) + + // All good. Return the cached child. + return cachedChild, nil + } + // No race, continue with the child we got. + } parent.cacheNewChildLocked(child, name) appendNewChildDentry(ds, parent, child) return child, nil @@ -253,6 +289,8 @@ func (fs *filesystem) getRemoteChildLocked(ctx context.Context, parent *dentry, // getChildAndWalkPathLocked is the same as getChildLocked, except that it // may prefetch the entire path represented by rp. +// +// +checklocksread:parent.opMu func (fs *filesystem) getChildAndWalkPathLocked(ctx context.Context, parent *dentry, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { if child, err := parent.getCachedChildLocked(rp.Component()); child != nil || err != nil { return child, err @@ -266,14 +304,18 @@ func (fs *filesystem) getChildAndWalkPathLocked(ctx context.Context, parent *den // // Preconditions: // - fs.renameMu must be locked. -// - d.dirMu must be locked. +// - d.opMu must be locked for reading. // - d.isDir(). // - name is not "." or "..". // - d and the dentry at name have been revalidated. +// +// +checklocksread:d.opMu func (d *dentry) getCachedChildLocked(name string) (*dentry, error) { if len(name) > MaxFilenameLen { return nil, linuxerr.ENAMETOOLONG } + d.childrenMu.Lock() + defer d.childrenMu.Unlock() if child, ok := d.children[name]; ok || d.isSynthetic() { if child == nil { return nil, linuxerr.ENOENT @@ -305,9 +347,9 @@ func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.Resolving return nil, err } for !rp.Final() { - d.dirMu.Lock() + d.opMu.RLock() next, followedSymlink, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) - d.dirMu.Unlock() + d.opMu.RUnlock() if err != nil { return nil, err } @@ -333,9 +375,9 @@ func (fs *filesystem) resolveLocked(ctx context.Context, rp *vfs.ResolvingPath, return nil, err } for !rp.Done() { - d.dirMu.Lock() + d.opMu.RLock() next, followedSymlink, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) - d.dirMu.Unlock() + d.opMu.RUnlock() if err != nil { return nil, err } @@ -385,8 +427,8 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir return err } - parent.dirMu.Lock() - defer parent.dirMu.Unlock() + parent.opMu.Lock() + defer parent.opMu.Unlock() if len(name) > MaxFilenameLen { return linuxerr.ENAMETOOLONG @@ -395,14 +437,18 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir // don't check for existence just yet. We will check for existence if the // checks for writability fail below. Existence check is done by the creation // RPCs themselves. + parent.childrenMu.Lock() if child, ok := parent.children[name]; ok && child != nil { + parent.childrenMu.Unlock() return linuxerr.EEXIST } if parent.childrenSet != nil { if _, ok := parent.childrenSet[name]; ok { + parent.childrenMu.Unlock() return linuxerr.EEXIST } } + parent.childrenMu.Unlock() checkExistence := func() error { if child, err := fs.getChildLocked(ctx, parent, name, &ds); err != nil && !linuxerr.Equals(linuxerr.ENOENT, err) { return err @@ -440,10 +486,12 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir if err != nil { return err } + parent.childrenMu.Lock() parent.cacheNewChildLocked(child, name) parent.syntheticChildren++ - parent.touchCMtime() parent.clearDirentsLocked() + parent.childrenMu.Unlock() + parent.touchCMtime() ev := linux.IN_CREATE if dir { ev |= linux.IN_ISDIR @@ -458,6 +506,7 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir if err != nil { return err } + parent.childrenMu.Lock() parent.cacheNewChildLocked(child, name) if child.isSynthetic() { parent.syntheticChildren++ @@ -471,9 +520,10 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir delete(parent.children, name) parent.negativeChildren-- } - parent.touchCMtime() parent.clearDirentsLocked() + parent.touchCMtime() } + parent.childrenMu.Unlock() ev := linux.IN_CREATE if dir { ev |= linux.IN_ISDIR @@ -522,21 +572,26 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b mntns := vfs.MountNamespaceFromContext(ctx) defer mntns.DecRef(ctx) - parent.dirMu.Lock() - defer parent.dirMu.Unlock() + parent.opMu.Lock() + defer parent.opMu.Unlock() + parent.childrenMu.Lock() if parent.childrenSet != nil { if _, ok := parent.childrenSet[name]; !ok { + parent.childrenMu.Unlock() return linuxerr.ENOENT } } + parent.childrenMu.Unlock() // Load child if sticky bit is set because we need to determine whether // deletion is allowed. var child *dentry if parent.mode.Load()&linux.ModeSticky == 0 { var ok bool + parent.childrenMu.Lock() child, ok = parent.children[name] + parent.childrenMu.Unlock() if ok && child == nil { // Hit a negative cached entry, child doesn't exist. return linuxerr.ENOENT @@ -557,16 +612,16 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b // // Also note that if child is nil, then it can't be a mount point. if child != nil { - // Hold child.dirMu so we can check child.children and + // Hold child.childrenMu so we can check child.children and // child.syntheticChildren. We don't access these fields until a bit later, - // but locking child.dirMu after calling vfs.PrepareDeleteDentry() would - // create an inconsistent lock ordering between dentry.dirMu and - // vfs.Dentry.mu (in the VFS lock order, it would make dentry.dirMu both "a + // but locking child.childrenMu after calling vfs.PrepareDeleteDentry() would + // create an inconsistent lock ordering between dentry.childrenMu and + // vfs.Dentry.mu (in the VFS lock order, it would make dentry.childrenMu both "a // FilesystemImpl lock" and "a lock acquired by a FilesystemImpl between // PrepareDeleteDentry and CommitDeleteDentry). To avoid this, lock - // child.dirMu before calling PrepareDeleteDentry. - child.dirMu.Lock() - defer child.dirMu.Unlock() + // child.childrenMu before calling PrepareDeleteDentry. + child.childrenMu.Lock() + defer child.childrenMu.Unlock() if err := vfsObj.PrepareDeleteDentry(mntns, &child.vfsd); err != nil { return err } @@ -576,7 +631,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b if dir { if child != nil { // child must be an empty directory. - if child.syntheticChildren != 0 { + if child.syntheticChildren != 0 { // +checklocksforce: child.childrenMu is held if child != nil. // This is definitely not an empty directory, irrespective of // fs.opts.interop. vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: PrepareDeleteDentry called if child != nil. @@ -592,7 +647,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above. return linuxerr.ENOTDIR } - for _, grandchild := range child.children { + for _, grandchild := range child.children { // +checklocksforce: child.childrenMu is held if child != nil. if grandchild != nil { vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above. return linuxerr.ENOTEMPTY @@ -638,6 +693,9 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b vfs.InotifyRemoveChild(ctx, cw, &parent.watches, name) } + parent.childrenMu.Lock() + defer parent.childrenMu.Unlock() + if child != nil { vfsObj.CommitDeleteDentry(ctx, &child.vfsd) // +checklocksforce: see above. child.setDeleted() @@ -812,7 +870,7 @@ func (fs *filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v // to creating a synthetic one, i.e. one that is kept entirely in memory. // Check that we're not overriding an existing file with a synthetic one. - _, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, ds) + _, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, ds) // +checklocksforce: parent.opMu taken by doCreateAt. switch { case err == nil: // Step succeeded, another file exists. @@ -908,10 +966,13 @@ afterTrailingSymlink: return nil, err } // Determine whether or not we need to create a file. - parent.dirMu.Lock() + // NOTE(b/263297063): Don't hold opMu for writing here, to avoid + // serializing OpenAt calls in the same directory in the common case + // that the file exists. + parent.opMu.RLock() child, followedSymlink, err := fs.stepLocked(ctx, rp, parent, true /* mayFollowSymlinks */, &ds) + parent.opMu.RUnlock() if followedSymlink { - parent.dirMu.Unlock() if mustCreate { // EEXIST must be returned if an existing symlink is opened with O_EXCL. return nil, linuxerr.EEXIST @@ -926,14 +987,32 @@ afterTrailingSymlink: } if linuxerr.Equals(linuxerr.ENOENT, err) && mayCreate { if parent.isSynthetic() { - parent.dirMu.Unlock() return nil, linuxerr.EPERM } - fd, err := parent.createAndOpenChildLocked(ctx, rp, &opts, &ds) - parent.dirMu.Unlock() - return fd, err + + // Take opMu for writing, but note that the file may have been + // created by another goroutine since we checked for existence + // a few lines ago. We must handle that case. + parent.opMu.Lock() + fd, createErr := parent.createAndOpenChildLocked(ctx, rp, &opts, &ds) + if !linuxerr.Equals(linuxerr.EEXIST, createErr) { + // Either the creation was a success, or we got an + // unexpected error. Either way we can return here. + parent.opMu.Unlock() + return fd, createErr + } + + // We raced, and now the file exists. + if mustCreate { + parent.opMu.Unlock() + return nil, linuxerr.EEXIST + } + + // Step to the file again. Since we still hold opMu for + // writing, there can't be a race here. + child, _, err = fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds) + parent.opMu.Unlock() } - parent.dirMu.Unlock() if err != nil { return nil, err } @@ -1111,8 +1190,10 @@ retry: // Preconditions: // - d.fs.renameMu must be locked. -// - d.dirMu must be locked. +// - d.opMu must be locked for writing. // - !d.isSynthetic(). +// +// +checklocks:d.opMu func (d *dentry) createAndOpenChildLocked(ctx context.Context, rp *vfs.ResolvingPath, opts *vfs.OpenOptions, ds **[]*dentry) (*vfs.FileDescription, error) { if err := d.checkPermissions(rp.Credentials(), vfs.MayWrite); err != nil { return nil, err @@ -1160,12 +1241,16 @@ func (d *dentry) createAndOpenChildLocked(ctx context.Context, rp *vfs.Resolving child.handleMu.Unlock() } // Insert the dentry into the tree. + d.childrenMu.Lock() + // We have d.opMu for writing, so there can not be a cached child with + // this name. We could not have raced. d.cacheNewChildLocked(child, name) appendNewChildDentry(ds, d, child) if d.cachedMetadataAuthoritative() { d.touchCMtime() d.clearDirentsLocked() } + d.childrenMu.Unlock() // Finally, construct a file description representing the created file. var childVFSFD *vfs.FileDescription @@ -1262,8 +1347,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa // We need a dentry representing the renamed file since, if it's a // directory, we need to check for write permission on it. - oldParent.dirMu.Lock() - defer oldParent.dirMu.Unlock() + oldParent.opMu.Lock() + defer oldParent.opMu.Unlock() renamed, err := fs.getChildLocked(ctx, oldParent, oldName, &ds) if err != nil { return err @@ -1290,13 +1375,13 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if err := newParent.checkPermissions(creds, vfs.MayWrite|vfs.MayExec); err != nil { return err } - newParent.dirMu.Lock() - defer newParent.dirMu.Unlock() + newParent.opMu.Lock() + defer newParent.opMu.Unlock() } if newParent.isDeleted() { return linuxerr.ENOENT } - replaced, err := fs.getChildLocked(ctx, newParent, newName, &ds) + replaced, err := fs.getChildLocked(ctx, newParent, newName, &ds) // +checklocksforce: newParent.opMu taken if newParent != oldParent. if err != nil && !linuxerr.Equals(linuxerr.ENOENT, err) { return err } @@ -1349,6 +1434,13 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa } // Update the dentry tree. + newParent.childrenMu.Lock() + defer newParent.childrenMu.Unlock() + if oldParent != newParent { + oldParent.childrenMu.Lock() + defer oldParent.childrenMu.Unlock() + } + vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) if replaced != nil { replaced.setDeleted() @@ -1357,12 +1449,16 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa replaced.decRefNoCaching() } ds = appendDentry(ds, replaced) + // Remove the replaced entry from its parent's cache. + delete(newParent.children, newName) } - oldParent.cacheNegativeLookupLocked(oldName) + oldParent.cacheNegativeLookupLocked(oldName) // +checklocksforce: oldParent.childrenMu is held if oldParent != newParent. if renamed.isSynthetic() { oldParent.syntheticChildren-- newParent.syntheticChildren++ } + // We have d.opMu for writing, so no need to check for existence of a + // child with the given name. We could not have raced. newParent.cacheNewChildLocked(renamed, newName) oldParent.decRefNoCaching() if oldParent != newParent { diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 663125850..c4c568df5 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -21,7 +21,8 @@ // filesystem.renameMu // dentry.cachingMu // dentryCache.mu -// dentry.dirMu +// dentry.opMu +// dentry.childrenMu // filesystem.syncMu // dentry.metadataMu // *** "memmap.Mappable locks" below this point @@ -33,7 +34,7 @@ // specialFileFD.mu // specialFileFD.bufMu // -// Locking dentry.dirMu and dentry.metadataMu in multiple dentries requires that +// Locking dentry.opMu and dentry.metadataMu in multiple dentries requires that // either ancestor dentries are locked before descendant dentries, or that // filesystem.renameMu is locked for writing. package gofer @@ -700,11 +701,11 @@ func (d *dentry) releaseSyntheticRecursiveLocked(ctx context.Context) { } if d.isDir() { var children []*dentry - d.dirMu.Lock() + d.childrenMu.Lock() for _, child := range d.children { children = append(children, child) } - d.dirMu.Unlock() + d.childrenMu.Unlock() for _, child := range children { if child != nil { child.releaseSyntheticRecursiveLocked(ctx) @@ -780,7 +781,13 @@ type dentry struct { // protected by filesystem.syncMu. syncableListEntry dentryListElem - dirMu sync.Mutex `state:"nosave"` + // opMu synchronizes operations on this dentry. Operations that mutate + // the dentry tree must hold this lock for writing. Operations that + // only read the tree must hold for reading. + opMu sync.RWMutex `state:"nosave"` + + // childrenMu protects the cached children data for this dentry. + childrenMu sync.Mutex `state:"nosave"` // If this dentry represents a directory, children contains: // @@ -790,29 +797,36 @@ type dentry struct { // dentries (only if InteropModeShared is not in effect and the directory // is not synthetic). // - // children is protected by dirMu. + // +checklocks:childrenMu children map[string]*dentry // If this dentry represents a directory, negativeChildrenCache cache - // names of negative children, negativeChildrenCache is protected by dirMu. + // names of negative children. + // + // +checklocks:childrenMu negativeChildrenCache stringFixedCache // If this dentry represents a directory, negativeChildren is the number - // of negative children cached in dentry.children. negativeChildren is - // protected by dirMu. + // of negative children cached in dentry.children + // + // +checklocks:childrenMu negativeChildren int // If this dentry represents a directory, syntheticChildren is the number // of child dentries for which dentry.isSynthetic() == true. - // syntheticChildren is protected by dirMu. + // + // +checklocks:childrenMu syntheticChildren int // If this dentry represents a directory, - // dentry.cachedMetadataAuthoritative() == true, and dirents is not nil, it - // is a cache of all entries in the directory, in the order they were - // returned by the server. childrenSet just stores the `Name` field of all - // dirents in a set for fast query. dirents and childrenSet are protected by - // dirMu and share the same lifecycle. - dirents []vfs.Dirent + // dentry.cachedMetadataAuthoritative() == true, and dirents is not + // nil, then dirents is a cache of all entries in the directory, in the + // order they were returned by the server. childrenSet just stores the + // `Name` field of all dirents in a set for fast query. dirents and + // childrenSet share the same lifecycle. + // + // +checklocks:childrenMu + dirents []vfs.Dirent + // +checklocks:childrenMu childrenSet map[string]struct{} // Cached metadata; protected by metadataMu. @@ -1543,9 +1557,9 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo defer d.fs.renameMu.Unlock() } if d.parent != nil { - d.parent.dirMu.Lock() + d.parent.childrenMu.Lock() delete(d.parent.children, d.name) - d.parent.dirMu.Unlock() + d.parent.childrenMu.Unlock() } d.destroyLocked(ctx) // +checklocksforce: see above. return @@ -1650,17 +1664,21 @@ func (d *dentry) evictLocked(ctx context.Context) { return } if d.parent != nil { - d.parent.dirMu.Lock() + d.parent.opMu.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) + + d.parent.childrenMu.Lock() delete(d.parent.children, d.name) + d.parent.childrenMu.Unlock() + // We're only deleting the dentry, not the file it // represents, so we don't need to update // victim parent.dirents etc. } - d.parent.dirMu.Unlock() + d.parent.opMu.Unlock() } // Safe to unlock cachingMu now that d.vfsd.IsDead(). Henceforth any // concurrent caching attempts on d will attempt to destroy it and so will @@ -1670,33 +1688,14 @@ func (d *dentry) evictLocked(ctx context.Context) { d.destroyLocked(ctx) // +checklocksforce: owned as precondition. } -// destroyLocked destroys the dentry. -// -// Preconditions: -// - d.fs.renameMu must be locked for writing; it may be temporarily unlocked. -// - d.refs == 0. -// - d.parent.children[d.name] != d, i.e. d is not reachable by path traversal -// from its former parent dentry. -// -// +checklocks:d.fs.renameMu -func (d *dentry) destroyLocked(ctx context.Context) { - switch d.refs.Load() { - case 0: - // Mark the dentry destroyed. - d.refs.Store(-1) - case -1: - panic("dentry.destroyLocked() called on already destroyed dentry") - default: - panic("dentry.destroyLocked() called with references on the dentry") - } - - // Allow the following to proceed without renameMu locked to improve - // scalability. - d.fs.renameMu.Unlock() - +// destroyDisconnected destroys an uncached, unparented dentry. There are no +// locking preconditions. +func (d *dentry) destroyDisconnected(ctx context.Context) { mf := d.fs.mfp.MemoryFile() + d.handleMu.Lock() d.dataMu.Lock() + if d.isWriteHandleOk() { // Write dirty pages back to the remote filesystem. h := d.writeHandle() @@ -1711,7 +1710,10 @@ func (d *dentry) destroyLocked(ctx context.Context) { d.dirty.RemoveAll() } d.dataMu.Unlock() + + // Close any resources held by the implementation. d.destroyImpl(ctx) + // Can use RacyLoad() because handleMu is locked. if d.readFD.RacyLoad() >= 0 { _ = unix.Close(int(d.readFD.RacyLoad())) @@ -1740,6 +1742,38 @@ func (d *dentry) destroyLocked(ctx context.Context) { d.fs.syncMu.Unlock() } + // Drop references and stop tracking this child. + d.refs.Store(-1) + refs.Unregister(d) +} + +// destroyLocked destroys the dentry. +// +// Preconditions: +// - d.fs.renameMu must be locked for writing; it may be temporarily unlocked. +// - d.refs == 0. +// - d.parent.children[d.name] != d, i.e. d is not reachable by path traversal +// from its former parent dentry. +// +// +checklocks:d.fs.renameMu +func (d *dentry) destroyLocked(ctx context.Context) { + switch d.refs.Load() { + case 0: + // Mark the dentry destroyed. + d.refs.Store(-1) + case -1: + panic("dentry.destroyLocked() called on already destroyed dentry") + default: + panic("dentry.destroyLocked() called with references on the dentry") + } + + // Allow the following to proceed without renameMu locked to improve + // scalability. + d.fs.renameMu.Unlock() + + // No locks need to be held during destoryDisconnected. + d.destroyDisconnected(ctx) + d.fs.renameMu.Lock() // Drop the reference held by d on its parent without recursively locking @@ -1747,7 +1781,6 @@ func (d *dentry) destroyLocked(ctx context.Context) { if d.parent != nil && d.parent.decRefNoCaching() == 0 { d.parent.checkCachingLocked(ctx, true /* renameMuWriteLocked */) } - refs.Unregister(d) } func (d *dentry) isDeleted() bool { diff --git a/pkg/sentry/fsimpl/gofer/gofer_test.go b/pkg/sentry/fsimpl/gofer/gofer_test.go index 0c4b5b05c..90e0319e3 100644 --- a/pkg/sentry/fsimpl/gofer/gofer_test.go +++ b/pkg/sentry/fsimpl/gofer/gofer_test.go @@ -44,7 +44,7 @@ func TestDestroyIdempotent(t *testing.T) { } parent, err := fs.newLisafsDentry(ctx, &parentInode) if err != nil { - t.Fatalf("fs.newDentry(): %v", err) + t.Fatalf("fs.newLisafsDentry(): %v", err) } childInode := lisafs.Inode{ @@ -57,9 +57,13 @@ func TestDestroyIdempotent(t *testing.T) { } child, err := fs.newLisafsDentry(ctx, &childInode) if err != nil { - t.Fatalf("fs.newDentry(): %v", err) + t.Fatalf("fs.newLisafsDentry(): %v", err) } + parent.opMu.Lock() + parent.childrenMu.Lock() parent.cacheNewChildLocked(child, "child") + parent.childrenMu.Unlock() + parent.opMu.Unlock() fs.renameMu.Lock() defer fs.renameMu.Unlock() diff --git a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go index 4899cb39a..c7d0e88c8 100644 --- a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go @@ -239,7 +239,7 @@ func (d *lisafsDentry) getRemoteChild(ctx context.Context, name string) (*dentry // Preconditions: // - fs.renameMu must be locked. -// - parent.dirMu must be locked. +// - parent.opMu must be locked. // - parent.isDir(). // - !rp.Done(). // - dentry at name must not already exist in dentry tree. @@ -262,6 +262,8 @@ func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp * return nil, err } if len(inodes) == 0 { + d.childrenMu.Lock() + defer d.childrenMu.Unlock() d.cacheNegativeLookupLocked(names[0]) return nil, linuxerr.ENOENT } @@ -269,14 +271,16 @@ func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp * // Add the walked inodes into the dentry tree. startParent := &d.dentry curParent := startParent - curParentDirMuLock := func() { + curParentLock := func() { if curParent != startParent { - curParent.dirMu.Lock() + curParent.opMu.RLock() } + curParent.childrenMu.Lock() } - curParentDirMuUnlock := func() { + curParentUnlock := func() { + curParent.childrenMu.Unlock() if curParent != startParent { - curParent.dirMu.Unlock() // +checklocksforce: locked via curParentDirMuLock(). + curParent.opMu.RUnlock() // +checklocksforce: locked via curParentLock(). } } var ret *dentry @@ -287,14 +291,26 @@ func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp * continue } - child, err := d.fs.newLisafsDentry(ctx, &inodes[i]) - if err != nil { - dentryCreationErr = err - continue + curParentLock() + // Did we race with another walk + cache operation? + child, ok := curParent.children[names[i]] // +checklocksforce: locked via curParentLock() + if ok && child != nil { + // We raced. Clean up the new inode and proceed with + // the cached child. + d.fs.client.CloseFD(ctx, inodes[i].ControlFD, false /* flush */) + } else { + // Create and cache the new dentry. + var err error + child, err = d.fs.newLisafsDentry(ctx, &inodes[i]) + if err != nil { + dentryCreationErr = err + curParentUnlock() + continue + } + curParent.cacheNewChildLocked(child, names[i]) // +checklocksforce: locked via curParentLock(). } - curParentDirMuLock() - curParent.cacheNewChildLocked(child, names[i]) - curParentDirMuUnlock() + curParentUnlock() + // For now, child has 0 references, so our caller should call // child.checkCachingLocked(). curParent gained a ref so we should also // call curParent.checkCachingLocked() so it can be removed from the cache @@ -311,9 +327,9 @@ func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp * } if status == lisafs.WalkComponentDoesNotExist && curParent.isDir() { - curParentDirMuLock() - curParent.cacheNegativeLookupLocked(names[len(inodes)]) - curParentDirMuUnlock() + curParentLock() + curParent.cacheNegativeLookupLocked(names[len(inodes)]) // +checklocksforce: locked via curParentLock(). + curParentUnlock() } return ret, dentryCreationErr } @@ -404,6 +420,8 @@ func (d *lisafsDentry) openCreate(ctx context.Context, name string, flags uint32 return child, h, nil } +// Preconditions: +// - getDirents may not be called concurrently with another getDirents call. func (d *lisafsDentry) getDirentsLocked(ctx context.Context, count int, recordDirent func(name string, key inoKey, dType uint8)) error { // shouldSeek0 indicates whether the server should SEEK to 0 before reading // directory entries. diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 82cb62a6b..817fbe79e 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -89,9 +89,9 @@ func (fs *filesystem) revalidateOne(ctx context.Context, vfsObj *vfs.VirtualFile return nil } - parent.dirMu.Lock() + parent.childrenMu.Lock() child, ok := parent.children[name] - parent.dirMu.Unlock() + parent.childrenMu.Unlock() if !ok { return nil } @@ -201,9 +201,9 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp *vfs.ResolvingPath, return d.parent, errPartialRevalidation{} default: - d.dirMu.Lock() + d.childrenMu.Lock() child, ok := d.children[name] - d.dirMu.Unlock() + d.childrenMu.Unlock() if !ok { // child is not cached, no need to validate any further. return nil, errRevalidationStepDone{} @@ -308,8 +308,9 @@ func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualF *ds = appendDentry(*ds, d) name := state.names[i] - d.parent.dirMu.Lock() + d.parent.opMu.RLock() + d.parent.childrenMu.Lock() if d.isSynthetic() { // Normally we don't mark invalidated dentries as deleted since // they may still exist (but at a different path), and also for @@ -324,14 +325,15 @@ func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualF d.parent.clearDirentsLocked() } - // Since the dirMu was released and reacquired, re-check that the + // Since the opMu was released and reacquired, re-check that the // parent's child with this name is still the same. Do not touch it if // it has been replaced with a different one. if child := d.parent.children[name]; child == d { // Invalidate dentry so it gets reloaded next time it's accessed. delete(d.parent.children, name) } - d.parent.dirMu.Unlock() + d.parent.childrenMu.Unlock() + d.parent.opMu.RUnlock() return nil } diff --git a/pkg/sentry/fsimpl/gofer/save_restore.go b/pkg/sentry/fsimpl/gofer/save_restore.go index cd2e62797..f31dcd6c3 100644 --- a/pkg/sentry/fsimpl/gofer/save_restore.go +++ b/pkg/sentry/fsimpl/gofer/save_restore.go @@ -117,8 +117,8 @@ func (d *dentry) prepareSaveRecursive(ctx context.Context) error { write: d.isWriteHandleOk(), } } - d.dirMu.Lock() - defer d.dirMu.Unlock() + d.childrenMu.Lock() + defer d.childrenMu.Unlock() for _, child := range d.children { if child != nil { if err := child.prepareSaveRecursive(ctx); err != nil { @@ -220,6 +220,8 @@ func (fs *filesystem) CompleteRestore(ctx context.Context, opts vfs.CompleteRest // Preconditions: d is not synthetic. func (d *dentry) restoreDescendantsRecursive(ctx context.Context, opts *vfs.CompleteRestoreOptions) error { + d.childrenMu.Lock() + defer d.childrenMu.Unlock() for _, child := range d.children { if child == nil { continue