From 1eb339c5e8fd653245caf1a13592ef1dc2d226df Mon Sep 17 00:00:00 2001 From: Ayush Sethi Date: Tue, 27 Aug 2024 13:54:45 +0530 Subject: [PATCH] Disable O_DIRECT by default (#2400) * disable O_DIRECT by default * minor additions * minor fixes * dummy commit --- cfg/config.go | 2 +- cmd/config_validation_test.go | 1 + cmd/root_test.go | 4 +++- internal/config/mount_config.go | 2 +- internal/config/testdata/valid_config.yaml | 2 +- internal/config/yaml_parser_test.go | 4 ++-- tools/config-gen/params.yaml | 2 +- .../cache_file_for_range_read_false_test.go | 22 +++++++++++++++++++ .../cache_file_for_range_read_true_test.go | 22 +++++++++++++++++++ .../read_cache/disabled_cache_ttl_test.go | 2 ++ .../read_cache/job_chunk_test.go | 2 +- .../read_cache/local_modification_test.go | 2 ++ .../read_cache/range_read_test.go | 2 ++ .../read_cache/remount_test.go | 2 ++ .../read_cache/setup_test.go | 2 ++ .../read_cache/small_cache_ttl_test.go | 2 ++ .../run_tests_mounted_directory.sh | 14 +++++++++++- 17 files changed, 80 insertions(+), 9 deletions(-) diff --git a/cfg/config.go b/cfg/config.go index 711fc82acf..1a41799270 100644 --- a/cfg/config.go +++ b/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 diff --git a/cmd/config_validation_test.go b/cmd/config_validation_test.go index 0a0fb56deb..ea6c359290 100644 --- a/cmd/config_validation_test.go +++ b/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, } } diff --git a/cmd/root_test.go b/cmd/root_test.go index 0682b12b25..63b1feb567 100644 --- a/cmd/root_test.go +++ b/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, }, }, }, diff --git a/internal/config/mount_config.go b/internal/config/mount_config.go index 5887fbfd30..824d042e2e 100644 --- a/internal/config/mount_config.go +++ b/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 { diff --git a/internal/config/testdata/valid_config.yaml b/internal/config/testdata/valid_config.yaml index 9a160d9f4e..d54fa5c1c7 100644 --- a/internal/config/testdata/valid_config.yaml +++ b/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 diff --git a/internal/config/yaml_parser_test.go b/internal/config/yaml_parser_test.go index 4bffab3737..0aadc8ce62 100644 --- a/internal/config/yaml_parser_test.go +++ b/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) diff --git a/tools/config-gen/params.yaml b/tools/config-gen/params.yaml index 5804721726..ae1289f2c7 100644 --- a/tools/config-gen/params.yaml +++ b/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" diff --git a/tools/integration_tests/read_cache/cache_file_for_range_read_false_test.go b/tools/integration_tests/read_cache/cache_file_for_range_read_false_test.go index 61648b4d39..0056000aea 100644 --- a/tools/integration_tests/read_cache/cache_file_for_range_read_false_test.go +++ b/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, }, } diff --git a/tools/integration_tests/read_cache/cache_file_for_range_read_true_test.go b/tools/integration_tests/read_cache/cache_file_for_range_read_true_test.go index 6debeeec7c..ed0bf92670 100644 --- a/tools/integration_tests/read_cache/cache_file_for_range_read_true_test.go +++ b/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, }, } diff --git a/tools/integration_tests/read_cache/disabled_cache_ttl_test.go b/tools/integration_tests/read_cache/disabled_cache_ttl_test.go index 2c31340b5c..8ab7947a84 100644 --- a/tools/integration_tests/read_cache/disabled_cache_ttl_test.go +++ b/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(), }, } diff --git a/tools/integration_tests/read_cache/job_chunk_test.go b/tools/integration_tests/read_cache/job_chunk_test.go index 2b781c85d9..514ed5f64c 100644 --- a/tools/integration_tests/read_cache/job_chunk_test.go +++ b/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) diff --git a/tools/integration_tests/read_cache/local_modification_test.go b/tools/integration_tests/read_cache/local_modification_test.go index 645f45a124..180161ddf6 100644 --- a/tools/integration_tests/read_cache/local_modification_test.go +++ b/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(), }, } diff --git a/tools/integration_tests/read_cache/range_read_test.go b/tools/integration_tests/read_cache/range_read_test.go index deb25a53a3..7579d20b17 100644 --- a/tools/integration_tests/read_cache/range_read_test.go +++ b/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(), }, } diff --git a/tools/integration_tests/read_cache/remount_test.go b/tools/integration_tests/read_cache/remount_test.go index 05e872a744..48d9f153c2 100644 --- a/tools/integration_tests/read_cache/remount_test.go +++ b/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(), }, } diff --git a/tools/integration_tests/read_cache/setup_test.go b/tools/integration_tests/read_cache/setup_test.go index f041cd3a0c..9fe2dcb51c 100644 --- a/tools/integration_tests/read_cache/setup_test.go +++ b/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, } diff --git a/tools/integration_tests/read_cache/small_cache_ttl_test.go b/tools/integration_tests/read_cache/small_cache_ttl_test.go index 1f7dc4d4f9..7e46a2aa28 100644 --- a/tools/integration_tests/read_cache/small_cache_ttl_test.go +++ b/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(), }, } diff --git a/tools/integration_tests/run_tests_mounted_directory.sh b/tools/integration_tests/run_tests_mounted_directory.sh index aa57a8106b..4b636912c6 100755 --- a/tools/integration_tests/run_tests_mounted_directory.sh +++ b/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"