rearrange how shrink_in_progress_map is populated (#30466)

This commit is contained in:
Jeff Washington (jwash) 2023-02-23 13:35:38 -06:00 committed by GitHub
parent d6da019ccf
commit a2b0b8e346
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 38 additions and 35 deletions

View File

@ -34,28 +34,37 @@ impl AccountStorage {
/// Return the append vec in 'slot' and with id='store_id'. /// Return the append vec in 'slot' and with id='store_id'.
/// can look in 'map' and 'shrink_in_progress_map' to find the specified append vec /// can look in 'map' and 'shrink_in_progress_map' to find the specified append vec
/// when shrinking begins, shrinking_in_progress is called. /// when shrinking begins, shrinking_in_progress is called.
/// This fn looks in 'map' first, then in 'shrink_in_progress_map' because /// This fn looks in 'map' first, then in 'shrink_in_progress_map', then in 'map' again because
/// 'shrink_in_progress_map' first inserts the old append vec into 'shrink_in_progress_map' /// 'shrink_in_progress' first inserts the new append vec into 'shrink_in_progress_map'
/// and then removes the old append vec from 'map' /// Then, when 'shrink_in_progress' is dropped,
/// Then, the index is updated for all entries to refer to the new id. /// the old append vec is replaced in 'map' with the new append vec
/// then the new append vec is dropped from 'shrink_in_progress_map'.
/// So, it is possible for a race with this fn and dropping 'shrink_in_progress'.
/// Callers to this function have 2 choices: /// Callers to this function have 2 choices:
/// 1. hold the account index read lock for the pubkey so that the account index entry cannot be changed prior to or during this call. (scans do this) /// 1. hold the account index read lock for the pubkey so that the account index entry cannot be changed prior to or during this call. (scans do this)
/// 2. expect to be ready to start over and read the index again if this function returns None /// 2. expect to be ready to start over and read the index again if this function returns None
/// Operations like shrinking or write cache flushing may have updated the index between when the caller read the index and called this function to /// Operations like shrinking or write cache flushing may have updated the index between when the caller read the index and called this function to
/// load from the append vec specified in the index. /// load from the append vec specified in the index.
/// In practice, this fn will return the entry from the map in the very first lookup unless a shrink is in progress.
/// The third lookup will only be called if a requesting thread exactly interposes itself between the 2 map manipulations in the drop of 'shrink_in_progress'.
pub(crate) fn get_account_storage_entry( pub(crate) fn get_account_storage_entry(
&self, &self,
slot: Slot, slot: Slot,
store_id: AppendVecId, store_id: AppendVecId,
) -> Option<Arc<AccountStorageEntry>> { ) -> Option<Arc<AccountStorageEntry>> {
self.map let lookup_in_map = || {
.get(&slot) self.map
.and_then(|r| (r.id == store_id).then_some(Arc::clone(&r.storage))) .get(&slot)
.and_then(|r| (r.id == store_id).then_some(Arc::clone(&r.storage)))
};
lookup_in_map()
.or_else(|| { .or_else(|| {
self.shrink_in_progress_map.get(&slot).and_then(|entry| { self.shrink_in_progress_map.get(&slot).and_then(|entry| {
(entry.value().append_vec_id() == store_id).then(|| Arc::clone(entry.value())) (entry.value().append_vec_id() == store_id).then(|| Arc::clone(entry.value()))
}) })
}) })
.or_else(lookup_in_map)
} }
/// return the append vec for 'slot' if it exists /// return the append vec for 'slot' if it exists
@ -124,18 +133,11 @@ impl AccountStorage {
} }
/// called when shrinking begins on a slot and append vec. /// called when shrinking begins on a slot and append vec.
/// When 'ShrinkInProgress' is dropped by caller, the old store will be removed from the storage map. /// When 'ShrinkInProgress' is dropped by caller, the old store will be replaced with 'new_store' in the storage map.
/// Fails if there are no existing stores at the slot. /// Fails if there are no existing stores at the slot.
/// 'new_store' will be replacing the current store at 'slot' in 'map' /// 'new_store' will be replacing the current store at 'slot' in 'map'
/// 1. insert 'shrinking_store' into 'shrink_in_progress_map' /// So, insert 'new_store' into 'shrink_in_progress_map'.
/// 2. remove 'shrinking_store' from 'map' /// This allows tx processing loads to find the items in 'shrink_in_progress_map' after the index is updated and item is now located in 'new_store'.
/// 3. insert 'new_store' into 'map' (atomic with #2)
/// #1 allows tx processing loads to find the item in 'shrink_in_progress_map' even when it is removed from 'map'
/// #3 allows tx processing loads to find the item in 'map' after the index is updated and it is now located in 'new_store'
/// loading for tx must check
/// a. 'map', because it is usually there
/// b. 'shrink_in_progress_map' because it may have moved there (#1) before it was removed from 'map' (#3)
/// Note that if it fails step a and b, then the retry code in accounts_db will look in the index again and should find the updated index entry to 'new_store'
pub(crate) fn shrinking_in_progress( pub(crate) fn shrinking_in_progress(
&self, &self,
slot: Slot, slot: Slot,
@ -150,26 +152,14 @@ impl AccountStorage {
.storage, .storage,
); );
let new_id = new_store.append_vec_id(); // insert 'new_store' into 'shrink_in_progress_map'
// 1. insert 'shrinking_store' into 'shrink_in_progress_map'
assert!( assert!(
self.shrink_in_progress_map self.shrink_in_progress_map
.insert(slot, Arc::clone(&shrinking_store)) .insert(slot, Arc::clone(&new_store))
.is_none(), .is_none(),
"duplicate call" "duplicate call"
); );
assert!(self
.map
.insert(
slot,
AccountStorageReference {
storage: Arc::clone(&new_store),
id: new_id
}
)
.is_some());
ShrinkInProgress { ShrinkInProgress {
storage: self, storage: self,
slot, slot,
@ -225,8 +215,21 @@ pub(crate) struct ShrinkInProgress<'a> {
/// called when the shrink is no longer in progress. This means we can release the old append vec and update the map of slot -> append vec /// called when the shrink is no longer in progress. This means we can release the old append vec and update the map of slot -> append vec
impl<'a> Drop for ShrinkInProgress<'a> { impl<'a> Drop for ShrinkInProgress<'a> {
fn drop(&mut self) { fn drop(&mut self) {
// The old append vec referenced in 'self' for `slot` assert_eq!(
// can be removed from 'shrink_in_progress_map' self.storage
.map
.insert(
self.slot,
AccountStorageReference {
storage: Arc::clone(&self.new_store),
id: self.new_store.append_vec_id()
}
)
.map(|store| store.id),
Some(self.old_store.append_vec_id())
);
// The new store can be removed from 'shrink_in_progress_map'
assert!(self assert!(self
.storage .storage
.shrink_in_progress_map .shrink_in_progress_map
@ -457,11 +460,11 @@ pub(crate) mod tests {
let shrinking_in_progress = storage.shrinking_in_progress(slot, sample.clone()); let shrinking_in_progress = storage.shrinking_in_progress(slot, sample.clone());
assert!(storage.map.contains_key(&slot)); assert!(storage.map.contains_key(&slot));
assert_eq!( assert_eq!(
id_shrunk, id_to_shrink,
storage.map.get(&slot).unwrap().storage.append_vec_id() storage.map.get(&slot).unwrap().storage.append_vec_id()
); );
assert_eq!( assert_eq!(
(slot, id_to_shrink), (slot, id_shrunk),
storage storage
.shrink_in_progress_map .shrink_in_progress_map
.iter() .iter()