From 567ce6c89d85242845d1e3d50396759b442db2a2 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Wed, 9 Mar 2022 16:31:17 -0800
Subject: [PATCH] Skip dirents that fail stat(2) during Getdents RPC.

A file might disappear in between the call to unix.Getdents() and unix.Stat().
We should simply skip these files while returning the results for Getdents RPC
instead of failing the entire RPC. This is consistent with what 9P fsgofer.

This was causing <0.1% flake rate for lisafs syscall tests on BuildKite.

PiperOrigin-RevId: 433606841
---
 pkg/lisafs/node.go       | 4 +++-
 runsc/boot/vfs.go        | 8 +++++---
 runsc/fsgofer/fsgofer.go | 2 +-
 runsc/fsgofer/lisafs.go  | 8 ++------
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/pkg/lisafs/node.go b/pkg/lisafs/node.go
index 662bd8fff..053237be2 100644
--- a/pkg/lisafs/node.go
+++ b/pkg/lisafs/node.go
@@ -169,7 +169,9 @@ func (n *Node) WithChildrenMu(fn func()) {
 // operation. The returned path should be free of any intermediate symlinks
 // because all internal (non-leaf) nodes are directories.
 //
-// Precondition: server's rename mutex must be at least read locked.
+// Precondition:
+// * server's rename mutex must be at least read locked. Calling handlers must
+//   at least have read concurrency guarantee from the server.
 func (n *Node) FilePath() string {
 	// Walk upwards and prepend name to res.
 	var res fspath.Builder
diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go
index aaf7e5672..0029f54e1 100644
--- a/runsc/boot/vfs.go
+++ b/runsc/boot/vfs.go
@@ -685,7 +685,7 @@ func (c *containerMounter) mountTmpVFS2(ctx context.Context, conf *config.Config
 			log.Infof(`Skipping internal tmpfs mount for "/tmp" because it's not empty`)
 			return nil
 		default:
-			return err
+			return fmt.Errorf("fd.IterDirents failed: %v", err)
 		}
 		fallthrough
 
@@ -699,8 +699,10 @@ func (c *containerMounter) mountTmpVFS2(ctx context.Context, conf *config.Config
 			// another user. This is normally done for /tmp.
 			Options: []string{"mode=01777"},
 		}
-		_, err := c.mountSubmountVFS2(ctx, conf, mns, creds, &mountAndFD{mount: &tmpMount})
-		return err
+		if _, err := c.mountSubmountVFS2(ctx, conf, mns, creds, &mountAndFD{mount: &tmpMount}); err != nil {
+			return fmt.Errorf("mountSubmountVFS2 failed: %v", err)
+		}
+		return nil
 
 	case linuxerr.Equals(linuxerr.ENOTDIR, err):
 		// Not a dir?! Let it be.
diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go
index 1b0b1647c..1e36a66f7 100644
--- a/runsc/fsgofer/fsgofer.go
+++ b/runsc/fsgofer/fsgofer.go
@@ -1096,7 +1096,7 @@ func (l *localFile) readDirent(f int, offset uint64, count uint32, skip uint64)
 		for _, name := range names {
 			stat, err := statAt(l.file.FD(), name)
 			if err != nil {
-				log.Warningf("Readdir is skipping file with failed stat %q, err: %v", l.hostPath, err)
+				log.Warningf("Readdir is skipping file %q with failed stat, err: %v", path.Join(l.hostPath, name), err)
 				continue
 			}
 			qid := l.attachPoint.makeQID(&stat)
diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go
index b0a7fa3de..e0d57d53d 100644
--- a/runsc/fsgofer/lisafs.go
+++ b/runsc/fsgofer/lisafs.go
@@ -809,7 +809,6 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa
 		}
 		bytesRead += n
 
-		var statErr error
 		parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string) bool {
 			dirent := lisafs.Dirent64{
 				Ino:  primitive.Uint64(ino),
@@ -822,17 +821,14 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa
 			// additional syscall per dirent. Live with it.
 			stat, err := statAt(fd.hostFD, name)
 			if err != nil {
-				statErr = err
-				return false
+				log.Warningf("Getdent64: skipping file %q with failed stat, err: %v", path.Join(fd.ControlFD().FD().Node().FilePath(), name), err)
+				return true
 			}
 			dirent.DevMinor = primitive.Uint32(unix.Minor(stat.Dev))
 			dirent.DevMajor = primitive.Uint32(unix.Major(stat.Dev))
 			recordDirent(dirent)
 			return true
 		})
-		if statErr != nil {
-			return statErr
-		}
 	}
 	return nil
 }