From b5bda60849183d50d2fd9e63c11c46d44df05ef2 Mon Sep 17 00:00:00 2001 From: Andrew Peace Date: Fri, 25 Oct 2024 11:27:08 +0000 Subject: [PATCH 1/5] Set max_background FUSE config to 64 by default. This improves sequential read performance on instances with multiple 100Gbps network interfaces. It controls the number of requests that are allowed in the pending queue that are classified as background, which includes at least some read requests. It also indirectly controls the "congestion threshold", which is set by default to 75% of the max background value. When the congestion threshold is reached, FUSE will stop sending the asynchronous part of readaheads from paged IO to the filesystem. Testing on 2 NIC instances shows up to approximately 29% speed-up on a sequential read workload with 32 open files, from 76.74 to 99Gbps, for paged IO. Although we don't have enough instrumentation to fully understand the change in queueing behaviour in FUSE, we think it is likely because we're able to serve sufficient readahead requests for the object before hitting the congestion threshold when the limit is higher, thus allowing mountpoint to start prefetching later parts of the object sooner. The value of 64 was picked by experimentation with values between 16 (the default) and 256, as well as specifically setting the congestion threshold. Increasing the value generally led to better performance up to 64, after which performance doesn't improve further (at least not significantly). We wanted to choose the lowest value that seemed reasonable for the desired performance improvement, to reduce the chance of affecting a workload that wasn't being tested. As well as the standard regression tests, the change was tested on trn1 instances with a 256KB sequential read workload reading 32 files in parallel over 1, 2, and 4 network interfaces. It does not regress our standard benchmarks nor performance on this test with 1 NIC in use. This change also temporarily introduces two environment variables to tune the behaviour, so we can isolate this change if a particular workload is found to regress. Signed-off-by: Andrew Peace --- mountpoint-s3/src/fs.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 403e0acf9..532b68b2e 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -155,6 +155,34 @@ where pub async fn init(&self, config: &mut KernelConfig) -> Result<(), libc::c_int> { let _ = config.add_capabilities(fuser::consts::FUSE_DO_READDIRPLUS); + // Set max_background FUSE parameter to 64 by default, or override with environment variable: + // NOTE: Support for this environment variable may be removed in future without notice. + if let Some(user_max_background) = std::env::var_os("UNSTABLE_MAX_BACKGROUND") { + let max_background = user_max_background + .to_string_lossy() + .parse::() + .expect("invalid env var value for UNSTABLE_MAX_BACKGROUND"); + let old = config + .set_max_background(max_background) + .expect("unable to set max background"); + tracing::warn!("set max background to {} from {}", max_background, old) + } else { + let _ = config + .set_max_background(64) + .expect("unable to set max background to default value"); + } + // Override FUSE congestion threshold if environment variable is present: + // NOTE: Support for this environment variable may be removed in future without notice. + if let Some(user_congestion_threshold) = std::env::var_os("UNSTABLE_CONGESTION_THRESHOLD") { + let congestion_threshold = user_congestion_threshold + .to_string_lossy() + .parse::() + .expect("invalid env var value for UNSTABLE_CONGESTION_THRESHOLD"); + let old = config + .set_congestion_threshold(congestion_threshold) + .expect("unable to set congestion threshold"); + tracing::warn!("set congestion threshold to {} from {}", congestion_threshold, old); + } if self.config.allow_overwrite { // Overwrites require FUSE_ATOMIC_O_TRUNC capability on the host, so we will panic if the // host doesn't support it. From c41c84f21c1900fbf7fdac80d4eb5e3fe3517fdb Mon Sep 17 00:00:00 2001 From: Andrew Peace Date: Sun, 17 Nov 2024 14:52:20 +0000 Subject: [PATCH 2/5] Address PR feedback (squash on merge please) - improved error messages - minor refactor of value parsing. Validated that the defaults and overrides are set when mounting the filesystem. Signed-off-by: Andrew Peace --- mountpoint-s3/src/fs.rs | 51 +++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 532b68b2e..8ba9825f3 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -153,36 +153,53 @@ where self.next_handle.fetch_add(1, Ordering::SeqCst) } + /// Helper to return the u16 value in an environment variable, or panic. Useful for unstable overrides. + fn parse_env_var_to_u16(var_name: &str, var_value: std::ffi::OsString) -> u16 { + var_value.to_string_lossy().parse::().unwrap_or_else(|_| { + panic!( + "Invalid value for environment variable {}. Must be positive integer.", + var_name + ) + }) + } + pub async fn init(&self, config: &mut KernelConfig) -> Result<(), libc::c_int> { + const ENV_VAR_KEY_MAX_BACKGROUND: &str = "UNSTABLE_MOUNTPOINT_MAX_BACKGROUND"; + const ENV_VAR_KEY_CONGESTION_THRESHOLD: &str = "UNSTABLE_MOUNTPOINT_CONGESTION_THRESHOLD"; let _ = config.add_capabilities(fuser::consts::FUSE_DO_READDIRPLUS); - // Set max_background FUSE parameter to 64 by default, or override with environment variable: + // Set max_background FUSE parameter to 64 by default, or override with environment variable. // NOTE: Support for this environment variable may be removed in future without notice. - if let Some(user_max_background) = std::env::var_os("UNSTABLE_MAX_BACKGROUND") { - let max_background = user_max_background - .to_string_lossy() - .parse::() - .expect("invalid env var value for UNSTABLE_MAX_BACKGROUND"); + if let Some(user_max_background) = std::env::var_os(ENV_VAR_KEY_MAX_BACKGROUND) { + let max_background = Self::parse_env_var_to_u16(ENV_VAR_KEY_MAX_BACKGROUND, user_max_background); let old = config .set_max_background(max_background) - .expect("unable to set max background"); - tracing::warn!("set max background to {} from {}", max_background, old) + .unwrap_or_else(|_| panic!("Unable to set FUSE max_background configuration to {}", max_background)); + tracing::warn!( + "Successfully overridden FUSE max_background configuration to {} (was {}) from unstable environment variable.", + max_background, + old + ); } else { let _ = config .set_max_background(64) - .expect("unable to set max background to default value"); + .expect("unable to set FUSE max_background to default value"); } - // Override FUSE congestion threshold if environment variable is present: + + // Override FUSE congestion threshold if environment variable is present. // NOTE: Support for this environment variable may be removed in future without notice. - if let Some(user_congestion_threshold) = std::env::var_os("UNSTABLE_CONGESTION_THRESHOLD") { - let congestion_threshold = user_congestion_threshold - .to_string_lossy() - .parse::() - .expect("invalid env var value for UNSTABLE_CONGESTION_THRESHOLD"); + if let Some(user_congestion_threshold) = std::env::var_os(ENV_VAR_KEY_CONGESTION_THRESHOLD) { + let congestion_threshold = + Self::parse_env_var_to_u16(ENV_VAR_KEY_CONGESTION_THRESHOLD, user_congestion_threshold); let old = config .set_congestion_threshold(congestion_threshold) - .expect("unable to set congestion threshold"); - tracing::warn!("set congestion threshold to {} from {}", congestion_threshold, old); + .unwrap_or_else(|_| panic!("unable to set FUSE congestion_threshold to {}", congestion_threshold)); + tracing::warn!( + "Successfully overridden FUSE congestion_threshold configuration to {} (was {}) from unstable environment variable.", + congestion_threshold, // Note: fixed a bug here, was using max_background + old + ); } + if self.config.allow_overwrite { // Overwrites require FUSE_ATOMIC_O_TRUNC capability on the host, so we will panic if the // host doesn't support it. From 0808c237e707d8114e0b8398b96b6643ed2ab9cb Mon Sep 17 00:00:00 2001 From: Andy Peace Date: Mon, 18 Nov 2024 10:57:43 +0000 Subject: [PATCH 3/5] Remove note comment. from fs.rs. Co-authored-by: Daniel Carl Jones Signed-off-by: Andy Peace --- mountpoint-s3/src/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 8ba9825f3..3dea8a56a 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -195,7 +195,7 @@ where .unwrap_or_else(|_| panic!("unable to set FUSE congestion_threshold to {}", congestion_threshold)); tracing::warn!( "Successfully overridden FUSE congestion_threshold configuration to {} (was {}) from unstable environment variable.", - congestion_threshold, // Note: fixed a bug here, was using max_background + congestion_threshold, old ); } From 2a72c0252251dbb96776a1b8419862c5b80c17cc Mon Sep 17 00:00:00 2001 From: Andy Peace Date: Mon, 18 Nov 2024 10:59:19 +0000 Subject: [PATCH 4/5] Warn rather than panic when unable to set max_background to desired new default value. Co-authored-by: Daniel Carl Jones Signed-off-by: Andy Peace --- mountpoint-s3/src/fs.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 3dea8a56a..37587fc7b 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -180,9 +180,12 @@ where old ); } else { - let _ = config - .set_max_background(64) - .expect("unable to set FUSE max_background to default value"); + const DEFAULT_MAX_BACKGROUND: u16 = 64; + let max_background_result = config + .set_max_background(DEFAULT_MAX_BACKGROUND); + if max_background_result.is_err() { + tracing::warn!("failed to set FUSE max_background to {}, using Kernel default", DEFAULT_MAX_BACKGROUND); + } } // Override FUSE congestion threshold if environment variable is present. From 6848d82f48b585e9c8fac4d5afada16edf20b0d0 Mon Sep 17 00:00:00 2001 From: Andrew Peace Date: Mon, 18 Nov 2024 11:25:40 +0000 Subject: [PATCH 5/5] Fix formatting. Signed-off-by: Andrew Peace --- mountpoint-s3/src/fs.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 37587fc7b..c2c44055c 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -181,10 +181,12 @@ where ); } else { const DEFAULT_MAX_BACKGROUND: u16 = 64; - let max_background_result = config - .set_max_background(DEFAULT_MAX_BACKGROUND); + let max_background_result = config.set_max_background(DEFAULT_MAX_BACKGROUND); if max_background_result.is_err() { - tracing::warn!("failed to set FUSE max_background to {}, using Kernel default", DEFAULT_MAX_BACKGROUND); + tracing::warn!( + "failed to set FUSE max_background to {}, using Kernel default", + DEFAULT_MAX_BACKGROUND + ); } }