From f9e10ef3a925c1183641386aa66d35e3725c91ab Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Fri, 7 Feb 2020 02:29:20 +0900 Subject: [PATCH 1/3] Restore last_root to fix unintended storage delete --- runtime/src/accounts_db.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 9f40734aa257c1..9aa6467ccc9b8a 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1265,6 +1265,7 @@ impl AccountsDB { let mut slots: Vec = storage.0.keys().cloned().collect(); slots.sort(); let mut accounts_index = self.accounts_index.write().unwrap(); + let mut last_root = 0; for slot_id in slots.iter() { let mut accumulator: Vec> = self .scan_account_storage( @@ -1289,13 +1290,15 @@ impl AccountsDB { AccountsDB::merge(&mut account_maps, &maps); } if !account_maps.is_empty() { - accounts_index.roots.insert(*slot_id); + last_root = *slot_id; + accounts_index.roots.insert(last_root); let mut _reclaims: Vec<(u64, AccountInfo)> = vec![]; for (pubkey, (_, account_info)) in account_maps.iter() { accounts_index.insert(*slot_id, pubkey, account_info.clone(), &mut _reclaims); } } } + accounts_index.last_root = last_root; let mut counts = HashMap::new(); for slot_list in accounts_index.account_maps.values() { From 31205049dd2183d88e8f141f4e0df26d0798b56b Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Sun, 9 Feb 2020 00:51:33 +0900 Subject: [PATCH 2/3] Remove last_root thing altogether --- runtime/src/accounts_db.rs | 28 +++++++++------------------- runtime/src/accounts_index.rs | 27 --------------------------- 2 files changed, 9 insertions(+), 46 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 9aa6467ccc9b8a..020961b76b23b4 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -671,8 +671,6 @@ impl AccountsDB { reclaims.extend(new_reclaims); } - let last_root = accounts_index.last_root; - drop(accounts_index); drop(storage); @@ -683,16 +681,16 @@ impl AccountsDB { } } - self.handle_reclaims(&reclaims, last_root); + self.handle_reclaims(&reclaims); } - fn handle_reclaims(&self, reclaims: &[(Slot, AccountInfo)], last_root: Slot) { + fn handle_reclaims(&self, reclaims: &[(Slot, AccountInfo)]) { let mut dead_accounts = Measure::start("reclaims::remove_dead_accounts"); let mut dead_slots = self.remove_dead_accounts(reclaims); dead_accounts.stop(); let mut cleanup_dead_slots = Measure::start("reclaims::purge_slots"); - self.cleanup_dead_slots(&mut dead_slots, last_root); + self.cleanup_dead_slots(&mut dead_slots); cleanup_dead_slots.stop(); let mut purge_slots = Measure::start("reclaims::purge_slots"); @@ -1097,7 +1095,7 @@ impl AccountsDB { slot_id: Slot, infos: Vec, accounts: &[(&Pubkey, &Account)], - ) -> (Vec<(Slot, AccountInfo)>, u64) { + ) -> Vec<(Slot, AccountInfo)> { let mut reclaims: Vec<(Slot, AccountInfo)> = Vec::with_capacity(infos.len() * 2); let index = self.accounts_index.read().unwrap(); let mut update_index_work = Measure::start("update_index_work"); @@ -1112,7 +1110,6 @@ impl AccountsDB { }) .collect(); - let last_root = index.last_root; drop(index); if !inserts.is_empty() { let mut index = self.accounts_index.write().unwrap(); @@ -1121,7 +1118,7 @@ impl AccountsDB { } } update_index_work.stop(); - (reclaims, last_root) + reclaims } fn remove_dead_accounts(&self, reclaims: &[(Slot, AccountInfo)]) -> HashSet { @@ -1156,9 +1153,7 @@ impl AccountsDB { dead_slots } - fn cleanup_dead_slots(&self, dead_slots: &mut HashSet, last_root: u64) { - // a slot is not totally dead until it is older than the root - dead_slots.retain(|slot| *slot < last_root); + fn cleanup_dead_slots(&self, dead_slots: &mut HashSet) { if !dead_slots.is_empty() { { let mut index = self.accounts_index.write().unwrap(); @@ -1219,11 +1214,11 @@ impl AccountsDB { store_accounts.stop(); let mut update_index = Measure::start("store::update_index"); - let (reclaims, last_root) = self.update_index(slot_id, infos, accounts); + let reclaims = self.update_index(slot_id, infos, accounts); update_index.stop(); trace!("reclaim: {}", reclaims.len()); - self.handle_reclaims(&reclaims, last_root); + self.handle_reclaims(&reclaims); } pub fn add_root(&self, slot: Slot) { @@ -1265,7 +1260,6 @@ impl AccountsDB { let mut slots: Vec = storage.0.keys().cloned().collect(); slots.sort(); let mut accounts_index = self.accounts_index.write().unwrap(); - let mut last_root = 0; for slot_id in slots.iter() { let mut accumulator: Vec> = self .scan_account_storage( @@ -1290,15 +1284,13 @@ impl AccountsDB { AccountsDB::merge(&mut account_maps, &maps); } if !account_maps.is_empty() { - last_root = *slot_id; - accounts_index.roots.insert(last_root); + accounts_index.roots.insert(*slot_id); let mut _reclaims: Vec<(u64, AccountInfo)> = vec![]; for (pubkey, (_, account_info)) in account_maps.iter() { accounts_index.insert(*slot_id, pubkey, account_info.clone(), &mut _reclaims); } } } - accounts_index.last_root = last_root; let mut counts = HashMap::new(); for slot_list in accounts_index.account_maps.values() { @@ -1785,9 +1777,7 @@ pub mod tests { let (list, idx) = index.get(&pubkey, &ancestors).unwrap(); list[idx].1.store_id }; - //slot 0 is behind root, but it is not root, therefore it is purged accounts.add_root(1); - assert!(accounts.accounts_index.read().unwrap().is_purged(0)); //slot is still there, since gc is lazy assert!(accounts.storage.read().unwrap().0[&0].get(&id).is_some()); diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index 0fa96263684734..dd5c2fadabbc55 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -12,9 +12,6 @@ pub struct AccountsIndex { pub account_maps: HashMap>>, pub roots: HashSet, - - // This value that needs to be stored to recover the index from AppendVec - pub last_root: Slot, } impl AccountsIndex { @@ -148,10 +145,6 @@ impl AccountsIndex { entry.write().unwrap().push((slot, account_info)); } - pub fn is_purged(&self, slot: Slot) -> bool { - slot < self.last_root - } - pub fn can_purge(max_root: Slot, slot: Slot) -> bool { slot < max_root } @@ -161,11 +154,6 @@ impl AccountsIndex { } pub fn add_root(&mut self, slot: Slot) { - assert!( - (self.last_root == 0 && slot == 0) || (slot >= self.last_root), - "new roots must be increasing" - ); - self.last_root = slot; self.roots.insert(slot); } /// Remove the slot when the storage for the slot is freed @@ -270,21 +258,6 @@ mod tests { assert_eq!(list[idx], (0, true)); } - #[test] - fn test_is_purged() { - let mut index = AccountsIndex::::default(); - assert!(!index.is_purged(0)); - index.add_root(1); - assert!(index.is_purged(0)); - } - - #[test] - fn test_max_last_root() { - let mut index = AccountsIndex::::default(); - index.add_root(1); - assert_eq!(index.last_root, 1); - } - #[test] #[should_panic] fn test_max_last_root_old() { From da1af6c23930a92237675a28c43d92534a19c71e Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Sun, 9 Feb 2020 01:33:38 +0900 Subject: [PATCH 3/3] Remove unneeded test... --- runtime/src/accounts_index.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index dd5c2fadabbc55..6138573536b92d 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -258,14 +258,6 @@ mod tests { assert_eq!(list[idx], (0, true)); } - #[test] - #[should_panic] - fn test_max_last_root_old() { - let mut index = AccountsIndex::::default(); - index.add_root(1); - index.add_root(0); - } - #[test] fn test_cleanup_first() { let mut index = AccountsIndex::::default();