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

* Change validity of files and directory from NEVER_EXPIRE to 100 ms while create() Signed-off-by: Ankit Saurabh <[email protected]> * Used cache config value instead to set validity Signed-off-by: Ankit Saurabh <[email protected]> * Trying to check if removing file validity assertions works as no way to test on local system Signed-off-by: Ankit Saurabh <[email protected]> * Removed Invalid Inode Stat test for setattr Signed-off-by: Ankit Saurabh <[email protected]> * Modified test for setattr on invalid stat as now it should be able to reset the stat expiry Signed-off-by: Ankit Saurabh <[email protected]> * Added validity update of local inode for lookup Signed-off-by: Ankit Saurabh <[email protected]> * Added resetting of InodeStat expiry in setattr as well Signed-off-by: Ankit Saurabh <[email protected]> * Added the change in Changelog Signed-off-by: Ankit Saurabh <[email protected]> * Removed unnecessary cloning of inode Signed-off-by: Ankit Saurabh <[email protected]> --------- Signed-off-by: Ankit Saurabh <[email protected]>
awslabs · Nov 14, 2023 · d38d45f · d38d45f
1 parent 493fc23
commit d38d45f
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 15 deletions.
diff --git a/mountpoint-s3/CHANGELOG.md b/mountpoint-s3/CHANGELOG.md
@@ -1,4 +1,5 @@
 ## Unreleased
+* Future creates for file that are deleted remotely should now succeed. ([#584](https://github.com/awslabs/mountpoint-s3/pull/584))
 
 ### Breaking changes
 * No breaking changes.

diff --git a/mountpoint-s3/src/fs/error.rs b/mountpoint-s3/src/fs/error.rs
@@ -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,
         }
     }

diff --git a/mountpoint-s3/src/inode.rs b/mountpoint-s3/src/inode.rs
@@ -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()));
-        }
+        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;
@@ -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,
+            ),
+            InodeKind::Directory => InodeStat::for_directory(self.inner.mount_time, self.inner.cache_config.dir_ttl),
         };
 
         let state = InodeState {
@@ -827,7 +835,16 @@ 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,
                         stat,
@@ -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,
@@ -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
         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]