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.

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

Fix issue with fstat after upload #1085

Merged
merged 1 commit into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions mountpoint-s3/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
12 changes: 6 additions & 6 deletions mountpoint-s3/src/fs/handles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down Expand Up @@ -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");
}
Expand Down
4 changes: 2 additions & 2 deletions mountpoint-s3/src/superblock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions mountpoint-s3/src/superblock/inode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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));
Expand Down
2 changes: 0 additions & 2 deletions mountpoint-s3/tests/fuse_tests/write_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading