Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add negative metadata cache ttl #1246

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add negative metadata cache ttl #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28838e4
add negative metadata cache ttl
notoriaga Jan 24, 2025
e3b600a
add negative_cache_ttl to other tests
notoriaga Jan 27, 2025
52774f1
Update mountpoint-s3/src/fs/config.rs
notoriaga Jan 31, 2025
3bd3836
Update mountpoint-s3/tests/fuse_tests/lookup_test.rs
notoriaga Jan 31, 2025
26e8c88
rename negative_cache_ttl to negative_metadata_ttl
notoriaga Jan 31, 2025
f546b22
add fuse lookup test
notoriaga Feb 1, 2025
6bc167c
update config struct to make cli flag name change
notoriaga Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions mountpoint-s3/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,14 @@ Learn more in Mountpoint's configuration documentation (CONFIGURATION.md).\
)]
pub metadata_ttl: Option<TimeToLive>,

#[clap(
long,
help = "Time-to-live (TTL) for cached negative entries in seconds [default: same as metadata TTL]",
value_name = "SECONDS|indefinite|minimal",
help_heading = CACHING_OPTIONS_HEADER,
)]
pub negative_metadata_ttl: Option<TimeToLive>,

#[clap(
long,
help = "Maximum size of the cache directory in MiB [default: preserve 5% of available space]",
Expand Down Expand Up @@ -934,6 +942,11 @@ where
}
tracing::trace!("using metadata TTL setting {metadata_cache_ttl:?}");
filesystem_config.cache_config = CacheConfig::new(metadata_cache_ttl);
if let Some(negative_cache_ttl) = args.negative_metadata_ttl {
filesystem_config.cache_config = filesystem_config
.cache_config
.with_negative_metadata_ttl(negative_cache_ttl);
}

match (args.disk_data_cache_config(), args.express_data_cache_config()) {
(None, Some((config, bucket_name, cache_bucket_name))) => {
Expand Down
22 changes: 22 additions & 0 deletions mountpoint-s3/src/fs/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ pub struct CacheConfig {
pub file_ttl: Duration,
/// How long the kernel will cache metadata for directories
pub dir_ttl: Duration,
/// How long the file system will cache negative entries
pub negative_cache_ttl: Duration,
/// Maximum number of negative entries to cache.
pub negative_cache_size: usize,
}
Expand Down Expand Up @@ -116,6 +118,7 @@ impl Default for CacheConfig {
serve_lookup_from_cache: false,
file_ttl,
dir_ttl,
negative_cache_ttl: file_ttl,
negative_cache_size,
}
}
Expand All @@ -130,14 +133,33 @@ impl CacheConfig {
serve_lookup_from_cache: true,
file_ttl: TimeToLive::INDEFINITE_DURATION,
dir_ttl: TimeToLive::INDEFINITE_DURATION,
negative_cache_ttl: TimeToLive::INDEFINITE_DURATION,
..Default::default()
},
TimeToLive::Duration(ttl) => Self {
serve_lookup_from_cache: true,
file_ttl: ttl,
dir_ttl: ttl,
negative_cache_ttl: ttl,
..Default::default()
},
}
}

