Delegate revalidation work in gofer client to dentry.impl.

Revalidation was the only outlier which was not delegating work to dentry.impl
via type switching in dentry_impl.go. It had its own type switches. This made
the code look quite obnoxious when other dentry implementations were added.

Abstract away sufficient details so dentry implementations can perform
revalidation on their own. The invalidation code is shared as necessary to
avoid code duplication.

There are 3 notable changes in behavior:
1. Fixed a bug where revalidateState.start could be a synthetic dentry.
   The old implementation would panic in this case. Now that case is handled.
2. revalidateState.names is not pre-populated anymore. Dentry implementations
   have to populate it as necessary. This is helpful because only lisafs impl
   requires such a list of names (for making WalkStat RPC). Other impls
   will save this slice allocation.
3. revalidateState.dentries is no longer used as a FIFO. A slice is not an
   optimal way to back a FIFO data structure. popFront() was advancing the
   slice head while leaving behind allocated storage. When append() would move
   past the end of the underlying storage, a new storage would be allocated,
   dentry slice would be copied and the old storage would be GC'd.
   revalidateState.dentries was not being "reused" as it was intended to be.
   Getting rid of popFront() operation fixes this issue.

PiperOrigin-RevId: 506067998

pkg/sentry/fsimpl/gofer/dentry_impl.go

@@ -426,3 +426,23 @@ func (d *dentry) restoreFile(ctx context.Context, opts *vfs.CompleteRestoreOptio
 		panic("unknown dentry implementation")
 	}
 }
+
+// doRevalidation calls into r.start's dentry implementation to perform
+// revalidation on all the dentries contained in r.
+//
+// Preconditions:
+//   - fs.renameMu must be locked.
+//   - InteropModeShared is in effect.
+func (r *revalidateState) doRevalidation(ctx context.Context, vfsObj *vfs.VirtualFilesystem, ds **[]*dentry) error {
+	// Skip synthetic dentries because there is no actual implementation that can
+	// be used to walk the remote filesystem. A start dentry cannot be replaced.
+	if r.start.isSynthetic() {
+		return nil
+	}
+	switch r.start.impl.(type) {
+	case *lisafsDentry:
+		return doRevalidationLisafs(ctx, vfsObj, r, ds)
+	default:
+		panic("unknown dentry implementation")
+	}
+}

pkg/sentry/fsimpl/gofer/lisafs_dentry.go

@@ -526,3 +526,84 @@ func (d *lisafsDentry) restoreFile(ctx context.Context, inode *lisafs.Inode, opt
 
 	return nil
 }
+
+// doRevalidationLisafs stats all dentries in `state`. It will update or
+// invalidate dentries in the cache based on the result.
+//
+// Preconditions:
+//   - fs.renameMu must be locked.
+//   - InteropModeShared is in effect.
+func doRevalidationLisafs(ctx context.Context, vfsObj *vfs.VirtualFilesystem, state *revalidateState, ds **[]*dentry) error {
+	start := state.start.impl.(*lisafsDentry)
+
+	// Populate state.names.
+	state.names = state.names[:0] // For sanity.
+	if state.refreshStart {
+		state.names = append(state.names, "")
+	}
+	for _, d := range state.dentries {
+		state.names = append(state.names, d.name)
+	}
+
+	// Lock metadata on all dentries *before* getting attributes for them.
+	if state.refreshStart {
+		start.metadataMu.Lock()
+		defer start.metadataMu.Unlock()
+	}
+	for _, d := range state.dentries {
+		d.metadataMu.Lock()
+	}
+	// lastUnlockedDentry keeps track of the dentries in state.dentries that have
+	// already had their metadataMu unlocked. Avoid defer unlock in the loop
+	// above to avoid heap allocation.
+	lastUnlockedDentry := -1
+	defer func() {
+		// Advance to the first unevaluated dentry and unlock the remaining
+		// dentries.
+		for lastUnlockedDentry++; lastUnlockedDentry < len(state.dentries); lastUnlockedDentry++ {
+			state.dentries[lastUnlockedDentry].metadataMu.Unlock()
+		}
+	}()
+
+	// Make WalkStat RPC.
+	stats, err := start.controlFD.WalkStat(ctx, state.names)
+	if err != nil {
+		return err
+	}
+
+	if state.refreshStart {
+		if len(stats) > 0 {
+			// First dentry is where the search is starting, just update attributes
+			// since it cannot be replaced.
+			start.updateMetadataFromStatxLocked(&stats[0]) // +checklocksforce: see above.
+			stats = stats[1:]
+		}
+	}
+
+	for i := 0; i < len(state.dentries); i++ {
+		d := state.dentries[i]
+		found := i < len(stats)
+		// Advance lastUnlockedDentry. It is the responsibility of this for loop
+		// block to unlock d.metadataMu.
+		lastUnlockedDentry = i
+
+		// Note that synthetic dentries will always fail this comparison check.
+		if !found || d.inoKey != inoKeyFromStatx(&stats[i]) {
+			d.metadataMu.Unlock()
+			if !found && d.isSynthetic() {
+				// We have a synthetic file, and no remote file has arisen to replace
+				// it.
+				return nil
+			}
+			// The file at this path has changed or no longer exists. Mark the
+			// dentry invalidated.
+			d.invalidate(ctx, vfsObj, ds)
+			return nil
+		}
+
+		// The file at this path hasn't changed. Just update cached metadata.
+		d.impl.(*lisafsDentry).updateMetadataFromStatxLocked(&stats[i]) // +checklocksforce: see above.
+		d.metadataMu.Unlock()
+	}
+	return nil
+}

