Skip to content

Test whether supporting passing an optional mutable buffer to Storage::read_slice has drawbacks (it does) #488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ia0 wants to merge 2 commits into from
Closed

Test whether supporting passing an optional mutable buffer to Storage::read_slice has drawbacks (it does) #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f571266
Allow read_slice to return a Vec
ia0 Jun 3, 2022
c26a8fa
Also take an optional buffer
ia0 Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/erase_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn is_page_erased(storage: &dyn Storage, page: usize) -> bool {
let index = StorageIndex { page, byte: 0 };
let length = storage.page_size();
storage
.read_slice(index, length)
.read_slice(index, length, None)
.unwrap()
.iter()
.all(|&x| x == 0xff)
Expand Down
8 changes: 4 additions & 4 deletions examples/store_latency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,10 @@ fn main() {
write_matrix(matrix);

// Results for nrf52840dk_opensk:
// StorageConfig { page_size: 4096, num_pages: 20 }
// Overwrite Length Boot Compaction Insert Remove
// no 50 words 16.2 ms 143.8 ms 18.3 ms 8.4 ms
// yes 1 words 303.8 ms 97.9 ms 9.7 ms 4.7 ms
// StorageConfig { num_pages: 20 }
// Overwrite Length Boot Compaction Insert Remove
// no 50 words 20.6 ms 147.1 ms 22.9 ms 10.8 ms
// yes 1 words 351.6 ms 102.3 ms 12.9 ms 6.3 ms
}

fn align(x: &str, n: usize) {
Expand Down
69 changes: 54 additions & 15 deletions libraries/persistent_store/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! actual flash storage. Instead it uses a buffer in memory to represent the storage state.

use crate::{Storage, StorageError, StorageIndex, StorageResult};
use alloc::borrow::Borrow;
use alloc::borrow::{Borrow, Cow};
use alloc::boxed::Box;
use alloc::vec;

Expand Down Expand Up @@ -301,8 +301,23 @@ impl Storage for BufferStorage {
self.options.max_page_erases
}

fn read_slice(&self, index: StorageIndex, length: usize) -> StorageResult<&[u8]> {
Ok(&self.storage[index.range(length, self)?])
fn read_slice<'a>(
&'a self,
index: StorageIndex,
length: usize,
buffer: Option<&'a mut [u8]>,
) -> StorageResult<Cow<'a, [u8]>> {
let slice = &self.storage[index.range(length, self)?];
match buffer {
None => Ok(Cow::Borrowed(slice)),
Some(buffer) => {
if buffer.len() < length {
return Err(StorageError::OutOfBounds);
}
buffer.copy_from_slice(slice);
Ok(Cow::Borrowed(buffer))
}
}
}

fn write_slice(&mut self, index: StorageIndex, value: &[u8]) -> StorageResult<()> {
Expand Down Expand Up @@ -494,6 +509,14 @@ mod tests {
vec![0xff; NUM_PAGES * OPTIONS.page_size].into_boxed_slice()
}

fn read_slice(
store: &BufferStorage,
index: StorageIndex,
length: usize,
) -> StorageResult<Cow<[u8]>> {
store.read_slice(index, length, None)
}

#[test]
fn words_are_decreasing() {
fn assert_is_decreasing(prev: &[u8], next: &[u8]) {
Expand Down Expand Up @@ -522,10 +545,10 @@ mod tests {
let mut buffer = BufferStorage::new(new_storage(), OPTIONS);
let index = StorageIndex { page: 0, byte: 0 };
let next_index = StorageIndex { page: 0, byte: 4 };
assert_eq!(buffer.read_slice(index, 4).unwrap(), BLANK_WORD);
assert_eq!(read_slice(&buffer, index, 4).unwrap(), BLANK_WORD);
buffer.write_slice(index, FIRST_WORD).unwrap();
assert_eq!(buffer.read_slice(index, 4).unwrap(), FIRST_WORD);
assert_eq!(buffer.read_slice(next_index, 4).unwrap(), BLANK_WORD);
assert_eq!(read_slice(&buffer, index, 4).unwrap(), FIRST_WORD);
assert_eq!(read_slice(&buffer, next_index, 4).unwrap(), BLANK_WORD);
}

#[test]
Expand All @@ -535,11 +558,11 @@ mod tests {
let other_index = StorageIndex { page: 1, byte: 0 };
buffer.write_slice(index, FIRST_WORD).unwrap();
buffer.write_slice(other_index, FIRST_WORD).unwrap();
assert_eq!(buffer.read_slice(index, 4).unwrap(), FIRST_WORD);
assert_eq!(buffer.read_slice(other_index, 4).unwrap(), FIRST_WORD);
assert_eq!(read_slice(&buffer, index, 4).unwrap(), FIRST_WORD);
assert_eq!(read_slice(&buffer, other_index, 4).unwrap(), FIRST_WORD);
buffer.erase_page(0).unwrap();
assert_eq!(buffer.read_slice(index, 4).unwrap(), BLANK_WORD);
assert_eq!(buffer.read_slice(other_index, 4).unwrap(), FIRST_WORD);
assert_eq!(read_slice(&buffer, index, 4).unwrap(), BLANK_WORD);
assert_eq!(read_slice(&buffer, other_index, 4).unwrap(), FIRST_WORD);
}

#[test]
Expand All @@ -551,15 +574,15 @@ mod tests {
let bad_page = StorageIndex { page: 2, byte: 0 };

// Reading a word in the storage is ok.
assert!(buffer.read_slice(index, 4).is_ok());
assert!(read_slice(&buffer, index, 4).is_ok());
// Reading a half-word in the storage is ok.
assert!(buffer.read_slice(half_index, 2).is_ok());
assert!(read_slice(&buffer, half_index, 2).is_ok());
// Reading even a single byte outside a page is not ok.
assert!(buffer.read_slice(over_index, 1).is_err());
assert!(read_slice(&buffer, over_index, 1).is_err());
// But reading an empty slice just after a page is ok.
assert!(buffer.read_slice(over_index, 0).is_ok());
assert!(read_slice(&buffer, over_index, 0).is_ok());
// Reading even an empty slice outside the storage is not ok.
assert!(buffer.read_slice(bad_page, 0).is_err());
assert!(read_slice(&buffer, bad_page, 0).is_err());

// Writing a word in the storage is ok.
assert!(buffer.write_slice(index, FIRST_WORD).is_ok());
Expand Down Expand Up @@ -689,4 +712,20 @@ mod tests {
assert_eq!(&buffer.storage[..8], &[0x5c; 8]);
assert!(buffer.storage[8..].iter().all(|&x| x == 0xff));
}

#[test]
fn read_slice_lifetime() {
let buffer = BufferStorage::new(new_storage(), OPTIONS);
let mut slice1 = [0; 10];
let mut slice2 = [0; 10];
let index = StorageIndex { page: 0, byte: 0 };
let result1 = buffer.read_slice(index, 10, Some(&mut slice1)).unwrap();
let result2 = buffer.read_slice(index, 10, Some(&mut slice2)).unwrap();
// We can use result2 while result1 is alive.
assert_eq!(result2, &[0xff; 10][..]);
// We can use slice2 once result2 is dead.
assert_eq!(slice2, &[0xff; 10][..]);
assert_eq!(result1, &[0xff; 10][..]);
assert_eq!(slice1, &[0xff; 10][..]);
}
}
9 changes: 8 additions & 1 deletion libraries/persistent_store/src/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

//! Flash storage abstraction.

use alloc::borrow::Cow;

/// Represents a byte position in a storage.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct StorageIndex {
Expand Down Expand Up @@ -60,7 +62,12 @@ pub trait Storage {
/// Reads a byte slice from the storage.
///
/// The `index` must designate `length` bytes in the storage.
fn read_slice(&self, index: StorageIndex, length: usize) -> StorageResult<&[u8]>;
fn read_slice<'a>(
&'a self,
index: StorageIndex,
length: usize,
buffer: Option<&'a mut [u8]>,
) -> StorageResult<Cow<'a, [u8]>>;

/// Writes a word slice to the storage.
///
Expand Down
38 changes: 21 additions & 17 deletions libraries/persistent_store/src/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use crate::{usize_to_nat, Nat, Storage, StorageError, StorageIndex};
pub use crate::{
BufferStorage, StoreDriver, StoreDriverOff, StoreDriverOn, StoreInterruption, StoreInvariant,
};
use alloc::borrow::Cow;
use alloc::boxed::Box;
use alloc::vec::Vec;
use core::borrow::Borrow;
Expand Down Expand Up @@ -490,7 +491,8 @@ impl<S: Storage> Store<S> {
fn recover_initialize(&mut self) -> StoreResult<()> {
let word_size = self.format.word_size();
for page in 0..self.format.num_pages() {
let (init, rest) = self.read_page(page).split_at(word_size as usize);
let content = self.read_page(page);
let (init, rest) = content.split_at(word_size as usize);
if (page > 0 && !is_erased(init)) || !is_erased(rest) {
return Ok(());
}
Expand Down Expand Up @@ -890,15 +892,15 @@ impl<S: Storage> Store<S> {

/// Sets the padding bit of a user header.
fn set_padding(&mut self, pos: Position) -> StoreResult<()> {
let mut word = Word::from_slice(self.read_word(pos));
let mut word = Word::from_slice(&self.read_word(pos));
self.format.set_padding(&mut word)?;
self.write_slice(pos, &word.as_slice())?;
Ok(())
}

/// Sets the deleted bit of a user header.
fn set_deleted(&mut self, pos: Position) -> StoreResult<()> {
let mut word = Word::from_slice(self.read_word(pos));
let mut word = Word::from_slice(&self.read_word(pos));
self.format.set_deleted(&mut word);
self.write_slice(pos, &word.as_slice())?;
Ok(())
Expand Down Expand Up @@ -1001,13 +1003,13 @@ impl<S: Storage> Store<S> {
0 => None,
_ => Some(self.read_word(*pos + length)),
};
if header.check(footer) {
if header.check(footer.as_deref()) {
if header.key > self.format.max_key() {
return Err(StoreError::InvalidStorage);
}
*pos += 1 + length;
ParsedEntry::User(header)
} else if footer.map_or(true, |x| is_erased(x)) {
} else if footer.map_or(true, |x| is_erased(&x)) {
self.parse_partial(pos)
} else {
*pos += 1 + length;
Expand All @@ -1028,7 +1030,7 @@ impl<S: Storage> Store<S> {
fn parse_partial(&self, pos: &mut Position) -> ParsedEntry {
let mut length = None;
for i in 0..self.format.max_prefix_len() {
if !is_erased(self.read_word(*pos + i)) {
if !is_erased(&self.read_word(*pos + i)) {
length = Some(i);
}
}
Expand All @@ -1045,20 +1047,20 @@ impl<S: Storage> Store<S> {
fn parse_init(&self, page: Nat) -> StoreResult<WordState<InitInfo>> {
let index = self.format.index_init(page);
let word = self.storage_read_slice(index, self.format.word_size());
self.format.parse_init(Word::from_slice(word))
self.format.parse_init(Word::from_slice(&word))
}

/// Parses the compact info of a page.
fn parse_compact(&self, page: Nat) -> StoreResult<WordState<CompactInfo>> {
let index = self.format.index_compact(page);
let word = self.storage_read_slice(index, self.format.word_size());
self.format.parse_compact(Word::from_slice(word))
self.format.parse_compact(Word::from_slice(&word))
}

/// Parses a word from the virtual storage.
fn parse_word(&self, pos: Position) -> StoreResult<WordState<ParsedWord>> {
self.format
.parse_word(Word::from_slice(self.read_word(pos)))
.parse_word(Word::from_slice(&self.read_word(pos)))
}

/// Reads a slice from the virtual storage.
Expand All @@ -1068,22 +1070,22 @@ impl<S: Storage> Store<S> {
let mut result = Vec::with_capacity(length as usize);
let index = pos.index(&self.format);
let max_length = self.format.page_size() - usize_to_nat(index.byte);
result.extend_from_slice(self.storage_read_slice(index, min(length, max_length)));
result.extend_from_slice(&self.storage_read_slice(index, min(length, max_length)));
if length > max_length {
// The slice spans the next page.
let index = pos.next_page(&self.format).index(&self.format);
result.extend_from_slice(self.storage_read_slice(index, length - max_length));
result.extend_from_slice(&self.storage_read_slice(index, length - max_length));
}
result
}

/// Reads a word from the virtual storage.
fn read_word(&self, pos: Position) -> &[u8] {
fn read_word(&self, pos: Position) -> Cow<[u8]> {
self.storage_read_slice(pos.index(&self.format), self.format.word_size())
}

/// Reads a physical page.
fn read_page(&self, page: Nat) -> &[u8] {
fn read_page(&self, page: Nat) -> Cow<[u8]> {
let index = StorageIndex {
page: page as usize,
byte: 0,
Expand All @@ -1092,9 +1094,11 @@ impl<S: Storage> Store<S> {
}

/// Reads a slice from the physical storage.
fn storage_read_slice(&self, index: StorageIndex, length: Nat) -> &[u8] {
fn storage_read_slice(&self, index: StorageIndex, length: Nat) -> Cow<[u8]> {
// The only possible failures are if the slice spans multiple pages.
self.storage.read_slice(index, length as usize).unwrap()
self.storage
.read_slice(index, length as usize, None)
.unwrap()
}

/// Writes a slice to the virtual storage.
Expand All @@ -1119,7 +1123,7 @@ impl<S: Storage> Store<S> {
fn storage_write_slice(&mut self, index: StorageIndex, value: &[u8]) -> StoreResult<()> {
let word_size = self.format.word_size();
debug_assert!(usize_to_nat(value.len()) % word_size == 0);
let slice = self.storage.read_slice(index, value.len())?;
let slice = self.storage.read_slice(index, value.len(), None)?;
// Skip as many words that don't need to be written as possible.
for start in (0..usize_to_nat(value.len())).step_by(word_size as usize) {
if is_write_needed(
Expand All @@ -1142,7 +1146,7 @@ impl<S: Storage> Store<S> {

/// Erases a page if not already erased.
fn storage_erase_page(&mut self, page: Nat) -> StoreResult<()> {
if !is_erased(self.read_page(page)) {
if !is_erased(&self.read_page(page)) {
self.storage.erase_page(page as usize)?;
}
Ok(())
Expand Down
27 changes: 22 additions & 5 deletions src/env/tock/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use crate::api::upgrade_storage::helper::{find_slice, is_aligned, ModRange};
use crate::api::upgrade_storage::UpgradeStorage;
use alloc::borrow::Cow;
use alloc::vec::Vec;
use core::cell::Cell;
use libtock_core::{callback, syscalls};
Expand Down Expand Up @@ -173,6 +174,11 @@ impl TockStorage {
fn is_page_aligned(&self, x: usize) -> bool {
is_aligned(self.page_size, x)
}

fn find_slice(&self, index: StorageIndex, length: usize) -> StorageResult<&[u8]> {
let start = index.range(length, self)?.start;
find_slice(&self.storage_locations, start, length)
}
}

impl Storage for TockStorage {
Expand All @@ -196,23 +202,34 @@ impl Storage for TockStorage {
self.max_page_erases
}

fn read_slice(&self, index: StorageIndex, length: usize) -> StorageResult<&[u8]> {
let start = index.range(length, self)?.start;
find_slice(&self.storage_locations, start, length)
fn read_slice<'a>(
&'a self,
index: StorageIndex,
length: usize,
buffer: Option<&'a mut [u8]>,
) -> StorageResult<Cow<'a, [u8]>> {
let slice = self.find_slice(index, length)?;
match buffer {
None => Ok(Cow::Borrowed(slice)),
Some(buffer) => {
buffer.copy_from_slice(slice);
Ok(Cow::Borrowed(slice))
}
}
}

fn write_slice(&mut self, index: StorageIndex, value: &[u8]) -> StorageResult<()> {
if !self.is_word_aligned(index.byte) || !self.is_word_aligned(value.len()) {
return Err(StorageError::NotAligned);
}
let ptr = self.read_slice(index, value.len())?.as_ptr() as usize;
let ptr = self.find_slice(index, value.len())?.as_ptr() as usize;
write_slice(ptr, value)
}

fn erase_page(&mut self, page: usize) -> StorageResult<()> {
let index = StorageIndex { page, byte: 0 };
let length = self.page_size();
let ptr = self.read_slice(index, length)?.as_ptr() as usize;
let ptr = self.find_slice(index, length)?.as_ptr() as usize;
erase_page(ptr, length)
}
}
Expand Down