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

[SC64][SW] Add inital fuse support #97

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Maschell
Copy link

@Maschell Maschell commented Nov 5, 2024

PoC/WIP implemenation of fuse support via fuse-mt.

Current limitations and notes:

  • This was basically the first doing something in rust, I might have done weird things in the code
  • Read only support
  • Only unix based operation system are supported. fuser/fuse-mt doesn't support windows. There is winfsp_wrs though, but I didn't look at it yet.
  • Everything is pretty slow. The biggest bottleneck was the getattr implementation in directory with many files. (I'm guessing fatfs looks through all files in a directory until it finds the requested one). Caching the file stat entries on a RO filesystem was pretty easy and it improved the permomance. However, it still feels pretty slow.
  • Some functions are not implemented (properly)

I tested this on WSL2 (mounting the summercart64 via usbipd to WSL2) and accessing the filesystem via \\wsl.localhost\

For now I'll probably take a break from this PR before I will implement write support, so feel free to pick up my work and improve/fix it.

Comment on lines +8 to +9
sw/deployer/.idea/
.idea/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use **/.idea instead of creating specific entries. Please keep the style consistent by moving it above !.. entry, and don't forget to sort entries alphabetically.

@@ -30,6 +30,11 @@ rust-ini = "0.18.0"
serial2 = "0.2.26"
serialport = "4.4.0"

[target.'cfg(unix)'.dependencies]
Copy link
Owner

@Polprzewodnikowy Polprzewodnikowy Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything preventing this code from working on macOS?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding "unix" does also include macOS? I didn't test it yet though

#[cfg(unix)]
Fuse {
/// Path to the directory
mount_path: PathBuf,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the name of parameter to path and explain what it does in the description one line above. "Path to the directory" tells noting in this context.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all your changes to a separate file (fuse.rs). Fuse layer can live separately from FatFs layer.

[target.'cfg(unix)'.dependencies]
fuse_mt = "0.6.1"
libc = "0.2.161"
log = "0.4.22"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this even needed? println! is not enough?

Comment on lines +365 to +369
// TODO, see https://github.com/wfraser/fuse-mt/issues/29
#[cfg(unix)]
unsafe impl Send for FatFs {}
#[cfg(unix)]
unsafe impl Sync for FatFs {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that. You're using a multi-threaded fuse library just to sidestep the problem by setting 1 max thread and hack an object to be compliant with the fuse_mt API. This is a really bad practice and it will lead to problems down the line.
While FatFs could be used in multi-threaded setting, the connection between PC and SC64 will always be limited by single thread no matter what.
Was the use of fuse_mt dictated by the fact, that it exposes "simple" API with path names instead of inodes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the main motivation for using fuse_mt was the simple path based API.
Even if the connection between PC and SC64 is always single thread, we probably still want to be able to list a dir while reading/writing a big file. This should be already possible with the current implementation because read/write happen in a seperate thread.

I do agree that the current hack to get FatFs working with the fuse_mt API is not ideal. I don't know enough about rust to resolve it properly though. I am open for any suggestions

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

Successfully merging this pull request may close these issues.

2 participants