app_persistency: new implementation #48

svenrademakers · 2023-09-07T14:03:24Z

This commit aims to improve the internal bmcd persistency:

Its more lean, no sqlite implementation anymore
Implemented a writeback mechanism that tries to reduce the amount of
writes to the file system (can be disabled all together)
read cache

The implementation is based on the following assumptions:

This store will only be used for internal application state.
We expect that it will contain a maximum of approximately 10 settings.
Settings are not updated frequently. Users will setup their system
once and no changes can occur in weeks .

The solution aims to reduce the amount of writes to the filesystem.
Secondly, no additional database driver is used. The key/value store
gets written as raw bytes. All to make sure the actual space on the
filesystem stays as small as possible.

Since the key/value store is not expected to go over 1 LEB, the whole
key/value store gets written on a update, even when only one value
changed.

further more the api of the persistency store got extended,
differentiating between try_* and get/set. Non-try functions will panic
on serialization errors, or when the given key is not registered.

A persistency component gets initialized using the PersistencyBuilder.
It enforces users to declare all the keys that are used by the
application. This is to make sure to limit the possibilities of using
wrong keys.

Note 1: There is no migration implemented from the old sqlite implementation.
This means that users will lose their settings when upgrading to this
version. At the moment of writing, there are only 2 low impact key/value
pairs, which does not justify writing migration code.

Note 2: There is no protection implemented for trans serializing
different types. Its up to the developer to make sure to use the correct
data types.

this commit fixes #107, turing-machines/BMC-Firmware#107

tpi_rs/src/middleware/persistency/app_persistency.rs

svenrademakers · 2023-09-11T12:52:35Z

tpi_rs/src/middleware/persistency/binary_persistency.rs

+use super::error::PersistencyError;
+
+const BINARY_VERSION: u32 = 1;
+const BINARY_MAGIC: u32 = 0xdeadbeef;


let's come up with something more original.. or unique

Suggested change

const BINARY_MAGIC: u32 = 0xdeadbeef;

const BINARY_MAGIC: u32 = 0xdef0ad9e;

b"TPI2APDB".as_slice()

what about b"TMAPPDB". This storage format is not necessarily tightly coupled to Turing Pi use-cases.

This commit aims to improve the internal bmcd persistency: * Its more lean, no sqlite implementation anymore * Implemented a writeback mechanism that tries to reduce the amount of writes to the file system (can be disabled all together) * read cache The implementation is based on the following assumptions: * This store will only be used for internal application state. * We expect that it will contain a maximum of approximately 10 settings. * Settings are not updated frequently. Users will setup their system once and no changes can occur in weeks . The solution aims to reduce the amount of writes to the filesystem. Secondly, no additional database driver is used. The key/value store gets written as raw bytes. All to make sure the actual space on the filesystem stays as small as possible. Since the key/value store is not expected to go over 1 LEB, the whole key/value store gets written on a update, even when only one value changed. furthermore the api of the persistency store got extended, differentiating between try_* and get/set. Non-try functions will panic on serialization errors, or when the given key is not registered. A persistency component gets initialized using the `PersistencyBuilder`. It enforces users to declare all the keys that are used by the application. This is to make sure to limit the possibilities of using wrong keys. Note 1: There is no migration implemented from the old sqlite implementation. This means that users will lose their settings when upgrading to this version. At the moment of writing, there are only 2 low impact key/value pairs, which does not justify writing migration code. Note 2: There is no protection implemented for trans serializing different types. Its up to the developer to make sure to use the correct data types. this commit fixes #107, turing-machines/BMC-Firmware#107

ruslashev

A bit of a preliminary review. I'll finish reviewing tomorrow.

tpi_rs/src/middleware/persistency/app_persistency.rs

ruslashev · 2023-09-13T12:59:58Z

tpi_rs/src/middleware/persistency/binary_persistency.rs

+use super::error::PersistencyError;
+
+const BINARY_VERSION: u32 = 1;
+const BINARY_MAGIC: u32 = 0xdeadbeef;


b"TPI2APDB".as_slice()

