v1.17: Cleans up stale accounts hash cache files (backport of #34933)…

… (#34937) * Cleans up stale accounts hash cache files (#34933) (cherry picked from commit 5898b9a) # Conflicts: # accounts-db/src/cache_hash_data.rs * resolves merge conflicts --------- Co-authored-by: Brooks <[email protected]>
jito-foundation · Jan 24, 2024 · dd28fd5 · dd28fd5
1 parent de8415c
commit dd28fd5
Show file tree

Hide file tree

Showing 2 changed files with 110 additions and 16 deletions.
diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs
@@ -7637,6 +7637,7 @@ impl AccountsDb {
         config: &CalcAccountsHashConfig<'_>,
         kind: CalcAccountsHashKind,
         slot: Slot,
+        storages_start_slot: Slot,
     ) -> CacheHashData {
         let accounts_hash_cache_path = if !config.store_detailed_debug_info_on_failure {
             accounts_hash_cache_path
@@ -7648,7 +7649,10 @@ impl AccountsDb {
             _ = std::fs::remove_dir_all(&failed_dir);
             failed_dir
         };
-        CacheHashData::new(accounts_hash_cache_path, kind == CalcAccountsHashKind::Full)
+        CacheHashData::new(
+            accounts_hash_cache_path,
+            (kind == CalcAccountsHashKind::Incremental).then_some(storages_start_slot),
+        )
     }
 
     // modeled after calculate_accounts_delta_hash
@@ -7707,7 +7711,8 @@ impl AccountsDb {
     ) -> Result<(AccountsHashKind, u64), AccountsHashVerificationError> {
         let total_time = Measure::start("");
         let _guard = self.active_stats.activate(ActiveStatItem::Hash);
-        stats.oldest_root = storages.range().start;
+        let storages_start_slot = storages.range().start;
+        stats.oldest_root = storages_start_slot;
 
         self.mark_old_slots_as_dirty(storages, config.epoch_schedule.slots_per_epoch, &mut stats);
 
@@ -7723,7 +7728,8 @@ impl AccountsDb {
                 accounts_hash_cache_path,
                 config,
                 kind,
-                slot
+                slot,
+                storages_start_slot,
             ));
             stats.cache_hash_data_us += cache_hash_data_us;
 
@@ -9963,7 +9969,7 @@ pub mod tests {
             let temp_dir = TempDir::new().unwrap();
             let accounts_hash_cache_path = temp_dir.path().to_path_buf();
             self.scan_snapshot_stores_with_cache(
-                &CacheHashData::new(accounts_hash_cache_path, true),
+                &CacheHashData::new(accounts_hash_cache_path, None),
                 storage,
                 stats,
                 bins,
@@ -11083,7 +11089,7 @@ pub mod tests {
         };
 
         let result = accounts_db.scan_account_storage_no_bank(
-            &CacheHashData::new(accounts_hash_cache_path, true),
+            &CacheHashData::new(accounts_hash_cache_path, None),
             &CalcAccountsHashConfig::default(),
             &get_storage_refs(&[storage]),
             test_scan,

diff --git a/accounts-db/src/cache_hash_data.rs b/accounts-db/src/cache_hash_data.rs
@@ -6,6 +6,7 @@ use {
     bytemuck::{Pod, Zeroable},
     memmap2::MmapMut,
     solana_measure::measure::Measure,
+    solana_sdk::clock::Slot,
     std::{
         collections::HashSet,
         fs::{self, remove_file, File, OpenOptions},
@@ -192,40 +193,59 @@ impl CacheHashDataFile {
 pub(crate) struct CacheHashData {
     cache_dir: PathBuf,
     pre_existing_cache_files: Arc<Mutex<HashSet<PathBuf>>>,
-    should_delete_old_cache_files_on_drop: bool,
+    /// Decides which old cache files to delete.  See `delete_old_cache_files()` for more info.
+    storages_start_slot: Option<Slot>,
     pub stats: Arc<CacheHashDataStats>,
 }
 
 impl Drop for CacheHashData {
     fn drop(&mut self) {
-        if self.should_delete_old_cache_files_on_drop {
-            self.delete_old_cache_files();
-        }
+        self.delete_old_cache_files();
         self.stats.report();
     }
 }
 
 impl CacheHashData {
-    pub(crate) fn new(
-        cache_dir: PathBuf,
-        should_delete_old_cache_files_on_drop: bool,
-    ) -> CacheHashData {
+    pub(crate) fn new(cache_dir: PathBuf, storages_start_slot: Option<Slot>) -> CacheHashData {
         std::fs::create_dir_all(&cache_dir).unwrap_or_else(|err| {
             panic!("error creating cache dir {}: {err}", cache_dir.display())
         });
 
         let result = CacheHashData {
             cache_dir,
             pre_existing_cache_files: Arc::new(Mutex::new(HashSet::default())),
-            should_delete_old_cache_files_on_drop,
+            storages_start_slot,
             stats: Arc::default(),
         };
 
         result.get_cache_files();
         result
     }
+
+    /// delete all pre-existing files that will not be used
     fn delete_old_cache_files(&self) {
-        let old_cache_files = std::mem::take(&mut *self.pre_existing_cache_files.lock().unwrap());
+        // all the renaming files in `pre_existing_cache_files` were *not* used for this
+        // accounts hash calculation
+        let mut old_cache_files =
+            std::mem::take(&mut *self.pre_existing_cache_files.lock().unwrap());
+
+        // If `storages_start_slot` is None, we're doing a full accounts hash calculation, and thus
+        // all unused cache files can be deleted.
+        // If `storages_start_slot` is Some, we're doing an incremental accounts hash calculation,
+        // and we only want to delete the unused cache files *that IAH considered*.
+        if let Some(storages_start_slot) = self.storages_start_slot {
+            old_cache_files.retain(|old_cache_file| {
+                let Some(parsed_filename) = parse_filename(old_cache_file) else {
+                    // if parsing the cache filename fails, we *do* want to delete it
+                    return true;
+                };
+
+                // if the old cache file is in the incremental accounts hash calculation range,
+                // then delete it
+                parsed_filename.slot_range_start >= storages_start_slot
+            });
+        }
+
         if !old_cache_files.is_empty() {
             self.stats
                 .unused_cache_files
@@ -356,6 +376,39 @@ impl CacheHashData {
     }
 }
 
+/// The values of each part of a cache hash data filename
+#[derive(Debug)]
+pub struct ParsedFilename {
+    pub slot_range_start: Slot,
+    pub slot_range_end: Slot,
+    pub bin_range_start: u64,
+    pub bin_range_end: u64,
+    pub hash: u64,
+}
+
+/// Parses a cache hash data filename into its parts
+///
+/// Returns None if the filename is invalid
+fn parse_filename(cache_filename: impl AsRef<Path>) -> Option<ParsedFilename> {
+    let filename = cache_filename.as_ref().to_string_lossy().to_string();
+    let parts: Vec<_> = filename.split('.').collect(); // The parts are separated by a `.`
+    if parts.len() != 5 {
+        return None;
+    }
+    let slot_range_start = parts.first()?.parse().ok()?;
+    let slot_range_end = parts.get(1)?.parse().ok()?;
+    let bin_range_start = parts.get(2)?.parse().ok()?;
+    let bin_range_end = parts.get(3)?.parse().ok()?;
+    let hash = u64::from_str_radix(parts.get(4)?, 16).ok()?; // the hash is in hex
+    Some(ParsedFilename {
+        slot_range_start,
+        slot_range_end,
+        bin_range_start,
+        bin_range_end,
+        hash,
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use {super::*, rand::Rng};
@@ -423,7 +476,7 @@ mod tests {
                                 data_this_pass.push(this_bin_data);
                             }
                         }
-                        let cache = CacheHashData::new(cache_dir.clone(), true);
+                        let cache = CacheHashData::new(cache_dir.clone(), None);
                         let file_name = PathBuf::from("test");
                         cache.save(&file_name, &data_this_pass).unwrap();
                         cache.get_cache_files();
@@ -513,4 +566,39 @@ mod tests {
             ct,
         )
     }
+
+    #[test]
+    fn test_parse_filename() {
+        let good_filename = "123.456.0.65536.537d65697d9b2baa";
+        let parsed_filename = parse_filename(good_filename).unwrap();
+        assert_eq!(parsed_filename.slot_range_start, 123);
+        assert_eq!(parsed_filename.slot_range_end, 456);
+        assert_eq!(parsed_filename.bin_range_start, 0);
+        assert_eq!(parsed_filename.bin_range_end, 65536);
+        assert_eq!(parsed_filename.hash, 0x537d65697d9b2baa);
+
+        let bad_filenames = [
+            // bad separator
+            "123-456-0-65536.537d65697d9b2baa",
+            // bad values
+            "abc.456.0.65536.537d65697d9b2baa",
+            "123.xyz.0.65536.537d65697d9b2baa",
+            "123.456.?.65536.537d65697d9b2baa",
+            "123.456.0.@#$%^.537d65697d9b2baa",
+            "123.456.0.65536.base19shouldfail",
+            "123.456.0.65536.123456789012345678901234567890",
+            // missing values
+            "123.456.0.65536.",
+            "123.456.0.65536",
+            // extra junk
+            "123.456.0.65536.537d65697d9b2baa.42",
+            "123.456.0.65536.537d65697d9b2baa.",
+            "123.456.0.65536.537d65697d9b2baa/",
+            ".123.456.0.65536.537d65697d9b2baa",
+            "/123.456.0.65536.537d65697d9b2baa",
+        ];
+        for bad_filename in bad_filenames {
+            assert!(parse_filename(bad_filename).is_none());
+        }
+    }
 }