Address post submit comments for fs benchmarks.

Also, drop fio total reads/writes to 1GB as 10GB is
prohibitively slow.

PiperOrigin-RevId: 363714060

6 files changed, 51 insertions, 33 deletions

diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml
index aa2fd1f47..98526dcec 100644
--- a/.buildkite/pipeline.yaml
+++ b/.buildkite/pipeline.yaml
@@ -186,10 +186,10 @@ steps:
   # For fio, running with --test.benchtime=Xs scales the written/read
   # bytes to several GB. This is not a problem for root/bind/volume mounts,
   # but for tmpfs mounts, the size can grow to more memory than the machine
-  # has availabe. Fix the runs to 10GB written/read for the benchmark.
+  # has availabe. Fix the runs to 1GB written/read for the benchmark.
   - <<: *benchmarks
     label: ":floppy_disk: FIO benchmarks"
-    command: make benchmark-platforms BENCHMARKS_SUITE=fio BENCHMARKS_TARGETS=test/benchmarks/fs:fio_test BENCHMARKS_OPTIONS=--test.benchtime=10000x
+    command: make benchmark-platforms BENCHMARKS_SUITE=fio BENCHMARKS_TARGETS=test/benchmarks/fs:fio_test BENCHMARKS_OPTIONS=--test.benchtime=1000x
   - <<: *benchmarks
     label: ":globe_with_meridians: HTTPD benchmarks"
     command: make benchmark-platforms BENCHMARKS_FILTER="Continuous" BENCHMARKS_SUITE=httpd BENCHMARKS_TARGETS=test/benchmarks/network:httpd_test
