Skip to content

Commit

Permalink
[TieredStorage] TieredStorageFile -> TieredReadonlyFile and TieredWri…
Browse files Browse the repository at this point in the history
…tableFIle (#260)

#### Problem
TieredStorageFile struct currently offers new_readonly() and new_writable()
to allow both read and write work-load to share the same struct.  However,
as we need the writer to use BufWriter to improve performance as well as
enable Hasher on writes.  There is a need to refactor TieredStorageFile to
split its usage for read-only and writable.

#### Summary of Changes
Refactor TieredStorageFile to TieredReadonlyFIle and TieredWritableFile.

#### Test Plan
Existing tiered-storage tests.
  • Loading branch information
yhchiang-sol authored and willhickey committed Mar 20, 2024
1 parent f6c22e9 commit 62c458e
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 61 deletions.
84 changes: 53 additions & 31 deletions accounts-db/src/tiered_storage/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ use {
};

#[derive(Debug)]
pub struct TieredStorageFile(pub File);
pub struct TieredReadableFile(pub File);

impl TieredStorageFile {
pub fn new_readonly(file_path: impl AsRef<Path>) -> Self {
impl TieredReadableFile {
pub fn new(file_path: impl AsRef<Path>) -> Self {
Self(
OpenOptions::new()
.read(true)
Expand All @@ -36,30 +36,6 @@ impl TieredStorageFile {
))
}

/// Writes `value` to the file.
///
/// `value` must be plain ol' data.
pub fn write_pod<T: NoUninit>(&self, value: &T) -> IoResult<usize> {
// SAFETY: Since T is NoUninit, it does not contain any uninitialized bytes.
unsafe { self.write_type(value) }
}

/// Writes `value` to the file.
///
/// Prefer `write_pod` when possible, because `write_value` may cause
/// undefined behavior if `value` contains uninitialized bytes.
///
/// # Safety
///
/// Caller must ensure casting T to bytes is safe.
/// Refer to the Safety sections in std::slice::from_raw_parts()
/// and bytemuck's Pod and NoUninit for more information.
pub unsafe fn write_type<T>(&self, value: &T) -> IoResult<usize> {
let ptr = value as *const _ as *const u8;
let bytes = unsafe { std::slice::from_raw_parts(ptr, mem::size_of::<T>()) };
self.write_bytes(bytes)
}

/// Reads a value of type `T` from the file.
///
/// Type T must be plain ol' data.
Expand Down Expand Up @@ -95,13 +71,59 @@ impl TieredStorageFile {
(&self.0).seek(SeekFrom::End(offset))
}

pub fn read_bytes(&self, buffer: &mut [u8]) -> IoResult<()> {
(&self.0).read_exact(buffer)
}
}

#[derive(Debug)]
pub struct TieredWritableFile(pub File);

impl TieredWritableFile {
pub fn new(file_path: impl AsRef<Path>) -> IoResult<Self> {
Ok(Self(
OpenOptions::new()
.create_new(true)
.write(true)
.open(file_path)?,
))
}

/// Writes `value` to the file.
///
/// `value` must be plain ol' data.
pub fn write_pod<T: NoUninit>(&self, value: &T) -> IoResult<usize> {
// SAFETY: Since T is NoUninit, it does not contain any uninitialized bytes.
unsafe { self.write_type(value) }
}

/// Writes `value` to the file.
///
/// Prefer `write_pod` when possible, because `write_value` may cause
/// undefined behavior if `value` contains uninitialized bytes.
///
/// # Safety
///
/// Caller must ensure casting T to bytes is safe.
/// Refer to the Safety sections in std::slice::from_raw_parts()
/// and bytemuck's Pod and NoUninit for more information.
pub unsafe fn write_type<T>(&self, value: &T) -> IoResult<usize> {
let ptr = value as *const _ as *const u8;
let bytes = unsafe { std::slice::from_raw_parts(ptr, mem::size_of::<T>()) };
self.write_bytes(bytes)
}

pub fn seek(&self, offset: u64) -> IoResult<u64> {
(&self.0).seek(SeekFrom::Start(offset))
}

pub fn seek_from_end(&self, offset: i64) -> IoResult<u64> {
(&self.0).seek(SeekFrom::End(offset))
}

pub fn write_bytes(&self, bytes: &[u8]) -> IoResult<usize> {
(&self.0).write_all(bytes)?;

Ok(bytes.len())
}

pub fn read_bytes(&self, buffer: &mut [u8]) -> IoResult<()> {
(&self.0).read_exact(buffer)
}
}
12 changes: 6 additions & 6 deletions accounts-db/src/tiered_storage/footer.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use {
crate::tiered_storage::{
error::TieredStorageError,
file::TieredStorageFile,
file::{TieredReadableFile, TieredWritableFile},
index::IndexBlockFormat,
mmap_utils::{get_pod, get_type},
owners::OwnersBlockFormat,
Expand Down Expand Up @@ -186,19 +186,19 @@ impl Default for TieredStorageFooter {

impl TieredStorageFooter {
pub fn new_from_path(path: impl AsRef<Path>) -> TieredStorageResult<Self> {
let file = TieredStorageFile::new_readonly(path);
let file = TieredReadableFile::new(path);
Self::new_from_footer_block(&file)
}

pub fn write_footer_block(&self, file: &TieredStorageFile) -> TieredStorageResult<()> {
pub fn write_footer_block(&self, file: &TieredWritableFile) -> TieredStorageResult<()> {
// SAFETY: The footer does not contain any uninitialized bytes.
unsafe { file.write_type(self)? };
file.write_pod(&TieredStorageMagicNumber::default())?;

Ok(())
}

pub fn new_from_footer_block(file: &TieredStorageFile) -> TieredStorageResult<Self> {
pub fn new_from_footer_block(file: &TieredReadableFile) -> TieredStorageResult<Self> {
file.seek_from_end(-(FOOTER_TAIL_SIZE as i64))?;

let mut footer_version: u64 = 0;
Expand Down Expand Up @@ -326,7 +326,7 @@ mod tests {
use {
super::*,
crate::{
append_vec::test_utils::get_append_vec_path, tiered_storage::file::TieredStorageFile,
append_vec::test_utils::get_append_vec_path, tiered_storage::file::TieredWritableFile,
},
memoffset::offset_of,
solana_sdk::hash::Hash,
Expand Down Expand Up @@ -356,7 +356,7 @@ mod tests {

// Persist the expected footer.
{
let file = TieredStorageFile::new_writable(&path.path).unwrap();
let file = TieredWritableFile::new(&path.path).unwrap();
expected_footer.write_footer_block(&file).unwrap();
}

Expand Down
24 changes: 12 additions & 12 deletions accounts-db/src/tiered_storage/hot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use {
accounts_hash::AccountHash,
tiered_storage::{
byte_block,
file::TieredStorageFile,
file::TieredWritableFile,
footer::{AccountBlockFormat, AccountMetaFormat, TieredStorageFooter},
index::{AccountIndexWriterEntry, AccountOffset, IndexBlockFormat, IndexOffset},
meta::{AccountMetaFlags, AccountMetaOptionalFields, TieredAccountMeta},
Expand Down Expand Up @@ -542,7 +542,7 @@ impl HotStorageReader {
}

fn write_optional_fields(
file: &TieredStorageFile,
file: &TieredWritableFile,
opt_fields: &AccountMetaOptionalFields,
) -> TieredStorageResult<usize> {
let mut size = 0;
Expand All @@ -558,14 +558,14 @@ fn write_optional_fields(
/// The writer that creates a hot accounts file.
#[derive(Debug)]
pub struct HotStorageWriter {
storage: TieredStorageFile,
storage: TieredWritableFile,
}

impl HotStorageWriter {
/// Create a new HotStorageWriter with the specified path.
pub fn new(file_path: impl AsRef<Path>) -> TieredStorageResult<Self> {
Ok(Self {
storage: TieredStorageFile::new_writable(file_path)?,
storage: TieredWritableFile::new(file_path)?,
})
}

Expand Down Expand Up @@ -706,7 +706,7 @@ pub mod tests {
super::*,
crate::tiered_storage::{
byte_block::ByteBlockWriter,
file::TieredStorageFile,
file::TieredWritableFile,
footer::{AccountBlockFormat, AccountMetaFormat, TieredStorageFooter, FOOTER_SIZE},
hot::{HotAccountMeta, HotStorageReader},
index::{AccountIndexWriterEntry, IndexBlockFormat, IndexOffset},
Expand Down Expand Up @@ -892,7 +892,7 @@ pub mod tests {
};

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
expected_footer.write_footer_block(&file).unwrap();
}

Expand Down Expand Up @@ -928,7 +928,7 @@ pub mod tests {
..TieredStorageFooter::default()
};
{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
let mut current_offset = 0;

account_offsets = hot_account_metas
Expand Down Expand Up @@ -971,7 +971,7 @@ pub mod tests {
};

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
footer.write_footer_block(&file).unwrap();
}

Expand Down Expand Up @@ -1016,7 +1016,7 @@ pub mod tests {
..TieredStorageFooter::default()
};
{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();

let cursor = footer
.index_block_format
Expand Down Expand Up @@ -1059,7 +1059,7 @@ pub mod tests {
};

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();

let mut owners_table = OwnersTable::default();
addresses.iter().for_each(|owner_address| {
Expand Down Expand Up @@ -1118,7 +1118,7 @@ pub mod tests {
let account_offsets: Vec<_>;

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
let mut current_offset = 0;

account_offsets = hot_account_metas
Expand Down Expand Up @@ -1237,7 +1237,7 @@ pub mod tests {
};

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
let mut current_offset = 0;

// write accounts blocks
Expand Down
16 changes: 8 additions & 8 deletions accounts-db/src/tiered_storage/index.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use {
crate::tiered_storage::{
file::TieredStorageFile, footer::TieredStorageFooter, mmap_utils::get_pod,
file::TieredWritableFile, footer::TieredStorageFooter, mmap_utils::get_pod,
TieredStorageResult,
},
bytemuck::{Pod, Zeroable},
Expand Down Expand Up @@ -59,7 +59,7 @@ impl IndexBlockFormat {
/// the total number of bytes written.
pub fn write_index_block(
&self,
file: &TieredStorageFile,
file: &TieredWritableFile,
index_entries: &[AccountIndexWriterEntry<impl AccountOffset>],
) -> TieredStorageResult<usize> {
match self {
Expand Down Expand Up @@ -147,7 +147,7 @@ mod tests {
use {
super::*,
crate::tiered_storage::{
file::TieredStorageFile,
file::TieredWritableFile,
hot::{HotAccountOffset, HOT_ACCOUNT_ALIGNMENT},
},
memmap2::MmapOptions,
Expand Down Expand Up @@ -181,7 +181,7 @@ mod tests {
.collect();

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
let indexer = IndexBlockFormat::AddressesThenOffsets;
let cursor = indexer.write_index_block(&file, &index_entries).unwrap();
footer.owners_block_offset = cursor as u64;
Expand Down Expand Up @@ -223,7 +223,7 @@ mod tests {
{
// we only write a footer here as the test should hit an assert
// failure before it actually reads the file.
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
footer.write_footer_block(&file).unwrap();
}

Expand Down Expand Up @@ -259,7 +259,7 @@ mod tests {
{
// we only write a footer here as the test should hit an assert
// failure before it actually reads the file.
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
footer.write_footer_block(&file).unwrap();
}

Expand Down Expand Up @@ -294,7 +294,7 @@ mod tests {
{
// we only write a footer here as the test should hit an assert
// failure before we actually read the file.
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
footer.write_footer_block(&file).unwrap();
}

Expand Down Expand Up @@ -334,7 +334,7 @@ mod tests {
{
// we only write a footer here as the test should hit an assert
// failure before we actually read the file.
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();
footer.write_footer_block(&file).unwrap();
}

Expand Down
8 changes: 4 additions & 4 deletions accounts-db/src/tiered_storage/owners.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use {
crate::tiered_storage::{
file::TieredStorageFile, footer::TieredStorageFooter, mmap_utils::get_pod,
file::TieredWritableFile, footer::TieredStorageFooter, mmap_utils::get_pod,
TieredStorageResult,
},
indexmap::set::IndexSet,
Expand Down Expand Up @@ -47,7 +47,7 @@ impl OwnersBlockFormat {
/// Persists the provided owners' addresses into the specified file.
pub fn write_owners_block(
&self,
file: &TieredStorageFile,
file: &TieredWritableFile,
owners_table: &OwnersTable,
) -> TieredStorageResult<usize> {
match self {
Expand Down Expand Up @@ -116,7 +116,7 @@ impl<'a> OwnersTable<'a> {
#[cfg(test)]
mod tests {
use {
super::*, crate::tiered_storage::file::TieredStorageFile, memmap2::MmapOptions,
super::*, crate::tiered_storage::file::TieredWritableFile, memmap2::MmapOptions,
std::fs::OpenOptions, tempfile::TempDir,
};

Expand All @@ -139,7 +139,7 @@ mod tests {
};

{
let file = TieredStorageFile::new_writable(&path).unwrap();
let file = TieredWritableFile::new(&path).unwrap();

let mut owners_table = OwnersTable::default();
addresses.iter().for_each(|owner_address| {
Expand Down

0 comments on commit 62c458e

Please sign in to comment.