pkg/sentry/fsimpl/gofer/revalidate.go

@@ -15,7 +15,6 @@
 package gofer
 
 import (
-	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 	"gvisor.dev/gvisor/pkg/sync"
@@ -81,6 +80,7 @@ func (fs *filesystem) revalidateParentDir(ctx context.Context, rpOrig *vfs.Resol
 //
 // Preconditions:
 //   - fs.renameMu must be locked.
+//   - parent must have up to date metadata.
 func (fs *filesystem) revalidateOne(ctx context.Context, vfsObj *vfs.VirtualFilesystem, parent *dentry, name string, ds **[]*dentry) error {
 	// Skip revalidation for interop mode different than InteropModeShared or
 	// if the parent is synthetic (child must be synthetic too, but it cannot be
@@ -96,11 +96,12 @@ func (fs *filesystem) revalidateOne(ctx context.Context, vfsObj *vfs.VirtualFile
 		return nil
 	}
 
-	state := makeRevalidateState(parent)
+	state := makeRevalidateState(parent, false /* refreshStart */)
 	defer state.release()
-
-	state.add(name, child)
-	return fs.revalidateHelper(ctx, vfsObj, state, ds)
+	// Note that child can not be nil, because we don't cache negative entries
+	// when InteropModeShared is in effect.
+	state.add(child)
+	return state.doRevalidation(ctx, vfsObj, ds)
 }
 
 // revalidate revalidates path components in rp until done returns true, or
@@ -111,15 +112,9 @@ func (fs *filesystem) revalidateOne(ctx context.Context, vfsObj *vfs.VirtualFile
 //   - fs.renameMu must be locked.
 //   - InteropModeShared is in effect.
 func (fs *filesystem) revalidate(ctx context.Context, rp *vfs.ResolvingPath, start *dentry, done func() bool, ds **[]*dentry) error {
-	state := makeRevalidateState(start)
+	state := makeRevalidateState(start, true /* refreshStart */)
 	defer state.release()
 
-	// Skip synthetic dentries because the start dentry cannot be replaced in case
-	// it has been created in the remote file system.
-	if !start.isSynthetic() {
-		state.add("", start)
-	}
-
 done:
 	for cur := start; !done(); {
 		var err error
@@ -127,20 +122,13 @@ done:
 		if err != nil {
 			switch err.(type) {
 			case errPartialRevalidation:
-				if err := fs.revalidateHelper(ctx, rp.VirtualFilesystem(), state, ds); err != nil {
+				if err := state.doRevalidation(ctx, rp.VirtualFilesystem(), ds); err != nil {
 					return err
 				}
 
 				// Reset state to release any remaining locks and restart from where
 				// stepping stopped.
-				state.reset()
-				state.start = cur
-
-				// Skip synthetic dentries because the start dentry cannot be replaced in
-				// case it has been created in the remote file system.
-				if !cur.isSynthetic() {
-					state.add("", cur)
-				}
+				state.reset(cur /* start */, true /* refreshStart */)
 
 			case errRevalidationStepDone:
 				break done
@@ -150,7 +138,7 @@ done:
 			}
 		}
 	}
-	return fs.revalidateHelper(ctx, rp.VirtualFilesystem(), state, ds)
+	return state.doRevalidation(ctx, rp.VirtualFilesystem(), ds)
 }
 
 // revalidateStep walks one element of the path and updates revalidationState
@@ -209,7 +197,9 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp *vfs.ResolvingPath,
 			return nil, errRevalidationStepDone{}
 		}
 
-		state.add(name, child)
+		// Note that child can not be nil, because we don't cache negative entries
+		// when InteropModeShared is in effect.
+		state.add(child)
 
 		// Symlink must be resolved before continuing with revalidation.
 		if child.isSymlink() {
@@ -223,132 +213,46 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp *vfs.ResolvingPath,
 	return d, nil
 }
 
-// revalidateHelper calls the gofer to stat all dentries in `state`. It will
-// update or invalidate dentries in the cache based on the result.
-//
-// Preconditions:
-//   - fs.renameMu must be locked.
-//   - InteropModeShared is in effect.
-func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualFilesystem, state *revalidateState, ds **[]*dentry) error {
-	if len(state.names) == 0 {
-		return nil
-	}
-	// Lock metadata on all dentries *before* getting attributes for them.
-	state.lockAllMetadata()
+// Precondition: fs.renameMu must be locked.
+func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, ds **[]*dentry) {
+	// If the dentry is a mountpoint, InvalidateDentry may drop the
+	// last reference on it, resulting in lock recursion. To avoid
+	// this, take a dentry reference first, then drop it while
+	// deferring the call to dentry.checkCachingLocked().
+	d.IncRef()
+	vfsObj.InvalidateDentry(ctx, &d.vfsd)
+	d.decRefNoCaching()
 
-	var stats []linux.Statx
-	switch dt := state.start.impl.(type) {
-	case *lisafsDentry:
-		var err error
-		stats, err = dt.controlFD.WalkStat(ctx, state.names)
-		if err != nil {
-			return err
-		}
-	default:
-		panic("unknown dentry implementation")
+	// Re-evaluate its caching status (i.e. if it has 0 references, drop it).
+	// The dentry will be reloaded next time it's accessed.
+	*ds = appendDentry(*ds, d)
+
+	d.parent.opMu.RLock()
+	defer d.parent.opMu.RUnlock()
+	d.parent.childrenMu.Lock()
+	defer d.parent.childrenMu.Unlock()
+
+	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
+		// consistency with Linux. However, synthetic files are guaranteed
+		// to become unreachable if their dentries are invalidated, so
+		// treat their invalidation as deletion.
+		d.setDeleted()
+		d.decRefNoCaching()
+		*ds = appendDentry(*ds, d)
+
+		d.parent.syntheticChildren--
+		d.parent.clearDirentsLocked()
 	}
 
-	i := -1
-	for d := state.popFront(); d != nil; d = state.popFront() {
-		i++
-		var found bool
-		switch state.start.impl.(type) {
-		case *lisafsDentry:
-			found = i < len(stats)
-		default:
-			panic("unknown dentry implementation")
-		}
-		if i == 0 && len(state.names[0]) == 0 {
-			if found && !d.isSynthetic() {
-				// First dentry is where the search is starting, just update attributes
-				// since it cannot be replaced.
-				switch dt := d.impl.(type) {
-				case *lisafsDentry:
-					dt.updateMetadataFromStatxLocked(&stats[i]) // +checklocksforce: acquired by lockAllMetadata.
-				default:
-					panic("unknown dentry implementation")
-				}
-			}
-			d.metadataMu.Unlock() // +checklocksforce: see above.
-			continue
-		}
-
-		var fileChanged bool
-		if found {
-			switch d.impl.(type) {
-			case *lisafsDentry:
-				fileChanged = d.inoKey != inoKeyFromStatx(&stats[i])
-			case nil:
-				// A remote file was found to replace this synthetic file.
-				fileChanged = true
-			default:
-				panic("unknown dentry implementation")
-			}
-		}
-		// Note that synthetic dentries will always fail this comparison check.
-		if !found || fileChanged {
-			d.metadataMu.Unlock() // +checklocksforce: see above.
-			if !found && d.isSynthetic() {
-				// We have a synthetic file, and no remote file has arisen to replace
-				// it.
-				return nil
-			}
-			// The file at this path has changed or no longer exists. Mark the
-			// dentry invalidated, and re-evaluate its caching status (i.e. if
-			// it has 0 references, drop it). The dentry will be reloaded next
-			// time it's accessed.
-			//
-			// If the dentry is a mountpoint, InvalidateDentry may drop the
-			// last reference on it, resulting in lock recursion. To avoid
-			// this, take a dentry reference first, then drop it while
-			// deferring the call to dentry.checkCachingLocked().
-			d.IncRef()
-			vfsObj.InvalidateDentry(ctx, &d.vfsd)
-			d.decRefNoCaching()
-			*ds = appendDentry(*ds, d)
-
-			name := state.names[i]
-			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
-				// consistency with Linux. However, synthetic files are guaranteed
-				// to become unreachable if their dentries are invalidated, so
-				// treat their invalidation as deletion.
-				d.setDeleted()
-				d.decRefNoCaching()
-				*ds = appendDentry(*ds, d)
-
-				d.parent.syntheticChildren--
-				d.parent.clearDirentsLocked()
-			}
-
-			// 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.childrenMu.Unlock()
-			d.parent.opMu.RUnlock()
-
-			return nil
-		}
-
-		// The file at this path hasn't changed. Just update cached metadata.
-		switch dt := d.impl.(type) {
-		case *lisafsDentry:
-			dt.updateMetadataFromStatxLocked(&stats[i]) // +checklocksforce: see above.
-		default:
-			panic("unknown dentry implementation")
-		}
-		d.metadataMu.Unlock()
+	// Since the opMu was just reacquired above, 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[d.name]; child == d {
+		// Invalidate dentry so it gets reloaded next time it's accessed.
+		delete(d.parent.children, d.name)
 	}
-
-	return nil
 }
 
 // revalidateStatePool caches revalidateState instances to save array
@@ -364,71 +268,51 @@ var revalidateStatePool = sync.Pool{
 // dentries. The list must be in ancestry order, in other words `n` must be
 // `n-1` child.
 type revalidateState struct {
-	// start is the dentry where to start the attributes search.
+	// start is the dentry where to start the revalidation of dentries.
 	start *dentry
 
-	// List of names of entries to refresh attributes. Names length must be the
-	// same as detries length. They are kept in separate slices because names is
-	// used to call File.MultiGetAttr().
+	// refreshStart indicates whether the attributes of the start dentry should
+	// be refreshed.
+	refreshStart bool
+
+	// names is just a slice of names which can be used while making LISAFS RPCs.
+	// This exists to avoid the cost of repeated string slice allocation to make
+	// RPCs.
 	names []string
 
-	// dentries is the list of dentries that correspond to the names above.
-	// dentry.metadataMu is acquired as each dentry is added to this list.
+	// dentries is the list of dentries that need to be revalidated. The first
+	// dentry is a child of start and each successive dentry is a child of the
+	// previous.
 	dentries []*dentry
-
-	// locked indicates if metadata lock has been acquired on dentries.
-	locked bool
 }
 
-func makeRevalidateState(start *dentry) *revalidateState {
+func makeRevalidateState(start *dentry, refreshStart bool) *revalidateState {
 	r := revalidateStatePool.Get().(*revalidateState)
 	r.start = start
+	r.refreshStart = refreshStart
 	return r
 }
 
 // release must be called after the caller is done with this object. It releases
 // all metadata locks and resources.
 func (r *revalidateState) release() {
-	r.reset()
+	r.reset(nil /* start */, false /* refreshStart */)
 	revalidateStatePool.Put(r)
 }
 
 // Preconditions:
+//   - d != nil.
 //   - d is a descendant of all dentries in r.dentries.
-func (r *revalidateState) add(name string, d *dentry) {
-	r.names = append(r.names, name)
+func (r *revalidateState) add(d *dentry) {
 	r.dentries = append(r.dentries, d)
 }
 
-// +checklocksignore
-func (r *revalidateState) lockAllMetadata() {
-	for _, d := range r.dentries {
-		d.metadataMu.Lock()
-	}
-	r.locked = true
-}
-
-func (r *revalidateState) popFront() *dentry {
-	if len(r.dentries) == 0 {
-		return nil
-	}
-	d := r.dentries[0]
-	r.dentries = r.dentries[1:]
-	return d
-}
-
 // reset releases all metadata locks and resets all fields to allow this
 // instance to be reused.
 // +checklocksignore
-func (r *revalidateState) reset() {
-	if r.locked {
-		// Unlock any remaining dentries.
-		for _, d := range r.dentries {
-			d.metadataMu.Unlock()
-		}
-		r.locked = false
-	}
-	r.start = nil
+func (r *revalidateState) reset(start *dentry, refreshStart bool) {
+	r.start = start
+	r.refreshStart = refreshStart
 	r.names = r.names[:0]
 	r.dentries = r.dentries[:0]
 }