Disable O_DIRECT by default (#2400)

* disable O_DIRECT by default

* minor additions

* minor fixes

* dummy commit

cfg/config.go

@@ -319,7 +319,7 @@ func BindFlags(v *viper.Viper, flagSet *pflag.FlagSet) error {
 		return err
 	}
 
-	flagSet.BoolP("disable-o-direct", "", false, "Whether to disable using O_DIRECT while writing to file-cache in case of parallel downloads.")
+	flagSet.BoolP("disable-o-direct", "", true, "Whether to disable using O_DIRECT while writing to file-cache in case of parallel downloads.")
 
 	if err := flagSet.MarkHidden("disable-o-direct"); err != nil {
 		return err

cmd/config_validation_test.go

@@ -61,6 +61,7 @@ func defaultFileCacheConfig(t *testing.T) cfg.FileCacheConfig {
 		MaxSizeMb:                -1,
 		ParallelDownloadsPerFile: 16,
 		WriteBufferSize:          4 * 1024 * 1024,
+		DisableODirect:           true,
 	}
 }
 

cmd/root_test.go

@@ -240,7 +240,7 @@ func TestArgsParsing_FileCacheFlags(t *testing.T) {
 	}{
 		{
 			name: "Test file cache flags.",
-			args: []string{"gcsfuse", "--cache-file-for-range-read", "--download-chunk-size-mb=20", "--enable-crc", "--enable-parallel-downloads", "--max-parallel-downloads=40", "--file-cache-max-size-mb=100", "--parallel-downloads-per-file=2", "abc", "pqr"},
+			args: []string{"gcsfuse", "--cache-file-for-range-read", "--download-chunk-size-mb=20", "--enable-crc", "--enable-parallel-downloads", "--max-parallel-downloads=40", "--file-cache-max-size-mb=100", "--parallel-downloads-per-file=2", "--disable-o-direct=false", "abc", "pqr"},
 			expectedConfig: &cfg.Config{
 				FileCache: cfg.FileCacheConfig{
 					CacheFileForRangeRead:    true,
@@ -251,6 +251,7 @@ func TestArgsParsing_FileCacheFlags(t *testing.T) {
 					MaxSizeMb:                100,
 					ParallelDownloadsPerFile: 2,
 					WriteBufferSize:          4 * 1024 * 1024,
+					DisableODirect:           false,
 				},
 			},
 		},
@@ -267,6 +268,7 @@ func TestArgsParsing_FileCacheFlags(t *testing.T) {
 					MaxSizeMb:                -1,
 					ParallelDownloadsPerFile: 16,
 					WriteBufferSize:          4 * 1024 * 1024,
+					DisableODirect:           true,
 				},
 			},
 		},

internal/config/mount_config.go

@@ -58,7 +58,7 @@ const (
 	DefaultDownloadChunkSizeMB      = 50
 	DefaultParallelDownloadsPerFile = 16
 	DefaultWriteBufferSize          = int64(4 * util.MiB)
-	DefaultDisableODirect           = false
+	DefaultDisableODirect           = true
 )
 
 type LogConfig struct {

internal/config/testdata/valid_config.yaml

@@ -18,7 +18,7 @@ file-cache:
   download-chunk-size-mb: 100
   enable-crc: false
   write-buffer-size: 8192
-  disable-o-direct: true
+  disable-o-direct: false
 metadata-cache:
   ttl-secs: 5
   type-cache-max-size-mb: 1

internal/config/yaml_parser_test.go

@@ -49,7 +49,7 @@ func validateDefaultConfig(t *testing.T, mountConfig *MountConfig) {
 	assert.Equal(t, 50, mountConfig.FileCacheConfig.DownloadChunkSizeMB)
 	assert.False(t, mountConfig.FileCacheConfig.EnableCRC)
 	assert.Equal(t, int64(4*1024*1024), mountConfig.FileCacheConfig.WriteBufferSize)
-	assert.Equal(t, false, mountConfig.FileCacheConfig.DisableODirect)
+	assert.Equal(t, true, mountConfig.FileCacheConfig.DisableODirect)
 	assert.Equal(t, 1, mountConfig.GCSConnection.GRPCConnPoolSize)
 	assert.False(t, mountConfig.GCSAuth.AnonymousAccess)
 	assert.False(t, bool(mountConfig.EnableHNS))
@@ -134,7 +134,7 @@ func (t *YamlParserTest) TestReadConfigFile_ValidConfig() {
 	assert.Equal(t.T(), 100, mountConfig.DownloadChunkSizeMB)
 	assert.False(t.T(), mountConfig.FileCacheConfig.EnableCRC)
 	assert.Equal(t.T(), int64(8192), mountConfig.FileCacheConfig.WriteBufferSize)
-	assert.Equal(t.T(), true, mountConfig.FileCacheConfig.DisableODirect)
+	assert.Equal(t.T(), false, mountConfig.FileCacheConfig.DisableODirect)
 
 	// gcs-retries
 	assert.Equal(t.T(), int64(6), mountConfig.GCSRetries.MaxRetryAttempts)

tools/config-gen/params.yaml

@@ -73,7 +73,7 @@
   flag-name: "disable-o-direct"
   type: "bool"
   usage: "Whether to disable using O_DIRECT while writing to file-cache in case of parallel downloads."
-  default: "false"
+  default: "true"
   hide-flag: true
 
 - config-path: "file-cache.download-chunk-size-mb"

tools/integration_tests/read_cache/cache_file_for_range_read_false_test.go

@@ -153,6 +153,7 @@ func TestCacheFileForRangeReadFalseTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -161,6 +162,7 @@ func TestCacheFileForRangeReadFalseTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            ramCacheDir,
 		},
 	}
@@ -182,6 +184,7 @@ func TestCacheFileForRangeReadFalseTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -190,6 +193,25 @@ func TestCacheFileForRangeReadFalseTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
+			cacheDirPath:            ramCacheDir,
+		},
+		{
+			cliFlags:                nil,
+			cacheSize:               cacheCapacityForRangeReadTestInMiB,
+			cacheFileForRangeRead:   false,
+			fileName:                configFileNameForParallelDownloadTests,
+			enableParallelDownloads: true,
+			disableODirect:          false,
+			cacheDirPath:            getDefaultCacheDirPathForTests(),
+		},
+		{
+			cliFlags:                nil,
+			cacheSize:               cacheCapacityForRangeReadTestInMiB,
+			cacheFileForRangeRead:   false,
+			fileName:                configFileNameForParallelDownloadTests,
+			enableParallelDownloads: true,
+			disableODirect:          false,
 			cacheDirPath:            ramCacheDir,
 		},
 	}

