From 69d9f90334fd77f54c7c1ae331e0ffb900fec60c Mon Sep 17 00:00:00 2001 From: Alessandro Passaro Date: Fri, 30 Jun 2023 17:29:44 +0100 Subject: [PATCH] Abort upload on out-of-order writes (#341) * Add fuse test for out-of-order writes Signed-off-by: Alessandro Passaro * Abort upload after an out-of-order write attempt. Signed-off-by: Alessandro Passaro --------- Signed-off-by: Alessandro Passaro --- mountpoint-s3/src/fs.rs | 13 ++--- mountpoint-s3/tests/fs.rs | 12 ++-- mountpoint-s3/tests/fuse_tests/write_test.rs | 61 +++++++++++++++++++- 3 files changed, 71 insertions(+), 15 deletions(-) diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs index 90cc4d45b..1cc318d03 100644 --- a/mountpoint-s3/src/fs.rs +++ b/mountpoint-s3/src/fs.rs @@ -76,15 +76,10 @@ impl UploadState { 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) } } } diff --git a/mountpoint-s3/tests/fs.rs b/mountpoint-s3/tests/fs.rs index 06132699e..e0642a561 100644 --- a/mountpoint-s3/tests/fs.rs +++ b/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); } diff --git a/mountpoint-s3/tests/fuse_tests/write_test.rs b/mountpoint-s3/tests/fuse_tests/write_test.rs index 51d718dbe..1934a1d74 100644 --- a/mountpoint-s3/tests/fuse_tests/write_test.rs +++ b/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(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); +}