From 367971a0e9671abd2bcb45fbfa168ae6de585bb5 Mon Sep 17 00:00:00 2001 From: Em Sharnoff Date: Thu, 2 Nov 2023 09:06:16 -0700 Subject: [PATCH] vm-monitor: Remove support for file cache in tmpfs (#5617) ref neondatabase/cloud#7516. We switched everything over to file cache on disk, now time to remove support for having it in tmpfs. --- compute_tools/src/bin/compute_ctl.rs | 4 ++-- libs/vm_monitor/src/filecache.rs | 24 ++++--------------- libs/vm_monitor/src/lib.rs | 10 -------- libs/vm_monitor/src/runner.rs | 35 +++++++--------------------- 4 files changed, 15 insertions(+), 58 deletions(-) diff --git a/compute_tools/src/bin/compute_ctl.rs b/compute_tools/src/bin/compute_ctl.rs index 263f71452d..81d4320b14 100644 --- a/compute_tools/src/bin/compute_ctl.rs +++ b/compute_tools/src/bin/compute_ctl.rs @@ -283,7 +283,6 @@ fn main() -> Result<()> { .expect("--vm-monitor-addr should always be set because it has a default arg"); let file_cache_connstr = matches.get_one::("filecache-connstr"); let cgroup = matches.get_one::("cgroup"); - let file_cache_on_disk = matches.get_flag("file-cache-on-disk"); // Only make a runtime if we need to. // Note: it seems like you can make a runtime in an inner scope and @@ -310,7 +309,6 @@ fn main() -> Result<()> { cgroup: cgroup.cloned(), pgconnstr: file_cache_connstr.cloned(), addr: vm_monitor_addr.clone(), - file_cache_on_disk, })), token.clone(), )) @@ -482,6 +480,8 @@ fn cli() -> clap::Command { .value_name("FILECACHE_CONNSTR"), ) .arg( + // DEPRECATED, NO LONGER DOES ANYTHING. + // See https://github.com/neondatabase/cloud/issues/7516 Arg::new("file-cache-on-disk") .long("file-cache-on-disk") .action(clap::ArgAction::SetTrue), diff --git a/libs/vm_monitor/src/filecache.rs b/libs/vm_monitor/src/filecache.rs index 3a860500f1..fe71e11197 100644 --- a/libs/vm_monitor/src/filecache.rs +++ b/libs/vm_monitor/src/filecache.rs @@ -21,11 +21,6 @@ pub struct FileCacheState { #[derive(Debug)] pub struct FileCacheConfig { - /// Whether the file cache is *actually* stored in memory (e.g. by writing to - /// a tmpfs or shmem file). If true, the size of the file cache will be counted against the - /// memory available for the cgroup. - pub(crate) in_memory: bool, - /// The size of the file cache, in terms of the size of the resource it consumes /// (currently: only memory) /// @@ -59,22 +54,9 @@ pub struct FileCacheConfig { spread_factor: f64, } -impl FileCacheConfig { - pub fn default_in_memory() -> Self { +impl Default for FileCacheConfig { + fn default() -> Self { Self { - in_memory: true, - // 75 % - resource_multiplier: 0.75, - // 640 MiB; (512 + 128) - min_remaining_after_cache: NonZeroU64::new(640 * MiB).unwrap(), - // ensure any increase in file cache size is split 90-10 with 10% to other memory - spread_factor: 0.1, - } - } - - pub fn default_on_disk() -> Self { - Self { - in_memory: false, resource_multiplier: 0.75, // 256 MiB - lower than when in memory because overcommitting is safe; if we don't have // memory, the kernel will just evict from its page cache, rather than e.g. killing @@ -83,7 +65,9 @@ impl FileCacheConfig { spread_factor: 0.1, } } +} +impl FileCacheConfig { /// Make sure fields of the config are consistent. pub fn validate(&self) -> anyhow::Result<()> { // Single field validity diff --git a/libs/vm_monitor/src/lib.rs b/libs/vm_monitor/src/lib.rs index 1cbfdc6ba6..a844f78bd6 100644 --- a/libs/vm_monitor/src/lib.rs +++ b/libs/vm_monitor/src/lib.rs @@ -39,16 +39,6 @@ pub struct Args { #[arg(short, long)] pub pgconnstr: Option, - /// Flag to signal that the Postgres file cache is on disk (i.e. not in memory aside from the - /// kernel's page cache), and therefore should not count against available memory. - // - // NB: Ideally this flag would directly refer to whether the file cache is in memory (rather - // than a roundabout way, via whether it's on disk), but in order to be backwards compatible - // during the switch away from an in-memory file cache, we had to default to the previous - // behavior. - #[arg(long)] - pub file_cache_on_disk: bool, - /// The address we should listen on for connection requests. For the /// agent, this is 0.0.0.0:10301. For the informant, this is 127.0.0.1:10369. #[arg(short, long)] diff --git a/libs/vm_monitor/src/runner.rs b/libs/vm_monitor/src/runner.rs index 99bea6d7ca..f162f53d24 100644 --- a/libs/vm_monitor/src/runner.rs +++ b/libs/vm_monitor/src/runner.rs @@ -156,10 +156,7 @@ impl Runner { // memory limits. if let Some(connstr) = &args.pgconnstr { info!("initializing file cache"); - let config = match args.file_cache_on_disk { - true => FileCacheConfig::default_on_disk(), - false => FileCacheConfig::default_in_memory(), - }; + let config = FileCacheConfig::default(); let mut file_cache = FileCacheState::new(connstr, config, token.clone()) .await @@ -187,10 +184,7 @@ impl Runner { info!("file cache size actually got set to {actual_size}") } - if args.file_cache_on_disk { - file_cache_disk_size = actual_size; - } - + file_cache_disk_size = actual_size; state.filecache = Some(file_cache); } @@ -239,17 +233,11 @@ impl Runner { let requested_mem = target.mem; let usable_system_memory = requested_mem.saturating_sub(self.config.sys_buffer_bytes); - let (expected_file_cache_size, expected_file_cache_disk_size) = self + let expected_file_cache_size = self .filecache .as_ref() - .map(|file_cache| { - let size = file_cache.config.calculate_cache_size(usable_system_memory); - match file_cache.config.in_memory { - true => (size, 0), - false => (size, size), - } - }) - .unwrap_or((0, 0)); + .map(|file_cache| file_cache.config.calculate_cache_size(usable_system_memory)) + .unwrap_or(0); if let Some(cgroup) = &self.cgroup { let (last_time, last_history) = *cgroup.watcher.borrow(); @@ -273,7 +261,7 @@ impl Runner { let new_threshold = self .config - .cgroup_threshold(usable_system_memory, expected_file_cache_disk_size); + .cgroup_threshold(usable_system_memory, expected_file_cache_size); let current = last_history.avg_non_reclaimable; @@ -300,13 +288,10 @@ impl Runner { .set_file_cache_size(expected_file_cache_size) .await .context("failed to set file cache size")?; - if !file_cache.config.in_memory { - file_cache_disk_size = actual_usage; - } + file_cache_disk_size = actual_usage; let message = format!( - "set file cache size to {} MiB (in memory = {})", + "set file cache size to {} MiB", bytes_to_mebibytes(actual_usage), - file_cache.config.in_memory, ); info!("downscale: {message}"); status.push(message); @@ -357,9 +342,7 @@ impl Runner { .set_file_cache_size(expected_usage) .await .context("failed to set file cache size")?; - if !file_cache.config.in_memory { - file_cache_disk_size = actual_usage; - } + file_cache_disk_size = actual_usage; if actual_usage != expected_usage { warn!(