tools/integration_tests/read_cache/cache_file_for_range_read_true_test.go

@@ -105,6 +105,7 @@ func TestCacheFileForRangeReadTrueTest(t *testing.T) {
 			cacheFileForRangeRead:   true,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -113,6 +114,7 @@ func TestCacheFileForRangeReadTrueTest(t *testing.T) {
 			cacheFileForRangeRead:   true,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -121,6 +123,25 @@ func TestCacheFileForRangeReadTrueTest(t *testing.T) {
 			cacheFileForRangeRead:   true,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
+			cacheDirPath:            ramCacheDir,
+		},
+		{
+			cliFlags:                nil,
+			cacheSize:               cacheCapacityForRangeReadTestInMiB,
+			cacheFileForRangeRead:   true,
+			fileName:                configFileNameForParallelDownloadTests,
+			enableParallelDownloads: true,
+			disableODirect:          false,
+			cacheDirPath:            getDefaultCacheDirPathForTests(),
+		},
+		{
+			cliFlags:                nil,
+			cacheSize:               cacheCapacityForRangeReadTestInMiB,
+			cacheFileForRangeRead:   true,
+			fileName:                configFileNameForParallelDownloadTests,
+			enableParallelDownloads: true,
+			disableODirect:          false,
 			cacheDirPath:            ramCacheDir,
 		},
 		{
@@ -129,6 +150,7 @@ func TestCacheFileForRangeReadTrueTest(t *testing.T) {
 			cacheFileForRangeRead:   true,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            ramCacheDir,
 		},
 	}

tools/integration_tests/read_cache/disabled_cache_ttl_test.go

@@ -101,6 +101,7 @@ func TestDisabledCacheTTLTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -109,6 +110,7 @@ func TestDisabledCacheTTLTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}

tools/integration_tests/read_cache/job_chunk_test.go

@@ -178,7 +178,7 @@ func TestJobChunkTest(t *testing.T) {
 
 	// Tests to validate chunk size when read cache parallel downloads are disabled.
 	var chunkSizeForReadCache int64 = 8
-	ts.flags = []string{"--config-file=" + createConfigFile(&gcsfuseTestFlags{cacheSize: cacheSizeMB, cacheFileForRangeRead: true, fileName: configFileName, enableParallelDownloads: false, cacheDirPath: getDefaultCacheDirPathForTests()})}
+	ts.flags = []string{"--config-file=" + createConfigFile(&gcsfuseTestFlags{cacheSize: cacheSizeMB, cacheFileForRangeRead: true, fileName: configFileName, enableParallelDownloads: false, disableODirect: true, cacheDirPath: getDefaultCacheDirPathForTests()})}
 	ts.chunkSize = chunkSizeForReadCache * util.MiB
 	log.Printf("Running tests with flags: %s", ts.flags)
 	test_setup.RunTests(t, ts)

tools/integration_tests/read_cache/local_modification_test.go

@@ -105,6 +105,7 @@ func TestLocalModificationTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -113,6 +114,7 @@ func TestLocalModificationTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}

tools/integration_tests/read_cache/range_read_test.go

@@ -113,6 +113,7 @@ func TestRangeReadTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName + "1",
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}
@@ -134,6 +135,7 @@ func TestRangeReadTest(t *testing.T) {
 			cacheFileForRangeRead:   true,
 			fileName:                configFileName + "2",
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}

