Fix ESTALE after upload (#1085)

Signed-off-by: Alessandro Passaro <alexpax@amazon.com> Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> Co-authored-by: Alessandro Passaro <alexpax@amazon.com>
awslabs · Oct 29, 2024 · ed999df · ed999df
1 parent 05a50da
commit ed999df
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 12 deletions.
diff --git a/mountpoint-s3/CHANGELOG.md b/mountpoint-s3/CHANGELOG.md
@@ -1,5 +1,13 @@
 ## Unreleased
 
+### New features
+
+### Breaking changes
+
+### Other changes
+
+* Fix an issue where `fstat` would fail and return `ESTALE` when invoked on a file descriptor after a successful `fsync`. ([#1085](https://github.com/awslabs/mountpoint-s3/pull/1085))
+
 ## v1.10.0 (October 15, 2024)
 
 ### New features

diff --git a/mountpoint-s3/src/fs/handles.rs b/mountpoint-s3/src/fs/handles.rs
@@ -175,7 +175,7 @@ impl<Client: ObjectClient> UploadState<Client> {
                 // Abort the request.
                 match std::mem::replace(self, Self::Failed(e.to_errno())) {
                     UploadState::InProgress { handle, .. } => {
-                        if let Err(err) = handle.finish() {
+                        if let Err(err) = handle.finish(None) {
                             // Log the issue but still return the write error.
                             error!(?err, ?key, "error updating the inode status");
                         }
@@ -231,14 +231,14 @@ impl<Client: ObjectClient> UploadState<Client> {
 
     async fn complete_upload(upload: UploadRequest<Client>, key: &str, handle: WriteHandle) -> Result<(), Error> {
         let size = upload.size();
-        let put_result = match upload.complete().await {
-            Ok(_) => {
+        let (put_result, etag) = match upload.complete().await {
+            Ok(result) => {
                 debug!(key, size, "put succeeded");
-                Ok(())
+                (Ok(()), Some(result.etag))
             }
-            Err(e) => Err(err!(libc::EIO, source:e, "put failed")),
+            Err(e) => (Err(err!(libc::EIO, source:e, "put failed")), None),
         };
-        if let Err(err) = handle.finish() {
+        if let Err(err) = handle.finish(etag) {
             // Log the issue but still return put_result.
             error!(?err, ?key, "error updating the inode status");
         }

diff --git a/mountpoint-s3/src/superblock.rs b/mountpoint-s3/src/superblock.rs
@@ -1964,7 +1964,7 @@ mod tests {
 
         // Invoke [finish_writing], without actually adding the
         // object to the client
-        writehandle.finish().unwrap();
+        writehandle.finish(None).unwrap();
 
         // All nested dirs disappear
         let dirname = nested_dirs.first().unwrap();
@@ -2149,7 +2149,7 @@ mod tests {
         assert_eq!(stat.mtime, mtime);
 
         // Invoke [finish_writing] to make the file remote
-        writehandle.finish().unwrap();
+        writehandle.finish(Some(ETag::for_tests())).unwrap();
 
         // Should get an error back when calling setattr
         let result = superblock

diff --git a/mountpoint-s3/src/superblock/inode.rs b/mountpoint-s3/src/superblock/inode.rs
@@ -5,7 +5,7 @@ use std::os::unix::ffi::OsStrExt as _;
 use std::time::{Duration, SystemTime};
 
 use fuser::FileType;
-use mountpoint_s3_client::types::RestoreStatus;
+use mountpoint_s3_client::types::{ETag, RestoreStatus};
 use mountpoint_s3_crt::checksums::crc32c::{self, Crc32c};
 use time::OffsetDateTime;
 use tracing::trace;
@@ -470,7 +470,7 @@ impl WriteHandle {
     }
 
     /// Update status of the inode and of containing "local" directories.
-    pub fn finish(self) -> Result<(), InodeError> {
+    pub fn finish(self, etag: Option<ETag>) -> Result<(), InodeError> {
         // Collect ancestor inodes that may need updating,
         // from parent to first remote ancestor.
         let ancestors = {
@@ -500,6 +500,7 @@ impl WriteHandle {
         match state.write_status {
             WriteStatus::LocalOpen => {
                 state.write_status = WriteStatus::Remote;
+                state.stat.etag = etag.map(|e| e.into_inner());
 
                 // Invalidate the inode's stats so we refresh them from S3 when next queried
                 state.stat.update_validity(Duration::from_secs(0));

diff --git a/mountpoint-s3/tests/fuse_tests/write_test.rs b/mountpoint-s3/tests/fuse_tests/write_test.rs
@@ -374,14 +374,12 @@ where
 }
 
 #[cfg(feature = "s3_tests")]
-#[ignore = "file upload replaces inode breaking fstat due to returning ESTALE"]
 #[test_case(true; "with fsync")]
 #[test_case(true; "without fsync")]
 fn fstat_after_writing_s3(with_fsync: bool) {
     fstat_after_writing(fuse::s3_session::new, with_fsync);
 }
 
-#[ignore = "file upload replaces inode breaking fstat due to returning ESTALE"]
 #[test_case(true; "with fsync")]
 #[test_case(true; "without fsync")]
 fn fstat_after_writing_mock(with_fsync: bool) {