3 files changed, 127 insertions, 38 deletions
diff --git a/pkg/sentry/fs/gofer/cache_policy.go b/pkg/sentry/fs/gofer/cache_policy.go
index fa8abf51c..98f43c578 100644
--- a/pkg/sentry/fs/gofer/cache_policy.go
+++ b/pkg/sentry/fs/gofer/cache_policy.go
@@ -17,6 +17,7 @@ package gofer
 import (
 	"fmt"
 
+	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs"
 )
 
@@ -108,25 +109,68 @@ func (cp cachePolicy) writeThrough(inode *fs.Inode) bool {
 	return cp == cacheNone || cp == cacheAllWritethrough
 }
 
-// revalidateDirent indicates that a dirent should be revalidated after a
-// lookup, because the looked up version may be stale.
-func (cp cachePolicy) revalidateDirent() bool {
+// revalidate revalidates the child Inode if the cache policy allows it.
+//
+// Depending on the cache policy, revalidate will walk from the parent to the
+// child inode, and if any unstable attributes have changed, will update the
+// cached attributes on the child inode. If the walk fails, or the returned
+// inode id is different from the one being revalidated, then the entire Dirent
+// must be reloaded.
+func (cp cachePolicy) revalidate(ctx context.Context, name string, parent, child *fs.Inode) bool {
 	if cp == cacheAll || cp == cacheAllWritethrough {
 		return false
 	}
 
-	// TODO: The cacheRemoteRevalidating policy should only
-	// return true if the remote file's attributes have changed.
-	return true
+	if cp == cacheNone {
+		return true
+	}
+
+	childIops, ok := child.InodeOperations.(*inodeOperations)
+	if !ok {
+		panic(fmt.Sprintf("revalidating inode operations of unknown type %T", child.InodeOperations))
+	}
+	parentIops, ok := parent.InodeOperations.(*inodeOperations)
+	if !ok {
+		panic(fmt.Sprintf("revalidating inode operations with parent of unknown type %T", parent.InodeOperations))
+	}
+
+	// Walk from parent to child again.
+	//
+	// TODO: If we have a directory FD in the parent
+	// inodeOperations, then we can use fstatat(2) to get the inode
+	// attributes instead of making this RPC.
+	qids, _, mask, attr, err := parentIops.fileState.file.walkGetAttr(ctx, []string{name})
+	if err != nil {
+		// Can't look up the name. Trigger reload.
+		return true
+	}
+
+	// If the Path has changed, then we are not looking at the file file.
+	// We must reload.
+	if qids[0].Path != childIops.fileState.key.Inode {
+		return true
+	}
+
+	// If we are not caching unstable attrs, then there is nothing to
+	// update on this inode.
+	if !cp.cacheUAttrs(child) {
+		return false
+	}
+
+	// Update the inode's cached unstable attrs.
+	s := childIops.session()
+	childIops.cachingInodeOps.UpdateUnstable(unstable(ctx, mask, attr, s.mounter, s.client))
+
+	return false
 }
 
-// keepDirent indicates that dirents should be kept pinned in the dirent tree
-// even if there are no application references on the file.
-func (cp cachePolicy) keepDirent(inode *fs.Inode) bool {
+// keep indicates that dirents should be kept pinned in the dirent tree even if
+// there are no application references on the file.
+func (cp cachePolicy) keep(d *fs.Dirent) bool {
 	if cp == cacheNone {
 		return false
 	}
-	sattr := inode.StableAttr
+	sattr := d.Inode.StableAttr
 	// NOTE: Only cache files, directories, and symlinks.
 	return fs.IsFile(sattr) || fs.IsDir(sattr) || fs.IsSymlink(sattr)
 }
diff --git a/pkg/sentry/fs/gofer/gofer_test.go b/pkg/sentry/fs/gofer/gofer_test.go
index 45fdaacfd..c8d7bd773 100644
--- a/pkg/sentry/fs/gofer/gofer_test.go
+++ b/pkg/sentry/fs/gofer/gofer_test.go
@@ -151,41 +151,60 @@ func TestLookup(t *testing.T) {
 
 func TestRevalidation(t *testing.T) {
 	tests := []struct {
-		cachePolicy               cachePolicy
-		preModificationWantReval  bool
-		postModificationWantReval bool
+		cachePolicy cachePolicy
+
+		// Whether dirent should be reloaded before any modifications.
+		preModificationWantReload bool
+
+		// Whether dirent should be reloaded after updating an unstable
+		// attribute on the remote fs.
+		postModificationWantReload bool
+
+		// Whether dirent unstable attributes should be updated after
+		// updating an attribute on the remote fs.
+		postModificationWantUpdatedAttrs bool
+
+		// Whether dirent should be reloaded after the remote has
+		// removed the file.
+		postRemovalWantReload bool
 	}{
 		{
 			// Policy cacheNone causes Revalidate to always return
 			// true.
-			cachePolicy:               cacheNone,
-			preModificationWantReval:  true,
-			postModificationWantReval: true,
+			cachePolicy:                      cacheNone,
+			preModificationWantReload:        true,
+			postModificationWantReload:       true,
+			postModificationWantUpdatedAttrs: true,
+			postRemovalWantReload:            true,
 		},
 		{
 			// Policy cacheAll causes Revalidate to always return
 			// false.
-			cachePolicy:               cacheAll,
-			preModificationWantReval:  false,
-			postModificationWantReval: false,
+			cachePolicy:                      cacheAll,
+			preModificationWantReload:        false,
+			postModificationWantReload:       false,
+			postModificationWantUpdatedAttrs: false,
+			postRemovalWantReload:            false,
 		},
 		{
 			// Policy cacheAllWritethrough causes Revalidate to
 			// always return false.
-			cachePolicy:               cacheAllWritethrough,
-			preModificationWantReval:  false,
-			postModificationWantReval: false,
+			cachePolicy:                      cacheAllWritethrough,
+			preModificationWantReload:        false,
+			postModificationWantReload:       false,
+			postModificationWantUpdatedAttrs: false,
+			postRemovalWantReload:            false,
 		},
 		{
 			// Policy cacheRemoteRevalidating causes Revalidate to
-			// always return true.
-			//
-			// TODO: The cacheRemoteRevalidating
-			// policy should only return true if the remote file's
-			// attributes have changed.
-			cachePolicy:               cacheRemoteRevalidating,
-			preModificationWantReval:  true,
-			postModificationWantReval: true,
+			// return update cached unstable attrs, and returns
+			// true only when the remote inode itself has been
+			// removed or replaced.
+			cachePolicy:                      cacheRemoteRevalidating,
+			preModificationWantReload:        false,
+			postModificationWantReload:       false,
+			postModificationWantUpdatedAttrs: true,
+			postRemovalWantReload:            true,
 		},
 	}
 
@@ -227,15 +246,17 @@ func TestRevalidation(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Lookup(%q) failed: %v", name, err)
 			}
-			if test.preModificationWantReval && dirent == newDirent {
+			if test.preModificationWantReload && dirent == newDirent {
 				t.Errorf("Lookup(%q) with cachePolicy=%s got old dirent %v, wanted a new dirent", name, test.cachePolicy, dirent)
 			}
-			if !test.preModificationWantReval && dirent != newDirent {
+			if !test.preModificationWantReload && dirent != newDirent {
 				t.Errorf("Lookup(%q) with cachePolicy=%s got new dirent %v, wanted old dirent %v", name, test.cachePolicy, newDirent, dirent)
 			}
 
 			// Modify the underlying mocked file's modification time.
-			file.GetAttrMock.Attr.MTimeSeconds = uint64(time.Now().Unix())
+			nowSeconds := time.Now().Unix()
+			rootFile.WalkGetAttrMock.Attr.MTimeSeconds = uint64(nowSeconds)
+			file.GetAttrMock.Attr.MTimeSeconds = uint64(nowSeconds)
 
 			// Walk again. Depending on the cache policy, we may get a new
 			// dirent.
@@ -243,12 +264,36 @@ func TestRevalidation(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Lookup(%q) failed: %v", name, err)
 			}
-			if test.postModificationWantReval && dirent == newDirent {
+			if test.postModificationWantReload && dirent == newDirent {
 				t.Errorf("Lookup(%q) with cachePolicy=%s got old dirent %v, wanted a new dirent", name, test.cachePolicy, dirent)
 			}
-			if !test.postModificationWantReval && dirent != newDirent {
+			if !test.postModificationWantReload && dirent != newDirent {
 				t.Errorf("Lookup(%q) with cachePolicy=%s got new dirent %v, wanted old dirent %v", name, test.cachePolicy, newDirent, dirent)
 			}
+			uattrs, err := newDirent.Inode.UnstableAttr(ctx)
+			if err != nil {
+				t.Fatalf("Error getting unstable attrs: %v", err)
+			}
+			gotModTimeSeconds := uattrs.ModificationTime.Seconds()
+			if test.postModificationWantUpdatedAttrs && gotModTimeSeconds != nowSeconds {
+				t.Fatalf("Lookup(%q) with cachePolicy=%s got new modification time %v, wanted %v", name, test.cachePolicy, gotModTimeSeconds, nowSeconds)
+			}
+
+			// Make WalkGetAttr return ENOENT. This simulates
+			// removing the file from the remote fs.
+			rootFile.WalkGetAttrMock = p9test.WalkGetAttrMock{
+				Err: syscall.ENOENT,
+			}
+
+			// Walk again. Depending on the cache policy, we may
+			// get ENOENT.
+			newDirent, err = rootDir.Walk(ctx, rootDir, name)
+			if test.postRemovalWantReload && err == nil {
+				t.Errorf("Lookup(%q) with cachePolicy=%s got nil error, wanted ENOENT", name, test.cachePolicy)
+			}
+			if !test.postRemovalWantReload && (err != nil || dirent != newDirent) {
+				t.Errorf("Lookup(%q) with cachePolicy=%s got new dirent %v and error %v, wanted old dirent %v and nil error", name, test.cachePolicy, newDirent, err, dirent)
+			}
 		})
 	}
 }
diff --git a/pkg/sentry/fs/gofer/session.go b/pkg/sentry/fs/gofer/session.go
index eeb9087e9..49d27ee88 100644
--- a/pkg/sentry/fs/gofer/session.go
+++ b/pkg/sentry/fs/gofer/session.go
@@ -146,13 +146,13 @@ func (s *session) Destroy() {
 }
 
 // Revalidate implements MountSource.Revalidate.
-func (s *session) Revalidate(ctx context.Context, i *fs.Inode) bool {
-	return s.cachePolicy.revalidateDirent()
+func (s *session) Revalidate(ctx context.Context, name string, parent, child *fs.Inode) bool {
+	return s.cachePolicy.revalidate(ctx, name, parent, child)
 }
 
 // Keep implements MountSource.Keep.
 func (s *session) Keep(d *fs.Dirent) bool {
-	return s.cachePolicy.keepDirent(d.Inode)
+	return s.cachePolicy.keep(d)
 }
 
 // ResetInodeMappings implements fs.MountSourceOperations.ResetInodeMappings.