From 72fa85d6851169cd6e8d299f11f6d8184f0d64db Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 04:05:46 -0800 Subject: [PATCH 1/9] [TieredStorage] Make TieredStorage::write_accounts() thread-safe --- accounts-db/src/tiered_storage.rs | 59 +++++++++++++++++++------------ 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index f0a23150e2fa70..67ebd64d164c57 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -29,7 +29,10 @@ use { borrow::Borrow, fs::{self, OpenOptions}, path::{Path, PathBuf}, - sync::OnceLock, + sync::{ + atomic::{AtomicBool, Ordering}, + OnceLock, + }, }, }; @@ -49,6 +52,7 @@ pub struct TieredStorageFormat { #[derive(Debug)] pub struct TieredStorage { reader: OnceLock, + read_only: AtomicBool, path: PathBuf, } @@ -72,6 +76,7 @@ impl TieredStorage { pub fn new_writable(path: impl Into) -> Self { Self { reader: OnceLock::::new(), + read_only: false.into(), path: path.into(), } } @@ -82,6 +87,7 @@ impl TieredStorage { let path = path.into(); Ok(Self { reader: TieredStorageReader::new_from_path(&path).map(OnceLock::from)?, + read_only: true.into(), path, }) } @@ -109,29 +115,36 @@ impl TieredStorage { skip: usize, format: &TieredStorageFormat, ) -> TieredStorageResult> { - if self.is_read_only() { - return Err(TieredStorageError::AttemptToUpdateReadOnly( - self.path.to_path_buf(), - )); - } - - if format == &HOT_FORMAT { - let result = { - let writer = HotStorageWriter::new(&self.path)?; - writer.write_accounts(accounts, skip) - }; - - // panic here if self.reader.get() is not None as self.reader can only be - // None since we have passed `is_read_only()` check previously, indicating - // self.reader is not yet set. - self.reader - .set(TieredStorageReader::new_from_path(&self.path)?) - .unwrap(); + let was_readonly = + self.read_only + .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed); + + // If it was not previously readonly, and the current thread has + // successfully updated the read_only flag, then the current + // thread will proceed and writes the input accounts. + if was_readonly == Ok(false) { + if format == &HOT_FORMAT { + let result = { + let writer = HotStorageWriter::new(&self.path)?; + writer.write_accounts(accounts, skip) + }; + + // panic here if self.reader.get() is not None as self.reader can only be + // None since we have passed `is_read_only()` check previously, indicating + // self.reader is not yet set. + self.reader + .set(TieredStorageReader::new_from_path(&self.path)?) + .unwrap(); + + return result; + } - return result; + Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) + } else { + Err(TieredStorageError::AttemptToUpdateReadOnly( + self.path.to_path_buf(), + )) } - - Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) } /// Returns the underlying reader of the TieredStorage. None will be @@ -142,7 +155,7 @@ impl TieredStorage { /// Returns true if the TieredStorage instance is read-only. pub fn is_read_only(&self) -> bool { - self.reader.get().is_some() + self.read_only.load(Ordering::Relaxed) } /// Returns the size of the underlying accounts file. From cdf4ce043ca301f2e75465b6c722749f6b8cf4b7 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 12:55:40 -0800 Subject: [PATCH 2/9] Use Acquired/Released. is_read_only() check doesn't use atomic. --- accounts-db/src/tiered_storage.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 67ebd64d164c57..dec4b4ae0e2d57 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -52,7 +52,7 @@ pub struct TieredStorageFormat { #[derive(Debug)] pub struct TieredStorage { reader: OnceLock, - read_only: AtomicBool, + already_written: AtomicBool, path: PathBuf, } @@ -76,7 +76,7 @@ impl TieredStorage { pub fn new_writable(path: impl Into) -> Self { Self { reader: OnceLock::::new(), - read_only: false.into(), + already_written: false.into(), path: path.into(), } } @@ -87,7 +87,7 @@ impl TieredStorage { let path = path.into(); Ok(Self { reader: TieredStorageReader::new_from_path(&path).map(OnceLock::from)?, - read_only: true.into(), + already_written: true.into(), path, }) } @@ -115,14 +115,14 @@ impl TieredStorage { skip: usize, format: &TieredStorageFormat, ) -> TieredStorageResult> { - let was_readonly = - self.read_only - .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed); + let was_written = + self.already_written + .compare_exchange(false, true, Ordering::Acquired, Ordering::Released); - // If it was not previously readonly, and the current thread has - // successfully updated the read_only flag, then the current + // If it was not previously written, and the current thread has + // successfully updated the already_written flag, then the current // thread will proceed and writes the input accounts. - if was_readonly == Ok(false) { + if was_written == Ok(false) { if format == &HOT_FORMAT { let result = { let writer = HotStorageWriter::new(&self.path)?; @@ -155,7 +155,7 @@ impl TieredStorage { /// Returns true if the TieredStorage instance is read-only. pub fn is_read_only(&self) -> bool { - self.read_only.load(Ordering::Relaxed) + self.reader.get().is_some() } /// Returns the size of the underlying accounts file. From 0b042dc5c3c87488506fe28da42ced539662f342 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 12:56:20 -0800 Subject: [PATCH 3/9] cargo fmt --- accounts-db/src/tiered_storage.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index dec4b4ae0e2d57..7b39067ec9f21c 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -115,9 +115,12 @@ impl TieredStorage { skip: usize, format: &TieredStorageFormat, ) -> TieredStorageResult> { - let was_written = - self.already_written - .compare_exchange(false, true, Ordering::Acquired, Ordering::Released); + let was_written = self.already_written.compare_exchange( + false, + true, + Ordering::Acquired, + Ordering::Released, + ); // If it was not previously written, and the current thread has // successfully updated the already_written flag, then the current From fcd7616bbb49add14781f138f6da31eb69640118 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 12:59:00 -0800 Subject: [PATCH 4/9] Correct the memory order --- accounts-db/src/tiered_storage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 7b39067ec9f21c..9a8bab6abb5db8 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -118,8 +118,8 @@ impl TieredStorage { let was_written = self.already_written.compare_exchange( false, true, - Ordering::Acquired, - Ordering::Released, + Ordering::AcqRel, + Ordering::Relaxed, ); // If it was not previously written, and the current thread has From 971cd7fcf70eeca44e794f747d2fc5f4d42f7f17 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 13:11:57 -0800 Subject: [PATCH 5/9] cargo fmt --- accounts-db/src/tiered_storage.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 9a8bab6abb5db8..7c59b8a2ff6df7 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -115,12 +115,9 @@ impl TieredStorage { skip: usize, format: &TieredStorageFormat, ) -> TieredStorageResult> { - let was_written = self.already_written.compare_exchange( - false, - true, - Ordering::AcqRel, - Ordering::Relaxed, - ); + let was_written = + self.already_written + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Relaxed); // If it was not previously written, and the current thread has // successfully updated the already_written flag, then the current From 5ab2ac7921dbbf8ab5360afe324d3ec21997640e Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 8 Feb 2024 17:08:21 -0800 Subject: [PATCH 6/9] Use swap instead for already_written. Update its test to expect panic. --- accounts-db/src/tiered_storage.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 7c59b8a2ff6df7..4821cfeae29f50 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -115,14 +115,12 @@ impl TieredStorage { skip: usize, format: &TieredStorageFormat, ) -> TieredStorageResult> { - let was_written = - self.already_written - .compare_exchange(false, true, Ordering::AcqRel, Ordering::Relaxed); + let was_written = self.already_written.swap(true, Ordering::AcqRel); // If it was not previously written, and the current thread has // successfully updated the already_written flag, then the current // thread will proceed and writes the input accounts. - if was_written == Ok(false) { + if !was_written { if format == &HOT_FORMAT { let result = { let writer = HotStorageWriter::new(&self.path)?; @@ -141,9 +139,7 @@ impl TieredStorage { Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) } else { - Err(TieredStorageError::AttemptToUpdateReadOnly( - self.path.to_path_buf(), - )) + panic!("TieredStorage::write_accounts() is expected to be invoked once per file."); } } @@ -271,6 +267,9 @@ mod tests { } #[test] + #[should_panic( + expected = "TieredStorage::write_accounts() is expected to be invoked once per file" + )] fn test_write_accounts_twice() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = tempdir().unwrap(); From c09e12e8fdd46995c2fd1b09fc7c408b45813f24 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Fri, 9 Feb 2024 16:33:45 -0800 Subject: [PATCH 7/9] * Use handle-all-the-branches style for write_accounts(). Improve code comments and add assertions. --- accounts-db/src/tiered_storage.rs | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 4821cfeae29f50..c21ff9804f6cd1 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -49,10 +49,14 @@ pub struct TieredStorageFormat { pub account_block_format: AccountBlockFormat, } +/// The implementation of AccountsFile for tiered-storage. #[derive(Debug)] pub struct TieredStorage { + /// The internal reader instance for its accounts file. reader: OnceLock, + /// A status flag indicating whether its file has been already written. already_written: AtomicBool, + /// The path to the file that stores accounts. path: PathBuf, } @@ -100,9 +104,7 @@ impl TieredStorage { /// Writes the specified accounts into this TieredStorage. /// /// Note that this function can only be called once per a TieredStorage - /// instance. TieredStorageError::AttemptToUpdateReadOnly will be returned - /// if this function is invoked more than once on the same TieredStorage - /// instance. + /// instance. Otherwise, it will trigger panic. pub fn write_accounts< 'a, 'b, @@ -117,10 +119,9 @@ impl TieredStorage { ) -> TieredStorageResult> { let was_written = self.already_written.swap(true, Ordering::AcqRel); - // If it was not previously written, and the current thread has - // successfully updated the already_written flag, then the current - // thread will proceed and writes the input accounts. - if !was_written { + if was_written { + panic!("cannot write same tiered storage file more than once"); + } else { if format == &HOT_FORMAT { let result = { let writer = HotStorageWriter::new(&self.path)?; @@ -128,18 +129,17 @@ impl TieredStorage { }; // panic here if self.reader.get() is not None as self.reader can only be - // None since we have passed `is_read_only()` check previously, indicating - // self.reader is not yet set. + // None since a false-value `was_written` indicates the accounts file has + // not been written previously, implying is_read_only() was also false. + debug_assert!(!self.is_read_only()); self.reader .set(TieredStorageReader::new_from_path(&self.path)?) .unwrap(); return result; + } else { + Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) } - - Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) - } else { - panic!("TieredStorage::write_accounts() is expected to be invoked once per file."); } } @@ -267,9 +267,7 @@ mod tests { } #[test] - #[should_panic( - expected = "TieredStorage::write_accounts() is expected to be invoked once per file" - )] + #[should_panic(expected = "cannot write same tiered storage file more than once")] fn test_write_accounts_twice() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = tempdir().unwrap(); From 33540842b07268954fbefd511e70d946bbb7c409 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Fri, 9 Feb 2024 18:11:08 -0800 Subject: [PATCH 8/9] fix cargo --- accounts-db/src/tiered_storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index c21ff9804f6cd1..0cd779a00dbf13 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -136,7 +136,7 @@ impl TieredStorage { .set(TieredStorageReader::new_from_path(&self.path)?) .unwrap(); - return result; + result } else { Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) } From 886182e96f7c0180b37980617e3e8ef6273cd347 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Fri, 9 Feb 2024 18:28:50 -0800 Subject: [PATCH 9/9] make cargo happy again --- accounts-db/src/tiered_storage.rs | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index 0cd779a00dbf13..6a5bbc9a2cc862 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -121,25 +121,25 @@ impl TieredStorage { if was_written { panic!("cannot write same tiered storage file more than once"); + } + + if format == &HOT_FORMAT { + let result = { + let writer = HotStorageWriter::new(&self.path)?; + writer.write_accounts(accounts, skip) + }; + + // panic here if self.reader.get() is not None as self.reader can only be + // None since a false-value `was_written` indicates the accounts file has + // not been written previously, implying is_read_only() was also false. + debug_assert!(!self.is_read_only()); + self.reader + .set(TieredStorageReader::new_from_path(&self.path)?) + .unwrap(); + + result } else { - if format == &HOT_FORMAT { - let result = { - let writer = HotStorageWriter::new(&self.path)?; - writer.write_accounts(accounts, skip) - }; - - // panic here if self.reader.get() is not None as self.reader can only be - // None since a false-value `was_written` indicates the accounts file has - // not been written previously, implying is_read_only() was also false. - debug_assert!(!self.is_read_only()); - self.reader - .set(TieredStorageReader::new_from_path(&self.path)?) - .unwrap(); - - result - } else { - Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) - } + Err(TieredStorageError::UnknownFormat(self.path.to_path_buf())) } }