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

@@ -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:

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 {

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 {

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 {

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()

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.

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
 		}

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