Implement fsync and handle write errors #313

passaro · 2023-06-23T14:55:15Z

Implement fsync to allow users to complete a put request and receive confirmation that it succeeded or failed. If a file handle is released without a call to fsync, release will still complete the upload as before.

Wrap the UploadRequest in the file handle in a new UploadState enum, in order to detect:

on release, whether the request had been already completed by an fsync call,
write is invoked after an fsync,
write (or fsync) is invoked after a previous call failed.

Also adds support for put failures to FailureClient.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

passaro · 2023-06-26T10:44:47Z

Note that this PR replaces #308

jamesbornholt · 2023-06-26T21:43:34Z

mountpoint-s3/src/fs.rs

+            FileHandleType::Read { .. } => return Ok(()),
+        };
+        let Some(in_progress_request) = request.take() else {
+            error!(key=?file_handle.full_key, "upload already completed or aborted");


I think we talked about tracking a little more state in the handle so we could distinguish these two cases. Is that worth doing?

Also, should be warn!

I've added UploadState to keep track of failure vs completion.

jamesbornholt · 2023-06-26T21:50:48Z

mountpoint-s3/src/fs.rs

                let request = request.into_inner();
-                let size = request.size() as usize;
-                let put = request.complete().await;
-                let result = match put {
-                    Ok(_) => {
-                        debug!(key, size, "put succeeded");
-                        Ok(())
-                    }
-                    Err(e) => {
-                        error!(key, size, "put failed, object was not uploaded: {e:?}");
-                        // This won't actually be seen by the user because `release` is async, but
-                        // it's the right thing to do.
-                        Err(libc::EIO)
+                if let Some(in_progress_request) = request {


This can just be

if let Some(request) = request.into_inner() { // ...

jamesbornholt · 2023-06-26T21:51:05Z

mountpoint-s3/src/fs.rs

+                    match in_progress_request.complete().await {
+                        Ok(_) => debug!(key, size, "put succeeded"),
+                        Err(e) => {
+                            error!(key, size, "put failed, object was not uploaded: {e:?}");


We should keep the comment about the error not being user-visible.

jamesbornholt · 2023-06-26T21:57:52Z

mountpoint-s3/src/upload.rs

    bucket: String,
    key: String,
    next_request_offset: u64,
    request: Client::PutObjectRequest,
+    _handle: Handle,


I'm not sure the handle should be here -- it doesn't really have anything to do with S3 uploads (it's a filesystem concept). It should probably be back in fs. If that gets too messy, we could factor out a WriteFileHandle type and change the enum to FileHandleType::Write(AsyncMutex<Option<WriteFileHandle>>) so that we can put the transition logic onto that struct instead.

So something like

struct WriteFileHandle { upload: UploadRequest<Client, WriteHandle>, token: WriteHandle, } impl WriteFileHandle { fn complete(mut self) -> Result<(), libc::c_int> { // ...the common logic that lives in both `fsync` and `release` right now... // and explicitly complete the write handle self.token.finish_writing()?; } }

The new UploadState struct does something similar, but I've actually moved the WriteHandle out of it and I'm calling finish_writing() only on release.
I still need to think a bit more about what should happen to the inode on failure and whether the current finish_writing() needs to behave differently in case of failure (e.g. should it still convert local directories to remote?). But maybe we can review that in a separate change.

jamesbornholt · 2023-06-26T21:59:02Z

mountpoint-s3/src/inode.rs

+impl Drop for WriteHandle {
+    fn drop(&mut self) {
+        if let Err(e) = self.finish_writing() {
+            error!(error=?e, ino=self.ino, "error closing file handle");
+        }
+    }
+}
+


See my other comment -- I don't like moving fallible stuff like this into Drop impls, it robs you of any chance to expose the error.

jamesbornholt · 2023-06-28T21:29:47Z

mountpoint-s3/src/fs.rs

+        let upload = match self {
+            Self::InProgress(upload) => upload,
+            Self::Failed | Self::Completed => {
+                // Nothing to do.
+                return Ok(());
+            }
+        };
+        Self::complete_upload(upload, key).await


nit:

Suggested change

let upload = match self {

Self::InProgress(upload) => upload,

Self::Failed | Self::Completed => {

// Nothing to do.

return Ok(());

}

};

Self::complete_upload(upload, key).await

match self {

Self::InProgress(upload) => Self::complete_upload(upload, key).await,

Self::Failed | Self::Completed => Ok(()),

}

jamesbornholt · 2023-06-28T21:33:46Z

mountpoint-s3/src/fs.rs

+    }
+
+    async fn complete(&mut self, key: &str) -> Result<(), libc::c_int> {
+        let upload = match std::mem::replace(self, Self::Completed) {


This is kinda weird because it can replace Self::Failed with Self::Completed. I think that only messes up error messages in some edge cases, but probably we should try to get it right anyway.

Signed-off-by: Alessandro Passaro <[email protected]>

passaro temporarily deployed to PR integration tests June 23, 2023 14:55 — with GitHub Actions Inactive

passaro mentioned this pull request Jun 23, 2023

Implement fsync and handle write errors #308

Closed

passaro temporarily deployed to PR integration tests June 26, 2023 10:42 — with GitHub Actions Inactive

passaro changed the title ~~Implement fsync and handle write errors (v2)~~ Implement fsync and handle write errors Jun 26, 2023

passaro marked this pull request as ready for review June 26, 2023 10:45

passaro requested a review from jamesbornholt June 26, 2023 10:45

jamesbornholt reviewed Jun 26, 2023

View reviewed changes

passaro temporarily deployed to PR integration tests June 27, 2023 14:46 — with GitHub Actions Inactive

passaro had a problem deploying to PR integration tests June 27, 2023 14:46 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests June 27, 2023 15:05 — with GitHub Actions Inactive

jamesbornholt reviewed Jun 28, 2023

View reviewed changes

passaro had a problem deploying to PR integration tests June 29, 2023 05:31 — with GitHub Actions Failure

passaro deployed to PR integration tests June 29, 2023 05:31 — with GitHub Actions Active

passaro enabled auto-merge (squash) June 29, 2023 05:36

passaro had a problem deploying to PR integration tests June 29, 2023 10:05 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests June 29, 2023 10:05 — with GitHub Actions Inactive

Implement fsync

2f9be92

Signed-off-by: Alessandro Passaro <[email protected]>

passaro force-pushed the fsync-v2 branch from ebe1bed to 2f9be92 Compare June 29, 2023 14:13

passaro temporarily deployed to PR integration tests June 29, 2023 14:13 — with GitHub Actions Inactive

jamesbornholt approved these changes Jun 29, 2023

View reviewed changes

passaro merged commit c89f05d into awslabs:main Jun 29, 2023

passaro deleted the fsync-v2 branch July 3, 2023 07:22

passaro mentioned this pull request Jul 20, 2023

Update inode status on fsync or write failure #395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fsync and handle write errors #313

Implement fsync and handle write errors #313

passaro commented Jun 23, 2023 •

edited

Loading

passaro commented Jun 26, 2023

jamesbornholt Jun 26, 2023

jamesbornholt Jun 26, 2023

passaro Jun 27, 2023

jamesbornholt Jun 26, 2023

jamesbornholt Jun 26, 2023

jamesbornholt Jun 26, 2023 •

edited

Loading

passaro Jun 27, 2023

jamesbornholt Jun 26, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

Implement fsync and handle write errors #313

Implement fsync and handle write errors #313

Conversation

passaro commented Jun 23, 2023 • edited Loading

passaro commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Jun 23, 2023 •

edited

Loading

jamesbornholt Jun 26, 2023 •

edited

Loading