[TieredStorage] TieredStorageFile -> TieredReadonlyFile and TieredWritableFIle (pyth-network#260)

yhchiang-sol · web-flow · commit 644120968292 · 2024-03-18T12:24:19.000-07:00
#### 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.
diff --git a/accounts-db/src/tiered_storage/file.rs b/accounts-db/src/tiered_storage/file.rs
@@ -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)
@@ -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.
@@ -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)
-    }
 }
diff --git a/accounts-db/src/tiered_storage/footer.rs b/accounts-db/src/tiered_storage/footer.rs
@@ -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,
@@ -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;
@@ -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,
@@ -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();
         }
 
diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs
@@ -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},
@@ -542,7 +542,7 @@ impl HotStorageReader {
 }
 
 fn write_optional_fields(
-    file: &TieredStorageFile,
+    file: &TieredWritableFile,
     opt_fields: &AccountMetaOptionalFields,
 ) -> TieredStorageResult<usize> {
     let mut size = 0;
@@ -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)?,
         })
     }
 
@@ -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},
@@ -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();
         }
 
@@ -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
@@ -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();
         }
 
@@ -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
@@ -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| {
@@ -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
@@ -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
diff --git a/accounts-db/src/tiered_storage/index.rs b/accounts-db/src/tiered_storage/index.rs
@@ -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},
@@ -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 {
@@ -147,7 +147,7 @@ mod tests {
     use {
         super::*,
         crate::tiered_storage::{
-            file::TieredStorageFile,
+            file::TieredWritableFile,
             hot::{HotAccountOffset, HOT_ACCOUNT_ALIGNMENT},
         },
         memmap2::MmapOptions,
@@ -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;
@@ -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();
         }
 
@@ -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();
         }
 
@@ -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();
         }
 
@@ -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();
         }
 
diff --git a/accounts-db/src/tiered_storage/owners.rs b/accounts-db/src/tiered_storage/owners.rs
@@ -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,
@@ -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 {
@@ -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,
     };
 
@@ -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| {
