Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TieredStorage] Make TieredStorage::write_accounts() thread-safe #35143

Merged
merged 9 commits into from
Feb 18, 2024
36 changes: 23 additions & 13 deletions accounts-db/src/tiered_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ use {
borrow::Borrow,
fs::{self, OpenOptions},
path::{Path, PathBuf},
sync::OnceLock,
sync::{
atomic::{AtomicBool, Ordering},
OnceLock,
},
},
};

Expand All @@ -46,9 +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<TieredStorageReader>,
/// A status flag indicating whether its file has been already written.
already_written: AtomicBool,
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
/// The path to the file that stores accounts.
path: PathBuf,
}

Expand All @@ -72,6 +80,7 @@ impl TieredStorage {
pub fn new_writable(path: impl Into<PathBuf>) -> Self {
Self {
reader: OnceLock::<TieredStorageReader>::new(),
already_written: false.into(),
path: path.into(),
}
}
Expand All @@ -82,6 +91,7 @@ impl TieredStorage {
let path = path.into();
Ok(Self {
reader: TieredStorageReader::new_from_path(&path).map(OnceLock::from)?,
already_written: true.into(),
path,
})
}
Expand All @@ -94,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,
Expand All @@ -109,10 +117,10 @@ impl TieredStorage {
skip: usize,
format: &TieredStorageFormat,
) -> TieredStorageResult<Vec<StoredAccountInfo>> {
if self.is_read_only() {
return Err(TieredStorageError::AttemptToUpdateReadOnly(
self.path.to_path_buf(),
));
let was_written = self.already_written.swap(true, Ordering::AcqRel);

if was_written {
panic!("cannot write same tiered storage file more than once");
}

if format == &HOT_FORMAT {
Expand All @@ -122,16 +130,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;
result
} else {
Err(TieredStorageError::UnknownFormat(self.path.to_path_buf()))
}

Err(TieredStorageError::UnknownFormat(self.path.to_path_buf()))
}

/// Returns the underlying reader of the TieredStorage. None will be
Expand Down Expand Up @@ -258,6 +267,7 @@ mod tests {
}

#[test]
#[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();
Expand Down
Loading