tools/integration_tests/read_cache/remount_test.go

@@ -138,6 +138,7 @@ func TestRemountTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -146,6 +147,7 @@ func TestRemountTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}

tools/integration_tests/read_cache/setup_test.go

@@ -86,6 +86,7 @@ type gcsfuseTestFlags struct {
 	cacheFileForRangeRead   bool
 	fileName                string
 	enableParallelDownloads bool
+	disableODirect          bool
 	cacheDirPath            string
 }
 
@@ -124,6 +125,7 @@ func createConfigFile(flags *gcsfuseTestFlags) string {
 			"max-parallel-downloads":      maxParallelDownloads,
 			"download-chunk-size-mb":      downloadChunkSizeMB,
 			"enable-crc":                  enableCrcCheck,
+			"disable-o-direct":            flags.disableODirect,
 		},
 		"cache-dir": cacheDirPath,
 	}

tools/integration_tests/read_cache/small_cache_ttl_test.go

@@ -127,6 +127,7 @@ func TestSmallCacheTTLTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileName,
 			enableParallelDownloads: false,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 		{
@@ -135,6 +136,7 @@ func TestSmallCacheTTLTest(t *testing.T) {
 			cacheFileForRangeRead:   false,
 			fileName:                configFileNameForParallelDownloadTests,
 			enableParallelDownloads: true,
+			disableODirect:          true,
 			cacheDirPath:            getDefaultCacheDirPathForTests(),
 		},
 	}

tools/integration_tests/run_tests_mounted_directory.sh

@@ -329,9 +329,15 @@ function read_cache_test_setup() {
     local enable_range_read_cache=$2
     local cache_ttl=$3
     local enable_parallel_downloads=$4
+    local disable_o_direct=$5
+    if [ -n "$disable_o_direct" ]; then
+      disable_o_direct=true
+    else
+      disable_o_direct=false
+    fi
 
     cleanup_test_environment
-    generate_config_file "$cache_size_mb" "$enable_range_read_cache" "$cache_ttl" "$enable_parallel_downloads"
+    generate_config_file "$cache_size_mb" "$enable_range_read_cache" "$cache_ttl" "$enable_parallel_downloads" "$disable_o_direct"
 }
 
 function cleanup_test_environment() {
@@ -392,6 +398,9 @@ done
 # 2. With enabled parallel downloads.
 read_cache_test_setup 50 false 3600 true
 run_read_cache_test "${test_cases[0]}"
+# 3. With enabled parallel downloads and disabled O_DIRECT
+read_cache_test_setup 50 false 3600 true false
+run_read_cache_test "${test_cases[0]}"
 
 # Read-cache test with cache-file-for-range-read:true.
 test_case="TestCacheFileForRangeReadTrueTest/TestRangeReadsWithCacheHit"
@@ -401,6 +410,9 @@ run_read_cache_test "$test_case"
 # 2. With enabled parallel downloads.
 read_cache_test_setup 50 true 3600 true
 run_read_cache_test "$test_case"
+# 3. With enabled parallel downloads and disabled O_DIRECT
+read_cache_test_setup 50 true 3600 true false
+run_read_cache_test "$test_case"
 
 # Read-cache test with disabled cache ttl.
 test_case="TestDisabledCacheTTLTest/TestReadAfterObjectUpdateIsCacheMiss"