pub fn with_negative_metadata_ttl(self, negative_metadata_ttl: TimeToLive) -> Self {
match negative_metadata_ttl {
TimeToLive::Minimal => Self {
negative_cache_ttl: Self::default().negative_cache_ttl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just setting the TTL here may not be enough. We use the same serve_lookup_from_cache flag both for the "positive" cache and the negative cache (here, in particular). I suspect we need to introduce a separate flag and ensure it is set to false on Minimal here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see an issue here where even when setting --negative-metadata-ttl minimal, the negative cache is still needlessly populated with a very short TTL. My suggestion is to add an independent flag to CacheConfig, e.g. use_negative_cache, and use it in update_from_remote to replace serve_lookup_from_cache (mountpoint-s3/mountpoint-s3/src/superblock.rs:792).

..self
},
TimeToLive::Indefinite => Self {
negative_cache_ttl: TimeToLive::INDEFINITE_DURATION,
..self
},
TimeToLive::Duration(ttl) => Self {
negative_cache_ttl: ttl,
..self
},
}
}
}
6 changes: 5 additions & 1 deletion mountpoint-s3/src/superblock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ impl Superblock {
let mut inodes = InodeMap::default();
inodes.insert(root.ino(), root);

let negative_cache = NegativeCache::new(config.cache_config.negative_cache_size, config.cache_config.file_ttl);
let negative_cache = NegativeCache::new(
config.cache_config.negative_cache_size,
config.cache_config.negative_cache_ttl,
);

let inner = SuperblockInner {
bucket: bucket.to_owned(),
Expand Down Expand Up @@ -1386,6 +1389,7 @@ mod tests {
serve_lookup_from_cache: true,
dir_ttl: ttl,
file_ttl: ttl,
negative_cache_ttl: ttl,
..Default::default()
},
s3_personality: S3Personality::Standard,
Expand Down
5 changes: 5 additions & 0 deletions mountpoint-s3/tests/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ async fn test_lookup_negative_cached() {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
..Default::default()
Expand Down Expand Up @@ -267,6 +268,7 @@ async fn test_lookup_then_open_cached() {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
..Default::default()
Expand Down Expand Up @@ -333,6 +335,7 @@ async fn test_readdir_then_open_cached() {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
..Default::default()
Expand Down Expand Up @@ -384,6 +387,7 @@ async fn test_unlink_cached() {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
allow_delete: true,
Expand Down Expand Up @@ -434,6 +438,7 @@ async fn test_mknod_cached() {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
..Default::default()
Expand Down
42 changes: 42 additions & 0 deletions mountpoint-s3/tests/fuse_tests/lookup_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ fn lookup_previously_shadowed_file_test(creator_fn: impl TestSessionCreator) {
serve_lookup_from_cache: false,
file_ttl: Duration::ZERO,
dir_ttl: Duration::ZERO,
negative_cache_ttl: Duration::ZERO,
..Default::default()
},
..Default::default()
Expand Down Expand Up @@ -198,6 +199,7 @@ fn lookup_with_negative_cache(creator_fn: impl TestSessionCreator) {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: Duration::from_secs(600),
..Default::default()
},
..Default::default()
Expand Down Expand Up @@ -232,3 +234,43 @@ fn lookup_with_negative_cache_s3() {
fn lookup_with_negative_cache_mock() {
lookup_with_negative_cache(fuse::mock_session::new);
}

fn lookup_with_negative_cache_ttl(creator_fn: impl TestSessionCreator, ttl: Duration) {
const FILE_NAME: &str = "hello.txt";
let config = TestSessionConfig {
filesystem_config: S3FilesystemConfig {
cache_config: CacheConfig {
serve_lookup_from_cache: true,
dir_ttl: Duration::from_secs(600),
file_ttl: Duration::from_secs(600),
negative_cache_ttl: ttl,
..Default::default()
},
..Default::default()
},
..Default::default()
};
let test_session = creator_fn("lookup_with_negative_cache_ttl", config);

let file_path = test_session.mount_path().join(FILE_NAME);
metadata(&file_path).expect_err("should fail as no object exists");

test_session.client().put_object(FILE_NAME, b"hello").unwrap();
metadata(&file_path).expect_err("should fail as mountpoint should use negative cache");

std::thread::sleep(ttl);

let m = metadata(&file_path).expect("should succeed as the ttl has expired");
assert!(m.file_type().is_file());
}

#[cfg(feature = "s3_tests")]
#[test]
fn lookup_with_negative_cache_ttl_s3() {
lookup_with_negative_cache_ttl(fuse::s3_session::new, Duration::from_secs(5));
}

#[test]
fn lookup_with_negative_cache_ttl_mock() {
lookup_with_negative_cache_ttl(fuse::mock_session::new, Duration::from_secs(1));
}
1 change: 1 addition & 0 deletions mountpoint-s3/tests/reftests/harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,6 +936,7 @@ mod mutations {
serve_lookup_from_cache: false,
dir_ttl: Duration::ZERO,
file_ttl: Duration::ZERO,
negative_cache_ttl: Duration::ZERO,
..Default::default()
},
..Default::default()
Expand Down
Loading