Abort upload on out-of-order writes (#341)

* Add fuse test for out-of-order writes

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Abort upload after an out-of-order write attempt.

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

mountpoint-s3/src/fs.rs

@@ -76,15 +76,10 @@ impl<Client: ObjectClient> UploadState<Client> {
             Ok(len) => Ok(len as u32),
             Err(e) => {
                 error!("write failed: {e}");
-                match e {
-                    UploadWriteError::PutRequestFailed(_) | UploadWriteError::ObjectTooBig { .. } => {
-                        // Abort the request.
-                        let ret: libc::c_int = e.into();
-                        *self = Self::Failed(ret);
-                        Err(ret)
-                    }
-                    UploadWriteError::OutOfOrderWrite { .. } => Err(e.into()),
-                }
+                // Abort the request.
+                let ret: libc::c_int = e.into();
+                *self = Self::Failed(ret);
+                Err(ret)
             }
         }
     }

mountpoint-s3/tests/fs.rs

@@ -350,8 +350,10 @@ async fn test_sequential_write(write_size: usize) {
     fs.release(file_ino, fh, 0, None, true).await.unwrap();
 }
 
+#[test_case(-27; "earlier offset")]
+#[test_case(28; "later offset")]
 #[tokio::test]
-async fn test_unordered_write_fails() {
+async fn test_unordered_write_fails(offset: i64) {
     const BUCKET_NAME: &str = "test_unordered_write_fails";
 
     let (_client, fs) = make_test_filesystem(BUCKET_NAME, &Default::default(), Default::default());
@@ -374,15 +376,15 @@ async fn test_unordered_write_fails() {
     assert_eq!(written, 27);
 
     let err = fs
-        .write(file_ino, fh, 0, &[0xaa; 27], 0, 0, None)
+        .write(file_ino, fh, written as i64 + offset, &[0xaa; 27], 0, 0, None)
         .await
-        .expect_err("writes to earlier offsets should fail");
+        .expect_err("writes to out-of-order offsets should fail");
     assert_eq!(err, libc::EINVAL);
 
     let err = fs
-        .write(file_ino, fh, 55, &[0xaa; 27], 0, 0, None)
+        .write(file_ino, fh, written as i64, &[0xaa; 27], 0, 0, None)
         .await
-        .expect_err("writes to later offsets should fail");
+        .expect_err("any write after an error should fail");
     assert_eq!(err, libc::EINVAL);
 }
 

mountpoint-s3/tests/fuse_tests/write_test.rs

@@ -1,5 +1,5 @@
 use std::fs::{metadata, read, read_dir, File};
-use std::io::{ErrorKind, Read, Write};
+use std::io::{ErrorKind, Read, Seek, Write};
 use std::os::unix::prelude::OpenOptionsExt;
 use std::path::Path;
 use std::time::Duration;
@@ -328,3 +328,62 @@ where
 fn write_too_big_test_mock(write_size: usize) {
     write_too_big_test(crate::fuse_tests::mock_session::new, write_size);
 }
+
+fn out_of_order_write_test<F>(creator_fn: F, offset: i64)
+where
+    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
+{
+    const OBJECT_SIZE: usize = 32;
+    const KEY: &str = "new.txt";
+
+    let (mount_point, _session, _test_client) = creator_fn("out_of_order_write_test", Default::default());
+
+    let path = mount_point.path().join(KEY);
+
+    let mut f = open_for_write(&path, false).unwrap();
+
+    // The file is visible with size 0 as soon as we open it for write
+    let m = metadata(&path).unwrap();
+    assert_eq!(m.len(), 0);
+
+    let mut rng = ChaCha20Rng::seed_from_u64(0x12345678 + OBJECT_SIZE as u64);
+    let mut body = vec![0u8; OBJECT_SIZE];
+    rng.fill(&mut body[..]);
+
+    f.write_all(&body).unwrap();
+
+    // Attempt to write out-of-order.
+    f.seek(std::io::SeekFrom::Current(offset)).unwrap();
+    let err = f.write_all(&body).expect_err("out of order write should fail");
+    assert_eq!(err.raw_os_error(), Some(libc::EINVAL));
+
+    // Seek where we left off and attempt to write.
+    f.seek(std::io::SeekFrom::Start((OBJECT_SIZE) as u64)).unwrap();
+    let err = f.write_all(&body).expect_err("writes after an error should fail");
+    assert_eq!(err.raw_os_error(), Some(libc::EINVAL));
+
+    drop(f);
+
+    // The kernel doesn't guarantee to flush the data as soon as the file is closed. Currently,
+    // the file won't be visible on the file system until it's flushed to S3, and so trying to stat
+    // the file will fail.
+    // TODO we can remove this when we implement fsync, or change it when we make files visible
+    // during writes
+    std::thread::sleep(Duration::from_secs(5));
+
+    let err = metadata(&path).expect_err("upload shouldn't have succeeded");
+    assert_eq!(err.raw_os_error(), Some(libc::ENOENT));
+}
+
+#[cfg(feature = "s3_tests")]
+#[test_case(-1; "earlier offset")]
+#[test_case(1; "later offset")]
+fn out_of_order_write_test_s3(offset: i64) {
+    out_of_order_write_test(crate::fuse_tests::s3_session::new, offset);
+}
+
+#[test_case(-1; "earlier offset")]
+#[test_case(1; "later offset")]
+fn out_of_order_write_test_mock(offset: i64) {
+    out_of_order_write_test(crate::fuse_tests::mock_session::new, offset);
+}