Thread-safe design

[sosthene-nitrokey]

The current bindings are not thread (or embedded tasks) safe. Everything is !Send & !Sync.

Here's how we could have an implementation that could be thread safe:

Create a trait that abstracts over Mutex and Refcell. Implement this trait for Refcell, std::sync::Mutex, critical-section based implementation and the rtic mutex (with the relevant feature flags).
Allow downstreams to implement and provide their own mutexes.

Switch all Refcell usage to this Mutex, this should make everything thread-safe I think.

[Ddystopia]

We think that making Filesystem: Send + !Sync is enough. It already is (de facto), because if you have &mut and & are not allowed to be used in other threads, littlefs will not corrupt anything (We are guaranteed that fs is only accessed in a single thread by not having & in other threads). You don't need &mut for each fs operation, you can convert &mut mutex gives to & and use it in the same thread.

We believe that it would be better design to require &mut more often and use UnsafeCell (to allow lfs to modify data, knowing that data races are impossible in a single thread) instead of RefCell etc. Rust is made to express when &mut and & is required, not to avoid &mut by panicking at runtime. Unless you produce 2 &mut for rust code (should be covered by api design) and not cause data race (guaranteed by !Sync) you can have your methods with just &self most of the time.

If you have Filesystem: Send + !Sync, user can wrap in the mutex and T: Send -> Mutex<T>: Sync (Mutex is a primitive that Rust uses to share !Sync types between threads).

We have our own Mutex (threadx), and someone else might have another. And critical-section for a fs operation is just killing any real time property you might have, making this crate barely usable.

[sosthene-nitrokey]

We think that making Filesystem: Send + !Sync is enough.

Hmm that's right, but it would still not be the best we can do. It would not allow having an iterator open in one thread and reading a file to be done in another thread. The solution I propose would allow that. I'll look into doing it in the meantime.

And critical-section for a fs operation is just killing any real time property you might have, making this crate barely usable.

I am not suggesting to make critical-section be a hard-dependency. It could just be a generic parameter with various implementers, one of them would be based on critical section, but the default would still be the single-threaded Refcell implementation, users that want to use critical section would be able to do so but it would not be the recommended solution.

[Ddystopia]

For what you want to achieve, I believe this is a wrong layer. lfs itself has multi-threading mode and can have mutexes inside. littlefs2 is a wrapper and you are severely limited in what you can do.

[sosthene-nitrokey]

The built-in mutex in upstream lfs is using a very similar approach to what I am suggesting but has the limitation that it needs to be configured at compile time through macros, which is not something that we can easily do in the littlefs2-sys bindings.

Also the custom trait approach I suggest also means that it's possible to have multiple approaches to locking in the same codebase.

[Ddystopia]

By the way, how do you plan to have 2 threads do something with lfs in parallel if it would be behind your mutex?

[Ddystopia]

For example, if you are concerned with lfs exhibiting a data race on read operation, you can either

• Have &mut self
• Have &self, !Sync and UnsafeCell to allow lfs mutating the state without fear for data races + be careful to not alias any &mut .
• Have lfs in thread safe mode, so that all its mutation are safe and encapsulated inside lfs.

What is the difference between thread safe lfs and your proposal?

lfs is the source of trust, we assume that all writes to its data will not cause data races. You can have your &self signatures and Send + Sync, without making the wrapper having too much logic. With having our own mutex around it, you would severely limit yourself, make this wrapper have too much logic and cause problems.

needs to be configured at compile time through macros, which is not something that we can easily do in the littlefs2-sys bindings.

We are currently using our custom bindings for lfs and believe that user must be able to configure lfs with headers. As a suggestion, maybe have unsafe feature flag that will make that thin wrapper assume that lfs is thread safe (it may also have compile time assert from bingen generated bindigns) and user will be responsible for providing mutex for lfs?

[Ddystopia]

The built-in mutex in upstream lfs is using a very similar approach to what I am suggesting

You trait mutex will not allow you to call protected methods in parallel by blocking or panicking, while lfs will eat it all, as it uses mutexes to modify its internal data and doesn't hold them for long (long enough to cause api user to feel it).

[sosthene-nitrokey]

How would you configure the lfs mutex from littlefs2-sys ? That would significantly complexity the littlefs build script, and cargo does not give us the necessary control to do that.

You trait mutex will not allow you to call protected methods in parallel by blocking or panicking, while lfs will eat it all, as it uses mutexes to modify its internal data and doesn't hold them for long.

The mutex in upstream lfs just locks the entire filesystem for every operation that's the same as what I'm suggesting to do

[Ddystopia]

The mutex in upstream lfs just locks the entire filesystem for every operation that's the same as what I'm suggesting to do

But you want to hold some iterators, don't you? I suspect that lfs would just relock it on every next, while your mutex would hold it the whole time.

[sosthene-nitrokey]

No, I would lock on every next too. That's the point compared to having Filesystem: Send + !Sync.

[Ddystopia]

In case of apis that have closures, would you hold the mutex during the closure execution? Because lfs certainly wouldn't, while it can cause panics/dead locks.

[sosthene-nitrokey]

The mutex would only be held during operations like read next close, these don't take a closure.

[Ddystopia]

But how would other operations have an access to lfs, if it is behind the mutex?