From 5326dfb21c4d288928b7f9edb9aac9561fc54e45 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Sat, 18 Nov 2023 15:01:49 +0800 Subject: [PATCH 1/5] Fix ObjectStore.LocalFileSystem.put_opts for blobfuse --- object_store/src/local.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index dd71d9ec1219..aaeb03ffa951 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -341,10 +341,14 @@ impl ObjectStore for LocalFileSystem { let err = match file.write_all(&bytes) { Ok(_) => match opts.mode { - PutMode::Overwrite => match std::fs::rename(&staging_path, &path) { - Ok(_) => None, - Err(source) => Some(Error::UnableToRenameFile { source }), - }, + PutMode::Overwrite => { + file.sync_all() + .map_err(|e| Error::UnableToCopyDataToFile { source: e })?; + match std::fs::rename(&staging_path, &path) { + Ok(_) => None, + Err(source) => Some(Error::UnableToRenameFile { source }), + } + } PutMode::Create => match std::fs::hard_link(&staging_path, &path) { Ok(_) => { let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup From f107dc01f13ad4d0ad1f6e97db07d773c3e79e01 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Sat, 18 Nov 2023 16:13:18 +0800 Subject: [PATCH 2/5] Fix ObjectStore.LocalFileSystem.put_opts for blobfuse --- object_store/src/lib.rs | 2 ++ object_store/src/local.rs | 21 ++++++++++++--------- object_store/src/memory.rs | 1 + 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/object_store/src/lib.rs b/object_store/src/lib.rs index 2d1d549f9e54..4244a98a9717 100644 --- a/object_store/src/lib.rs +++ b/object_store/src/lib.rs @@ -1064,6 +1064,8 @@ pub enum PutMode { /// Perform an atomic write operation if the current version of the object matches the /// provided [`UpdateVersion`], returning [`Error::Precondition`] otherwise Update(UpdateVersion), + /// Only for Mounted path, drop the file before renaming so that the file will upload. + OverwriteUnsafe, } /// Uniquely identifies a version of an object to update diff --git a/object_store/src/local.rs b/object_store/src/local.rs index aaeb03ffa951..b54552878b96 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -341,14 +341,10 @@ impl ObjectStore for LocalFileSystem { let err = match file.write_all(&bytes) { Ok(_) => match opts.mode { - PutMode::Overwrite => { - file.sync_all() - .map_err(|e| Error::UnableToCopyDataToFile { source: e })?; - match std::fs::rename(&staging_path, &path) { - Ok(_) => None, - Err(source) => Some(Error::UnableToRenameFile { source }), - } - } + PutMode::Overwrite => match std::fs::rename(&staging_path, &path) { + Ok(_) => None, + Err(source) => Some(Error::UnableToRenameFile { source }), + }, PutMode::Create => match std::fs::hard_link(&staging_path, &path) { Ok(_) => { let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup @@ -363,6 +359,13 @@ impl ObjectStore for LocalFileSystem { }, }, PutMode::Update(_) => unreachable!(), + PutMode::OverwriteUnsafe => { + std::mem::drop(file); + match std::fs::rename(&staging_path, &path) { + Ok(_) => None, + Err(source) => Some(Error::UnableToRenameFile { source }), + } + } }, Err(source) => Some(Error::UnableToCopyDataToFile { source }), }; @@ -372,7 +375,7 @@ impl ObjectStore for LocalFileSystem { return Err(err.into()); } - let metadata = file.metadata().map_err(|e| Error::Metadata { + let metadata = metadata(&path).map_err(|e| Error::Metadata { source: e.into(), path: path.to_string_lossy().to_string(), })?; diff --git a/object_store/src/memory.rs b/object_store/src/memory.rs index 382300123846..4ce9ab6b8252 100644 --- a/object_store/src/memory.rs +++ b/object_store/src/memory.rs @@ -177,6 +177,7 @@ impl ObjectStore for InMemory { PutMode::Overwrite => storage.overwrite(location, entry), PutMode::Create => storage.create(location, entry)?, PutMode::Update(v) => storage.update(location, v, entry)?, + PutMode::OverwriteUnsafe => storage.overwrite(location, entry), } storage.next_etag += 1; From 2fffc3789c253e4ddc5b017f1072564227759de4 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Sat, 18 Nov 2023 21:32:54 +0800 Subject: [PATCH 3/5] fix comment --- object_store/src/lib.rs | 2 -- object_store/src/local.rs | 18 +++++++----------- object_store/src/memory.rs | 1 - 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/object_store/src/lib.rs b/object_store/src/lib.rs index 4244a98a9717..2d1d549f9e54 100644 --- a/object_store/src/lib.rs +++ b/object_store/src/lib.rs @@ -1064,8 +1064,6 @@ pub enum PutMode { /// Perform an atomic write operation if the current version of the object matches the /// provided [`UpdateVersion`], returning [`Error::Precondition`] otherwise Update(UpdateVersion), - /// Only for Mounted path, drop the file before renaming so that the file will upload. - OverwriteUnsafe, } /// Uniquely identifies a version of an object to update diff --git a/object_store/src/local.rs b/object_store/src/local.rs index b54552878b96..648f486e6916 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -341,10 +341,13 @@ impl ObjectStore for LocalFileSystem { let err = match file.write_all(&bytes) { Ok(_) => match opts.mode { - PutMode::Overwrite => match std::fs::rename(&staging_path, &path) { - Ok(_) => None, - Err(source) => Some(Error::UnableToRenameFile { source }), - }, + PutMode::Overwrite => { + std::mem::drop(file); + match std::fs::rename(&staging_path, &path) { + Ok(_) => None, + Err(source) => Some(Error::UnableToRenameFile { source }), + } + } PutMode::Create => match std::fs::hard_link(&staging_path, &path) { Ok(_) => { let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup @@ -359,13 +362,6 @@ impl ObjectStore for LocalFileSystem { }, }, PutMode::Update(_) => unreachable!(), - PutMode::OverwriteUnsafe => { - std::mem::drop(file); - match std::fs::rename(&staging_path, &path) { - Ok(_) => None, - Err(source) => Some(Error::UnableToRenameFile { source }), - } - } }, Err(source) => Some(Error::UnableToCopyDataToFile { source }), }; diff --git a/object_store/src/memory.rs b/object_store/src/memory.rs index 4ce9ab6b8252..382300123846 100644 --- a/object_store/src/memory.rs +++ b/object_store/src/memory.rs @@ -177,7 +177,6 @@ impl ObjectStore for InMemory { PutMode::Overwrite => storage.overwrite(location, entry), PutMode::Create => storage.create(location, entry)?, PutMode::Update(v) => storage.update(location, v, entry)?, - PutMode::OverwriteUnsafe => storage.overwrite(location, entry), } storage.next_etag += 1; From f25ace390eac937a955aeac72787dd08c4def602 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Sun, 19 Nov 2023 18:22:16 +0800 Subject: [PATCH 4/5] fix race condition --- object_store/src/local.rs | 57 ++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 648f486e6916..927101b61109 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -338,31 +338,39 @@ impl ObjectStore for LocalFileSystem { maybe_spawn_blocking(move || { let (mut file, suffix) = new_staged_upload(&path)?; let staging_path = staged_upload_path(&path, &suffix); + let mut e_tag = None; let err = match file.write_all(&bytes) { - Ok(_) => match opts.mode { - PutMode::Overwrite => { - std::mem::drop(file); - match std::fs::rename(&staging_path, &path) { - Ok(_) => None, - Err(source) => Some(Error::UnableToRenameFile { source }), - } - } - PutMode::Create => match std::fs::hard_link(&staging_path, &path) { - Ok(_) => { - let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup - None + Ok(_) => { + let metadata = file.metadata().map_err(|e| Error::Metadata { + source: e.into(), + path: path.to_string_lossy().to_string(), + })?; + e_tag = Some(get_etag(&metadata)); + match opts.mode { + PutMode::Overwrite => { + std::mem::drop(file); + match std::fs::rename(&staging_path, &path) { + Ok(_) => None, + Err(source) => Some(Error::UnableToRenameFile { source }), + } } - Err(source) => match source.kind() { - ErrorKind::AlreadyExists => Some(Error::AlreadyExists { - path: path.to_str().unwrap().to_string(), - source, - }), - _ => Some(Error::UnableToRenameFile { source }), + PutMode::Create => match std::fs::hard_link(&staging_path, &path) { + Ok(_) => { + let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup + None + } + Err(source) => match source.kind() { + ErrorKind::AlreadyExists => Some(Error::AlreadyExists { + path: path.to_str().unwrap().to_string(), + source, + }), + _ => Some(Error::UnableToRenameFile { source }), + }, }, - }, - PutMode::Update(_) => unreachable!(), - }, + PutMode::Update(_) => unreachable!(), + } + } Err(source) => Some(Error::UnableToCopyDataToFile { source }), }; @@ -371,13 +379,8 @@ impl ObjectStore for LocalFileSystem { return Err(err.into()); } - let metadata = metadata(&path).map_err(|e| Error::Metadata { - source: e.into(), - path: path.to_string_lossy().to_string(), - })?; - Ok(PutResult { - e_tag: Some(get_etag(&metadata)), + e_tag, version: None, }) }) From f5677bf8532249b342bd145f2029ac0f586b9e87 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 20 Nov 2023 10:09:01 +0800 Subject: [PATCH 5/5] add comment --- object_store/src/local.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 927101b61109..71b96f058c79 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -349,6 +349,8 @@ impl ObjectStore for LocalFileSystem { e_tag = Some(get_etag(&metadata)); match opts.mode { PutMode::Overwrite => { + // For some fuse types of file systems, the file must be closed first + // to trigger the upload operation, and then renamed, such as Blobfuse std::mem::drop(file); match std::fs::rename(&staging_path, &path) { Ok(_) => None,