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] TieredStorageFile -> TieredReadonlyFile and TieredWritableFIle #260

Merged
merged 1 commit into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The name of the struct is now TieredReadableFile, but the PR title and description both use the incorrect TieredReadonlyFile. It would be preferable for all of these to remain in-sync and correct. It's now too late for the commit message, since the PR has been merged. Please consider this for future PRs.


impl TieredStorageFile {
pub fn new_readonly(file_path: impl AsRef<Path>) -> Self {
impl TieredReadableFile {
pub fn new(file_path: impl AsRef<Path>) -> Self {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github won't let me comment on line 30, which is my intent

I see below there's a TieredReableFile::new_writable() method. In what cases do we need a reader to create a new writable file?

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))
}
Comment on lines +116 to +122

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad these methods are duplicated. Do both the Readable and Writable have calls to these methods? I was under the impression that Writeable does everything in one pass, from top to bottom, and thus would not need to seek.


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
Loading