tpi_rs/src/middleware/persistency/binary_persistency.rs

ruslashev · 2023-09-13T13:06:52Z

tpi_rs/src/middleware/persistency/app_persistency.rs

+    ) -> anyhow::Result<Self>
+    where
+        I: IntoIterator<Item = (&'static str, Vec<u8>)>,
+        P: Into<PathBuf>,


These two types should also be Send to be safely sendable between await points

- I: IntoIterator<Item = (&'static str, Vec<u8>)>, - P: Into<PathBuf>, + I: IntoIterator<Item = (&'static str, Vec<u8>)> + Send, + P: Into<PathBuf> + Send,

Same for binary_persistency.rs:112

new is not a async function. Therefore the arguments do not travel over await points, Send does not apply here.

Same for binary_persistency.rs:112

Calling async functions does not imply that such functions can be executed across threads by the runtime (tokio). async functions are always executed in the current thread's context and can only be pushed to other threads by explicit operations. e.g. tokio::spawn.

You can see async functions as single-threaded state machines that yield back control to the event loop of the thread such function runs on. e.g., they express a way to execute a blocking function non-blocking.

This means that Send is not required here.

ruslashev · 2023-09-13T13:21:11Z

tpi_rs/src/middleware/persistency/app_persistency.rs

+    /// occurrence started from the last write. This function disables the write
+    /// on timeout. The key/value store only gets written when the
+    /// [ApplicationPersistency] is dropped.
+    pub fn disable_write_on_timeout(mut self) -> Self {


Doesn't seem to be used. Was this a debugging feature?

It was a debug feature, but I also anticipate we need to have this at hand. Its a self documenting way to disable timeout writebacks when needed

svenrademakers force-pushed the feature/persistency_changes branch 2 times, most recently from f9c6524 to 89848ab Compare September 8, 2023 11:52

svenrademakers requested a review from ruslashev September 8, 2023 11:52

svenrademakers marked this pull request as ready for review September 8, 2023 11:53

svenrademakers force-pushed the feature/persistency_changes branch 2 times, most recently from 986e80d to d81f174 Compare September 11, 2023 11:03

svenrademakers commented Sep 11, 2023

View reviewed changes

tpi_rs/src/middleware/persistency/app_persistency.rs Outdated Show resolved Hide resolved

svenrademakers commented Sep 11, 2023

View reviewed changes

tpi_rs/src/middleware/persistency/app_persistency.rs Outdated Show resolved Hide resolved

svenrademakers commented Sep 11, 2023

View reviewed changes

svenrademakers force-pushed the master branch 2 times, most recently from 0a405e4 to 48fd205 Compare September 12, 2023 07:38

svenrademakers added 2 commits September 13, 2023 12:46

app_persistency: update documentation

ecfe22d

svenrademakers force-pushed the feature/persistency_changes branch from d81f174 to ecfe22d Compare September 13, 2023 11:48

ruslashev reviewed Sep 13, 2023

View reviewed changes

app_persistency: improvements and review comments

4be4602

svenrademakers merged commit 5b3af5b into master Sep 15, 2023
3 checks passed

svenrademakers deleted the feature/persistency_changes branch September 15, 2023 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app_persistency: new implementation #48

app_persistency: new implementation #48

svenrademakers commented Sep 7, 2023 •

edited

Loading

svenrademakers Sep 11, 2023

ruslashev Sep 13, 2023

svenrademakers Sep 14, 2023

ruslashev left a comment

ruslashev Sep 13, 2023

ruslashev Sep 13, 2023

svenrademakers Sep 14, 2023

svenrademakers Sep 14, 2023 •

edited

Loading

ruslashev Sep 13, 2023

svenrademakers Sep 14, 2023

	const BINARY_MAGIC: u32 = 0xdeadbeef;
	const BINARY_MAGIC: u32 = 0xdef0ad9e;

app_persistency: new implementation #48

app_persistency: new implementation #48

Conversation

svenrademakers commented Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenrademakers Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenrademakers commented Sep 7, 2023 •

edited

Loading

svenrademakers Sep 14, 2023 •

edited

Loading