Skip to content

Commit

Permalink
Rename internal CacheConfig field 'prefer_s3' to 'serve_lookup_from_c…
Browse files Browse the repository at this point in the history
…ache'

Signed-off-by: Daniel Carl Jones <[email protected]>
  • Loading branch information
dannycjones committed Oct 24, 2023
1 parent 15bec26 commit 0873fdd
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 18 deletions.
13 changes: 7 additions & 6 deletions mountpoint-s3/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,14 @@ fn are_from_same_process(pid1: u32, pid2: u32) -> bool {

#[derive(Debug, Clone)]
pub struct CacheConfig {
/// Should the file system check S3 even when a valid cached entry may be available?
/// Should the file system serve lookup requests including open from cached entries,
/// or instead check S3 even when a valid cached entry may be available?
///
/// When enabled, some operations such as `getattr` are allowed to be served from cache
/// Even when disabled, some operations such as `getattr` are allowed to be served from cache
/// with a short TTL since Linux filesystems behave badly when the TTL is zero.
/// For example, results from `readdir` will expire immediately, and so the kernel will
/// For example, results from `readdir` would expire immediately, and the kernel would
/// immediately `getattr` every entry returned from `readdir`.
pub prefer_s3: bool,
pub serve_lookup_from_cache: bool,
/// How long the kernel will cache metadata for files
pub file_ttl: Duration,
/// How long the kernel will cache metadata for directories
Expand All @@ -280,7 +281,7 @@ impl Default for CacheConfig {
let dir_ttl = Duration::from_millis(1000);

Self {
prefer_s3: true,
serve_lookup_from_cache: false,
file_ttl,
dir_ttl,
}
Expand Down Expand Up @@ -531,7 +532,7 @@ where
pub async fn open(&self, ino: InodeNo, flags: i32, pid: u32) -> Result<Opened, Error> {
trace!("fs:open with ino {:?} flags {:?} pid {:?}", ino, flags, pid);

let force_revalidate = self.config.cache_config.prefer_s3;
let force_revalidate = !self.config.cache_config.serve_lookup_from_cache;
let lookup = self.superblock.getattr(&self.client, ino, force_revalidate).await?;

match lookup.inode.kind() {
Expand Down
33 changes: 24 additions & 9 deletions mountpoint-s3/src/inode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,12 @@ impl Superblock {
trace!(parent=?parent_ino, ?name, "lookup");
let lookup = self
.inner
.lookup_by_name(client, parent_ino, name, self.inner.cache_config.prefer_s3)
.lookup_by_name(
client,
parent_ino,
name,
self.inner.cache_config.serve_lookup_from_cache,
)
.await?;
self.inner.remember(&lookup.inode);
Ok(lookup)
Expand Down Expand Up @@ -303,7 +308,7 @@ impl Superblock {

let existing = self
.inner
.lookup_by_name(client, dir, name, self.inner.cache_config.prefer_s3)
.lookup_by_name(client, dir, name, self.inner.cache_config.serve_lookup_from_cache)
.await;
match existing {
Ok(lookup) => return Err(InodeError::FileAlreadyExists(lookup.inode.err())),
Expand Down Expand Up @@ -360,7 +365,12 @@ impl Superblock {
) -> Result<(), InodeError> {
let LookedUp { inode, .. } = self
.inner
.lookup_by_name(client, parent_ino, name, self.inner.cache_config.prefer_s3)
.lookup_by_name(
client,
parent_ino,
name,
self.inner.cache_config.serve_lookup_from_cache,
)
.await?;

if inode.kind() == InodeKind::File {
Expand Down Expand Up @@ -429,7 +439,12 @@ impl Superblock {
let parent = self.inner.get(parent_ino)?;
let LookedUp { inode, .. } = self
.inner
.lookup_by_name(client, parent_ino, name, self.inner.cache_config.prefer_s3)
.lookup_by_name(
client,
parent_ino,
name,
self.inner.cache_config.serve_lookup_from_cache,
)
.await?;

if inode.kind() == InodeKind::Directory {
Expand Down Expand Up @@ -534,7 +549,7 @@ impl SuperblockInner {
client: &OC,
parent_ino: InodeNo,
name: &OsStr,
skip_cache: bool,
allow_cache: bool,
) -> Result<LookedUp, InodeError> {
let name = name
.to_str()
Expand All @@ -546,10 +561,10 @@ impl SuperblockInner {
return Err(InodeError::InvalidFileName(name.into()));
}

let lookup = if skip_cache {
None
} else {
let lookup = if allow_cache {
self.cache_lookup(parent_ino, name)
} else {
None
};

let lookup = match lookup {
Expand Down Expand Up @@ -1635,7 +1650,7 @@ mod tests {
bucket,
&prefix,
CacheConfig {
prefer_s3: false,
serve_lookup_from_cache: true,
dir_ttl: ttl,
file_ttl: ttl,
},
Expand Down
2 changes: 1 addition & 1 deletion mountpoint-s3/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ fn mount(args: CliArgs) -> anyhow::Result<FuseSession> {
// TODO: Review default for TTL
let metadata_cache_ttl = args.metadata_cache_ttl.unwrap_or(Duration::from_secs(3600));
filesystem_config.cache_config = CacheConfig {
prefer_s3: false,
serve_lookup_from_cache: true,
dir_ttl: metadata_cache_ttl,
file_ttl: metadata_cache_ttl,
};
Expand Down
2 changes: 1 addition & 1 deletion mountpoint-s3/tests/fuse_tests/lookup_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ where
{
let filesystem_config = S3FilesystemConfig {
cache_config: CacheConfig {
prefer_s3: true,
serve_lookup_from_cache: false,
file_ttl: Duration::ZERO,
dir_ttl: Duration::ZERO,
},
Expand Down
2 changes: 1 addition & 1 deletion mountpoint-s3/tests/reftests/harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,7 @@ mod mutations {
allow_delete: true,
cache_config: CacheConfig {
// We are only interested in strong consistency for the reference tests. FUSE isn't even in the loop.
prefer_s3: true,
serve_lookup_from_cache: false,
dir_ttl: Duration::ZERO,
file_ttl: Duration::ZERO,
},
Expand Down

0 comments on commit 0873fdd

Please sign in to comment.