Skip to content
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.

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

Make Filesystem owned #31

Open
robin-nitrokey opened this issue Mar 10, 2023 · 2 comments
Open

Make Filesystem owned #31

robin-nitrokey opened this issue Mar 10, 2023 · 2 comments

Comments

@robin-nitrokey
Copy link
Member

Lots of the complexity in Trussed’s store implementation comes from the fact that Filesystem takes mutable references to Allocation and Storage instances, which means that we need three global mutables to get a &mut Filesystem<'static>. I think this could be simplified if Filesystem would take ownership of the allocation and storage. This could be implemented pretty simple:

  • For the storage, implement Storage for &mut Storage and let Filesystem take ownership of S: Storage. This would make it possible to use either an owned storage implementation or a mutable reference to it.
  • For the allocation, I see two options:
    • The simple and cleanest way would be to always take ownership of the allocation and remove the lifetime. The allocation could still be re-used by calling Filesystem::into_inner.
    • If you want to keep the possibility to use a mutable reference to an allocation, we could use a helper enum similar to Cow to also make it possible to use an owned allocation.

What do you think?

@sosthene-nitrokey
Copy link
Contributor

I think the issue with the allocation is that littlefs keeps internal pointers to it, so it cannot be moved, which is why a lot of the "normal" APIs require unsafe or a callback based API.

The way I see it the ideal way to deal with that would be to use Pin pretty much everywhere. This would enable safe APIs.

This seems actually possible in Trussed if the entire trussed Service is Pinned.
It might be worth exploring this would allow even faster iteration and streaming APIs in trussed.

@sosthene-nitrokey
Copy link
Contributor

We indeed can't own the allocation without pinning because the config contains references to the allocation's cache buffers: src/fs.rs#L1189.

I tried to take a stab at what a pinned based API would look like in sosthene-nitrokey/littlefs2/pinned.

I think the overall approach is sound but this requires pinning the filesystem, the files structures and the dir iteration structures. In trussed this would mean pinning the service itself and the backbend, which should be fine, but would require heavy use of pin-project.

We could still have perfectly safe callback-based APIs like we currently have, but the pinning specific APIs would enable more use-cases, especially O(n) iteration and file streaming by keeping the file/directories open.

The main issue of the pinning approach, that would make it much more complex, is that the close function for files and directories needs a reference to the Filesystem, which means that we can't easily close them in the Drop implementation (see here) .

The solutions would be:

  • store a reference to the filesystem, and wrap the allocation in a RefCell like is currently done. This might work for trussed because the Filesystems are allocated statically. If would not work if we ever make them owned by the service, which is the missing part for running tests in parallel after Make the interchange be provided by the runner trussed#100
  • Use an intrinsically linked list in the files and directory structures, so that:
    • They can get access to the filesystem (at the head of the list) when they are dropped
    • When the filesystem is dropped, it can close any remaining files
      Internally they already have an unidirectional linked list so that their state can be updated and so that they are not invalidated when the filesystem is modified while they are live. So we'd just be adding the other direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants