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

Change local file/directory expiry TTL from NEVER_EXPIRE to 100ms #584

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Change local file/directory expiry TTL from NEVER_EXPIRE to 100ms #584

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba0b697
Change validity of files and directory from NEVER_EXPIRE to 100 ms wh…
Oct 30, 2023
01ecbdb
Used cache config value instead to set validity
Oct 30, 2023
acf0f7e
Trying to check if removing file validity assertions works as no way …
Nov 2, 2023
b96d9bb
Removed Invalid Inode Stat test for setattr
Nov 9, 2023
f8e1e4c
Modified test for setattr on invalid stat as now it should be able to…
Nov 10, 2023
da9d0d5
Added validity update of local inode for lookup
Nov 13, 2023
c5730d3
Added resetting of InodeStat expiry in setattr as well
Nov 14, 2023
caf8315
Added the change in Changelog
Nov 14, 2023
0745dd6
Removed unnecessary cloning of inode
Nov 14, 2023
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
1 change: 0 additions & 1 deletion mountpoint-s3/src/fs/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ impl ToErrno for InodeError {
InodeError::UnlinkNotPermittedWhileWriting(_) => libc::EPERM,
InodeError::CorruptedMetadata(_) => libc::EIO,
InodeError::SetAttrNotPermittedOnRemoteInode(_) => libc::EPERM,
InodeError::SetAttrOnExpiredStat(_) => libc::EIO,
InodeError::StaleInode { .. } => libc::ESTALE,
}
}
Expand Down
51 changes: 36 additions & 15 deletions mountpoint-s3/src/inode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,13 @@ impl Superblock {
return Err(InodeError::SetAttrNotPermittedOnRemoteInode(inode.err()));
}

// Should be impossible since local file stat never expire.
if !sync.stat.is_valid() {
error!(?ino, "local inode stat already expired");
return Err(InodeError::SetAttrOnExpiredStat(inode.err()));
passaro marked this conversation as resolved.
Show resolved Hide resolved
}
let validity = match inode.kind() {
InodeKind::File => self.inner.cache_config.file_ttl,
InodeKind::Directory => self.inner.cache_config.dir_ttl,
};

// Resetting the InodeStat expiry because the new InodeStat should have new validity
sync.stat.update_validity(validity);

if let Some(t) = atime {
sync.stat.atime = t;
Expand Down Expand Up @@ -334,11 +336,17 @@ impl Superblock {
return Err(InodeError::FileAlreadyExists(inode.err()));
}

// Local inode stats never expire, because they can't be looked up remotely
let stat = match kind {
// Objects don't have an ETag until they are uploaded to S3
InodeKind::File => InodeStat::for_file(0, OffsetDateTime::now_utc(), None, None, None, NEVER_EXPIRE_TTL),
InodeKind::Directory => InodeStat::for_directory(self.inner.mount_time, NEVER_EXPIRE_TTL),
InodeKind::File => InodeStat::for_file(
0,
OffsetDateTime::now_utc(),
None,
None,
None,
self.inner.cache_config.file_ttl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this will never happen since I'm guessing applications almost always open the file immediately after performing a mknod, but an open 100ms after the mknod will now go to S3 even though we know we'll always choose the local node over a remote one.

Not worried right now, but just noting this observation.

),
InodeKind::Directory => InodeStat::for_directory(self.inner.mount_time, self.inner.cache_config.dir_ttl),
};

let state = InodeState {
Expand Down Expand Up @@ -827,9 +835,18 @@ impl SuperblockInner {
unreachable!("we know parent is a directory");
};
if writing_children.contains(&existing_inode.ino()) {
let stat = existing_inode.get_inode_state()?.stat.clone();
let mut sync = existing_inode.get_mut_inode_state()?;

let validity = match existing_inode.kind() {
InodeKind::File => self.cache_config.file_ttl,
InodeKind::Directory => self.cache_config.dir_ttl,
};
sync.stat.update_validity(validity);
let stat = sync.stat.clone();
drop(sync);

Ok(LookedUp {
inode: existing_inode,
inode: existing_inode.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not need to clone here:

Suggested change
inode: existing_inode.clone(),
inode: existing_inode,

stat,
})
} else {
Expand Down Expand Up @@ -1474,8 +1491,6 @@ pub enum InodeError {
CorruptedMetadata(InodeErrorInfo),
#[error("inode {0} is a remote inode and its attributes cannot be modified")]
SetAttrNotPermittedOnRemoteInode(InodeErrorInfo),
#[error("inode {0} stat is already expired")]
SetAttrOnExpiredStat(InodeErrorInfo),
#[error("inode {old_inode} for remote key {remote_key:?} is stale, replaced by inode {new_inode}")]
StaleInode {
remote_key: String,
Expand Down Expand Up @@ -2522,11 +2537,17 @@ mod tests {
let stat = inode.get_inode_state().unwrap().stat.clone();
assert!(!stat.is_valid());

// Should get an error back when calling setattr
// Should be able to reset expiry back and make stat valid when calling setattr
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's review this test, when/if we change the way the stats are updated in update_slow_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also add a test for lookup on local inode with Invalid InodeStat?

let atime = OffsetDateTime::UNIX_EPOCH + Duration::days(90);
let mtime = OffsetDateTime::UNIX_EPOCH + Duration::days(60);
let result = superblock.setattr(&client, ino, Some(atime), Some(mtime)).await;
assert!(matches!(result, Err(InodeError::SetAttrOnExpiredStat(_))));
let lookup = superblock
.setattr(&client, ino, Some(atime), Some(mtime))
.await
.expect("setattr should be successful");
let stat = lookup.stat;
assert_eq!(stat.atime, atime);
assert_eq!(stat.mtime, mtime);
assert!(stat.is_valid());
}

#[test]
Expand Down
Loading