From 342ca4ff8ff5da1ab7097e69be8e4394a61e9b15 Mon Sep 17 00:00:00 2001 From: Dion Dokter Date: Tue, 26 Mar 2024 15:52:33 +0100 Subject: [PATCH] Fix wrongful cache invalidation issue --- src/cache/mod.rs | 24 +++++++++++++++++++ src/cache/tests.rs | 57 ++++++++++++++++++++++++++++++---------------- src/item.rs | 32 ++++++++++++-------------- src/lib.rs | 12 +++++----- src/map.rs | 3 +++ src/queue.rs | 36 ++++++++++++++--------------- 6 files changed, 102 insertions(+), 62 deletions(-) diff --git a/src/cache/mod.rs b/src/cache/mod.rs index e37836a..3b94a97 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -215,6 +215,12 @@ impl NoCache { } } +impl Default for NoCache { + fn default() -> Self { + Self::new() + } +} + impl PrivateCacheImpl for NoCache { type PSC = UncachedPageStates; type PPC = UncachedPagePointers; @@ -279,6 +285,12 @@ impl PageStateCache { } } +impl Default for PageStateCache { + fn default() -> Self { + Self::new() + } +} + impl PrivateCacheImpl for PageStateCache { type PSC = CachedPageStates; type PPC = UncachedPagePointers; @@ -346,6 +358,12 @@ impl PagePointerCache { } } +impl Default for PagePointerCache { + fn default() -> Self { + Self::new() + } +} + impl PrivateCacheImpl for PagePointerCache { type PSC = CachedPageStates; type PPC = CachedPagePointers; @@ -418,6 +436,12 @@ impl KeyPointerCache Default for KeyPointerCache { + fn default() -> Self { + Self::new() + } +} + impl PrivateCacheImpl for KeyPointerCache { diff --git a/src/cache/tests.rs b/src/cache/tests.rs index 729114d..b95fc4c 100644 --- a/src/cache/tests.rs +++ b/src/cache/tests.rs @@ -125,9 +125,9 @@ mod map_tests { run_test(&mut NoCache::new()).await, FlashStatsResult { erases: 198, - reads: 224161, + reads: 233786, writes: 5201, - bytes_read: 1770974, + bytes_read: 1837101, bytes_written: 50401 } ); @@ -139,9 +139,9 @@ mod map_tests { run_test(&mut PageStateCache::::new()).await, FlashStatsResult { erases: 198, - reads: 171543, + reads: 181162, writes: 5201, - bytes_read: 1718356, + bytes_read: 1784477, bytes_written: 50401 } ); @@ -153,9 +153,9 @@ mod map_tests { run_test(&mut PagePointerCache::::new()).await, FlashStatsResult { erases: 198, - reads: 153667, + reads: 163273, writes: 5201, - bytes_read: 1575348, + bytes_read: 1641365, bytes_written: 50401 } ); @@ -164,12 +164,12 @@ mod map_tests { #[test] async fn key_pointer_cache_half() { assert_eq!( - run_test(&mut KeyPointerCache::::new()).await, + run_test(&mut KeyPointerCache::::new()).await, FlashStatsResult { erases: 198, - reads: 130523, + reads: 131503, writes: 5201, - bytes_read: 1318479, + bytes_read: 1299275, bytes_written: 50401 } ); @@ -178,18 +178,18 @@ mod map_tests { #[test] async fn key_pointer_cache_full() { assert_eq!( - run_test(&mut KeyPointerCache::::new()).await, + run_test(&mut KeyPointerCache::::new()).await, FlashStatsResult { erases: 198, - reads: 14506, + reads: 14510, writes: 5201, - bytes_read: 150566, + bytes_read: 150592, bytes_written: 50401 } ); } - async fn run_test(cache: &mut impl KeyCacheImpl) -> FlashStatsResult { + async fn run_test(cache: &mut impl KeyCacheImpl) -> FlashStatsResult { let mut flash = mock_flash::MockFlashBase::::new(WriteCountCheck::Twice, None, true); const FLASH_RANGE: Range = 0x00..0x400; @@ -202,19 +202,36 @@ mod map_tests { let start_snapshot = flash.stats_snapshot(); for _ in 0..100 { - for i in 0..24 { - store_item(&mut flash, FLASH_RANGE, cache, &mut data_buffer, i as u8, &vec![i as u8; LENGHT_PER_KEY[i]].as_slice()) - .await - .unwrap(); + const WRITE_ORDER: [usize; 24] = [ + 15, 0, 4, 22, 18, 11, 19, 8, 14, 23, 5, 1, 16, 10, 6, 12, 20, 17, 3, 9, 7, 13, 21, + 2, + ]; + + for i in WRITE_ORDER { + store_item( + &mut flash, + FLASH_RANGE, + cache, + &mut data_buffer, + i as u16, + &vec![i as u8; LENGHT_PER_KEY[i]].as_slice(), + ) + .await + .unwrap(); } - for i in 0..24 { - let item = fetch_item::( + const READ_ORDER: [usize; 24] = [ + 8, 22, 21, 11, 16, 23, 13, 15, 19, 7, 6, 2, 12, 1, 17, 4, 20, 14, 10, 5, 9, 3, 18, + 0, + ]; + + for i in READ_ORDER { + let item = fetch_item::( &mut flash, FLASH_RANGE, cache, &mut data_buffer, - i as u8, + i as u16, ) .await .unwrap() diff --git a/src/item.rs b/src/item.rs index 068f1f0..54a1342 100644 --- a/src/item.rs +++ b/src/item.rs @@ -21,8 +21,8 @@ //! and has some other modifications to make corruption less likely to happen. //! -use core::ops::Range; use core::num::NonZeroU32; +use core::ops::Range; use embedded_storage_async::nor_flash::{MultiwriteNorFlash, NorFlash}; @@ -54,7 +54,7 @@ impl ItemHeader { flash: &mut S, address: u32, end_address: u32, - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { let mut buffer = [0; MAX_WORD_SIZE]; let header_slice = &mut buffer[..round_up_to_alignment_usize::(Self::LENGTH)]; @@ -104,7 +104,7 @@ impl ItemHeader { data_buffer: &'d mut [u8], address: u32, end_address: u32, - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { match self.crc { None => Ok(MaybeItem::Erased(self, data_buffer)), Some(header_crc) => { @@ -141,11 +141,7 @@ impl ItemHeader { } } - async fn write( - &self, - flash: &mut S, - address: u32, - ) -> Result<(), Error< S::Error>> { + async fn write(&self, flash: &mut S, address: u32) -> Result<(), Error> { let mut buffer = AlignedBuf([0xFF; MAX_WORD_SIZE]); buffer[Self::DATA_CRC_FIELD] @@ -174,7 +170,7 @@ impl ItemHeader { flash_range: Range, cache: &mut impl PrivateCacheImpl, address: u32, - ) -> Result> { + ) -> Result> { self.crc = None; cache.notice_item_erased::(flash_range.clone(), address, &self); self.write(flash, address).await?; @@ -223,7 +219,7 @@ impl<'d> Item<'d> { cache: &mut impl PrivateCacheImpl, address: u32, data: &'d [u8], - ) -> Result> { + ) -> Result> { let header = ItemHeader { length: data.len() as u16, crc: Some(adapted_crc32(data)), @@ -241,7 +237,7 @@ impl<'d> Item<'d> { header: &ItemHeader, data: &[u8], address: u32, - ) -> Result<(), Error< S::Error>> { + ) -> Result<(), Error> { cache.notice_item_written::(flash_range, address, header); header.write(flash, address).await?; @@ -282,7 +278,7 @@ impl<'d> Item<'d> { flash_range: Range, cache: &mut impl PrivateCacheImpl, address: u32, - ) -> Result<(), Error< S::Error>> { + ) -> Result<(), Error> { Self::write_raw( flash, flash_range, @@ -340,7 +336,7 @@ pub async fn find_next_free_item_spot( start_address: u32, end_address: u32, data_length: u32, -) -> Result, Error< S::Error>> { +) -> Result, Error> { let page_index = calculate_page_index::(flash_range, start_address); let free_item_address = match cache.first_item_after_written(page_index) { @@ -392,7 +388,7 @@ impl<'d> core::fmt::Debug for MaybeItem<'d> { } impl<'d> MaybeItem<'d> { - pub fn unwrap(self) -> Result, Error< E>> { + pub fn unwrap(self) -> Result, Error> { match self { MaybeItem::Corrupted(_, _) => Err(Error::Corrupted { #[cfg(feature = "_test")] @@ -476,7 +472,7 @@ pub async fn is_page_empty( cache: &mut impl PrivateCacheImpl, page_index: usize, page_state: Option, -) -> Result> { +) -> Result> { let page_state = match page_state { Some(page_state) => page_state, None => get_page_state::(flash, flash_range.clone(), cache, page_index).await?, @@ -521,7 +517,7 @@ impl ItemIter { &mut self, flash: &mut S, data_buffer: &'m mut [u8], - ) -> Result, u32)>, Error< S::Error>> { + ) -> Result, u32)>, Error> { let mut data_buffer = Some(data_buffer); while let (Some(header), address) = self.header.next(flash).await? { let buffer = data_buffer.take().unwrap(); @@ -558,7 +554,7 @@ impl ItemHeaderIter { pub async fn next( &mut self, flash: &mut S, - ) -> Result<(Option, u32), Error< S::Error>> { + ) -> Result<(Option, u32), Error> { self.traverse(flash, |_, _| false).await } @@ -570,7 +566,7 @@ impl ItemHeaderIter { &mut self, flash: &mut S, callback: impl Fn(&ItemHeader, u32) -> bool, - ) -> Result<(Option, u32), Error< S::Error>> { + ) -> Result<(Option, u32), Error> { loop { match ItemHeader::read_new(flash, self.current_address, self.end_address).await { Ok(Some(header)) => { diff --git a/src/lib.rs b/src/lib.rs index 0e6e357..82930e1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,7 +50,7 @@ async fn try_general_repair( flash: &mut S, flash_range: Range, cache: &mut impl PrivateCacheImpl, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { // Loop through the pages and get their state. If one returns the corrupted error, // the page is likely half-erased. Fix for that is to re-erase again to hopefully finish the job. for page_index in get_pages::(flash_range.clone(), 0) { @@ -74,7 +74,7 @@ async fn find_first_page( cache: &mut impl PrivateCacheImpl, starting_page_index: usize, page_state: PageState, -) -> Result, Error< S::Error>> { +) -> Result, Error> { for page_index in get_pages::(flash_range.clone(), starting_page_index) { if page_state == get_page_state::(flash, flash_range.clone(), cache, page_index).await? { return Ok(Some(page_index)); @@ -138,7 +138,7 @@ async fn get_page_state( flash_range: Range, cache: &mut impl PrivateCacheImpl, page_index: usize, -) -> Result> { +) -> Result> { if let Some(cached_page_state) = cache.get_page_state(page_index) { return Ok(cached_page_state); } @@ -206,7 +206,7 @@ async fn open_page( flash_range: Range, cache: &mut impl PrivateCacheImpl, page_index: usize, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { cache.notice_page_state(page_index, PageState::Open, true); flash @@ -230,7 +230,7 @@ async fn close_page( flash_range: Range, cache: &mut impl PrivateCacheImpl, page_index: usize, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { let current_state = partial_close_page::(flash, flash_range.clone(), cache, page_index).await?; @@ -263,7 +263,7 @@ async fn partial_close_page( flash_range: Range, cache: &mut impl PrivateCacheImpl, page_index: usize, -) -> Result> { +) -> Result> { let current_state = get_page_state::(flash, flash_range.clone(), cache, page_index).await?; if current_state != PageState::Open { diff --git a/src/map.rs b/src/map.rs index 290d573..b3c26fc 100644 --- a/src/map.rs +++ b/src/map.rs @@ -736,6 +736,9 @@ async fn migrate_items( let key = K::deserialize_from(&item.data()[..K::LEN]); let (_, data_buffer) = item.destruct(); + // We're in a decent state here + cache.unmark_dirty(); + // Search for the newest item with the key we found let Some((found_item, found_address)) = fetch_item_with_location::( flash, diff --git a/src/queue.rs b/src/queue.rs index 516e131..c0ca295 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -76,7 +76,7 @@ pub async fn push( cache: &mut impl CacheImpl, data: &[u8], allow_overwrite_old_data: bool, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { run_with_auto_repair!( function = push_inner( flash, @@ -96,7 +96,7 @@ async fn push_inner( cache: &mut impl CacheImpl, data: &[u8], allow_overwrite_old_data: bool, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { assert_eq!(flash_range.start % S::ERASE_SIZE as u32, 0); assert_eq!(flash_range.end % S::ERASE_SIZE as u32, 0); @@ -199,7 +199,7 @@ pub async fn peek_many<'d, S: NorFlash, CI: CacheImpl>( flash: &'d mut S, flash_range: Range, cache: &'d mut CI, -) -> Result, Error< S::Error>> { +) -> Result, Error> { // Note: Corruption repair is done in these functions already Ok(PeekIterator { iter: QueueIterator::new(flash, flash_range, cache).await?, @@ -221,7 +221,7 @@ pub async fn peek<'d, S: NorFlash>( flash_range: Range, cache: &mut impl CacheImpl, data_buffer: &'d mut [u8], -) -> Result, Error< S::Error>> { +) -> Result, Error> { // Note: Corruption repair is done in these functions already peek_many(flash, flash_range, cache) .await? @@ -238,7 +238,7 @@ pub async fn pop_many<'d, S: MultiwriteNorFlash, CI: CacheImpl>( flash: &'d mut S, flash_range: Range, cache: &'d mut CI, -) -> Result, Error< S::Error>> { +) -> Result, Error> { // Note: Corruption repair is done in these functions already Ok(PopIterator { iter: QueueIterator::new(flash, flash_range, cache).await?, @@ -260,7 +260,7 @@ pub async fn pop<'d, S: MultiwriteNorFlash>( flash_range: Range, cache: &mut impl CacheImpl, data_buffer: &'d mut [u8], -) -> Result, Error< S::Error>> { +) -> Result, Error> { pop_many(flash, flash_range, cache) .await? .next(data_buffer) @@ -285,7 +285,7 @@ impl<'d, S: MultiwriteNorFlash, CI: CacheImpl> PopIterator<'d, S, CI> { pub async fn next<'m>( &mut self, data_buffer: &'m mut [u8], - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { let value = run_with_auto_repair!( function = self.next_inner(data_buffer).await, repair = try_repair( @@ -302,7 +302,7 @@ impl<'d, S: MultiwriteNorFlash, CI: CacheImpl> PopIterator<'d, S, CI> { async fn next_inner( &mut self, data_buffer: &mut [u8], - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { if self.iter.cache.is_dirty() { self.iter.cache.invalidate_cache_state(); } @@ -356,7 +356,7 @@ impl<'d, S: NorFlash, CI: CacheImpl> PeekIterator<'d, S, CI> { pub async fn next<'m>( &mut self, data_buffer: &'m mut [u8], - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { // Note: Corruption repair is done in these functions already if self.iter.cache.is_dirty() { @@ -397,7 +397,7 @@ impl<'d, S: NorFlash, CI: CacheImpl> QueueIterator<'d, S, CI> { flash: &'d mut S, flash_range: Range, cache: &'d mut CI, - ) -> Result> { + ) -> Result> { let start_address = run_with_auto_repair!( function = Self::find_start_address(flash, flash_range.clone(), cache).await, repair = try_repair(flash, flash_range.clone(), cache).await? @@ -415,7 +415,7 @@ impl<'d, S: NorFlash, CI: CacheImpl> QueueIterator<'d, S, CI> { flash: &mut S, flash_range: Range, cache: &mut CI, - ) -> Result> { + ) -> Result> { assert_eq!(flash_range.start % S::ERASE_SIZE as u32, 0); assert_eq!(flash_range.end % S::ERASE_SIZE as u32, 0); @@ -442,7 +442,7 @@ impl<'d, S: NorFlash, CI: CacheImpl> QueueIterator<'d, S, CI> { async fn next<'m>( &mut self, data_buffer: &'m mut [u8], - ) -> Result, u32)>, Error< S::Error>> { + ) -> Result, u32)>, Error> { let value = run_with_auto_repair!( function = self.next_inner(data_buffer).await, repair = try_repair(self.flash, self.flash_range.clone(), self.cache).await? @@ -454,7 +454,7 @@ impl<'d, S: NorFlash, CI: CacheImpl> QueueIterator<'d, S, CI> { async fn next_inner( &mut self, data_buffer: &mut [u8], - ) -> Result, Error< S::Error>> { + ) -> Result, Error> { let mut data_buffer = Some(data_buffer); if self.cache.is_dirty() { @@ -570,7 +570,7 @@ pub async fn find_max_fit( flash: &mut S, flash_range: Range, cache: &mut impl CacheImpl, -) -> Result, Error< S::Error>> { +) -> Result, Error> { run_with_auto_repair!( function = find_max_fit_inner(flash, flash_range.clone(), cache).await, repair = try_repair(flash, flash_range.clone(), cache).await? @@ -581,7 +581,7 @@ async fn find_max_fit_inner( flash: &mut S, flash_range: Range, cache: &mut impl CacheImpl, -) -> Result, Error< S::Error>> { +) -> Result, Error> { assert_eq!(flash_range.start % S::ERASE_SIZE as u32, 0); assert_eq!(flash_range.end % S::ERASE_SIZE as u32, 0); @@ -647,7 +647,7 @@ async fn find_youngest_page( flash: &mut S, flash_range: Range, cache: &mut impl PrivateCacheImpl, -) -> Result> { +) -> Result> { let last_used_page = find_first_page(flash, flash_range.clone(), cache, 0, PageState::PartialOpen).await?; @@ -673,7 +673,7 @@ async fn find_oldest_page( flash: &mut S, flash_range: Range, cache: &mut impl PrivateCacheImpl, -) -> Result> { +) -> Result> { let youngest_page = find_youngest_page(flash, flash_range.clone(), cache).await?; // The oldest page is the first non-open page after the youngest page @@ -697,7 +697,7 @@ async fn try_repair( flash: &mut S, flash_range: Range, cache: &mut impl CacheImpl, -) -> Result<(), Error< S::Error>> { +) -> Result<(), Error> { cache.invalidate_cache_state(); crate::try_general_repair(flash, flash_range.clone(), cache).await?;