diff --git a/test/benchmarks/fs/BUILD b/test/benchmarks/fs/BUILD
index c94caab60..dc82e63b2 100644
--- a/test/benchmarks/fs/BUILD
+++ b/test/benchmarks/fs/BUILD
@@ -8,6 +8,7 @@ benchmark_test(
     srcs = ["bazel_test.go"],
     visibility = ["//:sandbox"],
     deps = [
+        "//pkg/cleanup",
         "//pkg/test/dockerutil",
         "//test/benchmarks/harness",
         "//test/benchmarks/tools",
@@ -21,6 +22,7 @@ benchmark_test(
     srcs = ["fio_test.go"],
     visibility = ["//:sandbox"],
     deps = [
+        "//pkg/cleanup",
         "//pkg/test/dockerutil",
         "//test/benchmarks/harness",
         "//test/benchmarks/tools",
diff --git a/test/benchmarks/fs/bazel_test.go b/test/benchmarks/fs/bazel_test.go
index 7ced963f6..797b1952d 100644
--- a/test/benchmarks/fs/bazel_test.go
+++ b/test/benchmarks/fs/bazel_test.go
@@ -20,6 +20,7 @@ import (
 	"strings"
 	"testing"
 
+	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/test/dockerutil"
 	"gvisor.dev/gvisor/test/benchmarks/harness"
 	"gvisor.dev/gvisor/test/benchmarks/tools"
@@ -28,8 +29,8 @@ import (
 // Dimensions here are clean/dirty cache (do or don't drop caches)
 // and if the mount on which we are compiling is a tmpfs/bind mount.
 type benchmark struct {
-	clearCache bool   // clearCache drops caches before running.
-	fstype     string // type of filesystem to use.
+	clearCache bool                   // clearCache drops caches before running.
+	fstype     harness.FileSystemType // type of filesystem to use.
 }
 
 // Note: CleanCache versions of this test require running with root permissions.
@@ -48,12 +49,12 @@ func runBuildBenchmark(b *testing.B, image, workdir, target string) {
 	// Get a machine from the Harness on which to run.
 	machine, err := harness.GetMachine()
 	if err != nil {
-		b.Fatalf("failed to get machine: %v", err)
+		b.Fatalf("Failed to get machine: %v", err)
 	}
 	defer machine.CleanUp()
 
 	benchmarks := make([]benchmark, 0, 6)
-	for _, filesys := range []string{harness.BindFS, harness.TmpFS, harness.RootFS} {
+	for _, filesys := range []harness.FileSystemType{harness.BindFS, harness.TmpFS, harness.RootFS} {
 		benchmarks = append(benchmarks, benchmark{
 			clearCache: true,
 			fstype:     filesys,
@@ -75,7 +76,7 @@ func runBuildBenchmark(b *testing.B, image, workdir, target string) {
 
 		filesystem := tools.Parameter{
 			Name:  "filesystem",
-			Value: bm.fstype,
+			Value: string(bm.fstype),
 		}
 		name, err := tools.ParametersToName(pageCache, filesystem)
 		if err != nil {
@@ -86,13 +87,14 @@ func runBuildBenchmark(b *testing.B, image, workdir, target string) {
 			// Grab a container.
 			ctx := context.Background()
 			container := machine.GetContainer(ctx, b)
-			defer container.CleanUp(ctx)
-
-			mts, prefix, cleanup, err := harness.MakeMount(machine, bm.fstype)
+			cu := cleanup.Make(func() {
+				container.CleanUp(ctx)
+			})
+			defer cu.Clean()
+			mts, prefix, err := harness.MakeMount(machine, bm.fstype, &cu)
 			if err != nil {
 				b.Fatalf("Failed to make mount: %v", err)
 			}
-			defer cleanup()
 
 			runOpts := dockerutil.RunOpts{
 				Image:  image,
@@ -104,8 +106,9 @@ func runBuildBenchmark(b *testing.B, image, workdir, target string) {
 				b.Fatalf("run failed with: %v", err)
 			}
 
+			cpCmd := fmt.Sprintf("mkdir -p %s && cp -r %s %s/.", prefix, workdir, prefix)
 			if out, err := container.Exec(ctx, dockerutil.ExecOpts{},
-				"cp", "-rf", workdir, prefix+"/."); err != nil {
+				"/bin/sh", "-c", cpCmd); err != nil {
 				b.Fatalf("failed to copy directory: %v (%s)", err, out)
 			}
 
diff --git a/test/benchmarks/fs/fio_test.go b/test/benchmarks/fs/fio_test.go
index f783a2b33..1482466f4 100644
--- a/test/benchmarks/fs/fio_test.go
+++ b/test/benchmarks/fs/fio_test.go
@@ -21,6 +21,7 @@ import (
 	"strings"
 	"testing"
 
+	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/test/dockerutil"
 	"gvisor.dev/gvisor/test/benchmarks/harness"
 	"gvisor.dev/gvisor/test/benchmarks/tools"
@@ -69,7 +70,7 @@ func BenchmarkFio(b *testing.B) {
 	}
 	defer machine.CleanUp()
 
-	for _, fsType := range []string{harness.BindFS, harness.TmpFS, harness.RootFS} {
+	for _, fsType := range []harness.FileSystemType{harness.BindFS, harness.TmpFS, harness.RootFS} {
 		for _, tc := range testCases {
 			operation := tools.Parameter{
 				Name:  "operation",
@@ -81,7 +82,7 @@ func BenchmarkFio(b *testing.B) {
 			}
 			filesystem := tools.Parameter{
 				Name:  "filesystem",
-				Value: fsType,
+				Value: string(fsType),
 			}
 			name, err := tools.ParametersToName(operation, blockSize, filesystem)
 			if err != nil {
@@ -90,15 +91,18 @@ func BenchmarkFio(b *testing.B) {
 			b.Run(name, func(b *testing.B) {
 				b.StopTimer()
 				tc.Size = b.N
+
 				ctx := context.Background()
 				container := machine.GetContainer(ctx, b)
-				defer container.CleanUp(ctx)
+				cu := cleanup.Make(func() {
+					container.CleanUp(ctx)
+				})
+				defer cu.Clean()
 
-				mnts, outdir, mountCleanup, err := harness.MakeMount(machine, fsType)
+				mnts, outdir, err := harness.MakeMount(machine, fsType, &cu)
 				if err != nil {
 					b.Fatalf("failed to make mount: %v", err)
 				}
-				defer mountCleanup()
 
 				// Start the container with the mount.
 				if err := container.Spawn(
@@ -112,6 +116,11 @@ func BenchmarkFio(b *testing.B) {
 					b.Fatalf("failed to start fio container with: %v", err)
 				}
 
+				if out, err := container.Exec(ctx, dockerutil.ExecOpts{},
+					"mkdir", "-p", outdir); err != nil {
+					b.Fatalf("failed to copy directory: %v (%s)", err, out)
+				}
+
 				// Directory and filename inside container where fio will read/write.
 				outfile := filepath.Join(outdir, "test.txt")
 
@@ -130,7 +139,6 @@ func BenchmarkFio(b *testing.B) {
 				}
 
 				cmd := tc.MakeCmd(outfile)
-
 				if err := harness.DropCaches(machine); err != nil {
 					b.Fatalf("failed to drop caches: %v", err)
 				}
diff --git a/test/benchmarks/harness/BUILD b/test/benchmarks/harness/BUILD
index 116610938..367316661 100644
--- a/test/benchmarks/harness/BUILD
+++ b/test/benchmarks/harness/BUILD
@@ -12,6 +12,7 @@ go_library(
     ],
     visibility = ["//:sandbox"],
     deps = [
+        "//pkg/cleanup",
         "//pkg/test/dockerutil",
         "//pkg/test/testutil",
         "@com_github_docker_docker//api/types/mount:go_default_library",
diff --git a/test/benchmarks/harness/util.go b/test/benchmarks/harness/util.go
index 36abe1069..f7e569751 100644
--- a/test/benchmarks/harness/util.go
+++ b/test/benchmarks/harness/util.go
@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types/mount"
+	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/test/dockerutil"
 	"gvisor.dev/gvisor/pkg/test/testutil"
 )
@@ -58,52 +59,55 @@ func DebugLog(b *testing.B, msg string, args ...interface{}) {
 	}
 }
 
+// FileSystemType represents a type container mount.
+type FileSystemType string
+
 const (
 	// BindFS indicates a bind mount should be created.
-	BindFS = "bindfs"
+	BindFS FileSystemType = "bindfs"
 	// TmpFS indicates a tmpfs mount should be created.
-	TmpFS = "tmpfs"
+	TmpFS FileSystemType = "tmpfs"
 	// RootFS indicates no mount should be created and the root mount should be used.
-	RootFS = "rootfs"
+	RootFS FileSystemType = "rootfs"
 )
 
 // MakeMount makes a mount and cleanup based on the requested type. Bind
 // and volume mounts are backed by a temp directory made with mktemp.
 // tmpfs mounts require no such backing and are just made.
 // rootfs mounts do not make a mount, but instead return a target direectory at root.
-// It is up to the caller to call the returned cleanup.
-func MakeMount(machine Machine, fsType string) ([]mount.Mount, string, func(), error) {
+// It is up to the caller to call Clean on the passed *cleanup.Cleanup
+func MakeMount(machine Machine, fsType FileSystemType, cu *cleanup.Cleanup) ([]mount.Mount, string, error) {
 	mounts := make([]mount.Mount, 0, 1)
+	target := "/data"
 	switch fsType {
 	case BindFS:
 		dir, err := machine.RunCommand("mktemp", "-d")
 		if err != nil {
-			return mounts, "", func() {}, fmt.Errorf("failed to create tempdir: %v", err)
+			return mounts, "", fmt.Errorf("failed to create tempdir: %v", err)
 		}
 		dir = strings.TrimSuffix(dir, "\n")
-
+		cu.Add(func() {
+			machine.RunCommand("rm", "-rf", dir)
+		})
 		out, err := machine.RunCommand("chmod", "777", dir)
 		if err != nil {
-			machine.RunCommand("rm", "-rf", dir)
-			return mounts, "", func() {}, fmt.Errorf("failed modify directory: %v %s", err, out)
+			return mounts, "", fmt.Errorf("failed modify directory: %v %s", err, out)
 		}
-		target := "/data"
 		mounts = append(mounts, mount.Mount{
 			Target: target,
 			Source: dir,
 			Type:   mount.TypeBind,
 		})
-		return mounts, target, func() { machine.RunCommand("rm", "-rf", dir) }, nil
+		return mounts, target, nil
 	case RootFS:
-		return mounts, "/", func() {}, nil
+		return mounts, target, nil
 	case TmpFS:
-		target := "/data"
 		mounts = append(mounts, mount.Mount{
 			Target: target,
 			Type:   mount.TypeTmpfs,
 		})
-		return mounts, target, func() {}, nil
+		return mounts, target, nil
 	default:
-		return mounts, "", func() {}, fmt.Errorf("illegal mount type not supported: %v", fsType)
+		return mounts, "", fmt.Errorf("illegal mount type not supported: %v", fsType)
 	}
 }