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

Filesystems and Ramfs #459

Closed
wants to merge 58 commits into from

Conversation

niklasmohrin
Copy link

@niklasmohrin niklasmohrin commented Jul 28, 2021

Closes #369

WIP

Hey everyone, we are now starting to shift our focus away from coding for the lecture and can start working on your requirements. :)

There are some TODOs already

  • rebase our branch
  • split into multiple PRs?
  • FileSystemRegistration (+ macro for module?)
  • investigating the static mut we have for struct filesystem_type (maybe rather static UnsafeCell?)
  • investigate duplication of *Operations and consider vtable macro from Implement a #[vtable] macro #322
  • consider renaming FileSystemBase -> FileSystemOperations
  • investigate enhancement to declare_*_operations macro to allow for something like declare_file_operations(stat=simple_statfs) (seems very hard, but maybe we can come up with a nice way somehow); or other suggestions for how to get rid of the huge boilerplate for things where the C implementation just writes a function into the vtable, whereas we need to enumerate every argument
  • benchmarks (we have very simple ones already, we would have to see how to run them reliably / in CI?)
  • fix clippy warnings and errors
  • discuss pinning of kernel data structures (when working on a FAT impl, we needed a mutex in a private / in an impl FileOperations, but failed to come up with an elegant solution (elegant meaning no unsafe)). I saw that Pin already implements PointerWrapper, but we will have to see how it all plays out - maybe we will even ignore pinning for now
  • more things, including stuff I forgot

@foxhlchen We gladly invite you to work this out together :)

Reviewers: There is probably no point in reviewing until we have rebased at least :^)

niklasmohrin and others added 30 commits June 10, 2021 22:59
Co-authored-by: Benedikt Weber <44091658+bewee@users.noreply.github.com>
This commit is the merged version with upstream, fix coming later
This mainly includes:
- `sed -i 's/KernelResult/Result/g' rust/kernel/**/*.rs`
- fixing some `CStr`s
- adding small things I missed when rebasing

I tested with the VM and everything seems to work. Namely,

```shell
$ ./run.sh
$ echo a > bs3/a
$ cat bs3/a
a
```

worked, so it should all be good. I think there are some new compiler
warnings, we'll have to see through them some other time :^)
I converted some names to their intended casing and put some underscores
before unused variable names. I also added some Rust style where I saw
something odd.
This also replaces some uses of bindings::mode_t with the Mode type from
the kernel crate.

There are still some things that require another indirection to be
ported to Rust types, but most of the code we have now uses the new
type. I think that wrapping the `i_mapping` field could be difficult to
get right.
This also includes the first wrapper, `generic_file_llseek`.
This method should be preferred over accessing the member variable
directly, because it enforces an implementation of
`file_operations::FileOperations`.

For now, we added a `FileOpenAdapter` that doesn't return anything,
because we currently don't need anything more complicated.
This commit adds the call to `Inode::set_file_operations`, the
`FileOperations` implementation is not done yet though.
Previously, the kernel crate implemented `read_iter` in terms of `read`.
Now, the default implementation of the trait method does just that, but
it can also be overridden.
taminob and others added 22 commits June 10, 2021 23:05
This moves the shared behavior of the `build` method from the file
operations vtable into its own trait, so that other types can also
implement it. Namely, we added the unit struct `SimpleDirOperations`
that returns a ref to the C global.

This could be expanded, I can see a general `trait BuildVtable<Table>`
and a macro like

    declare_c_vtable!(
        SimpleDirOperations,
        bindings::file_operations,
        bindings::simple_dir_operations,
    );

that expands to

    struct SimpleDirOperations;
    impl BuildVtable<bindings::file_operations> for SimpleDirOperations {
        fn build() -> &'static bindings::file_operations {
            &bindings::simple_dir_operations
        }
    }

so that we can easily make these constants accessible. The downside is,
that the interface for the caller is more abstract / complicated, since
the vtable struct has to be constructed on the calling site

    .set_file_operations::<FileOperationsVtable<NopFileOpenAdapter, Bs2RamfsFileOps>>();

We could however aid with some helper methods

    .set_file_ops_from_trait::<Bs2RamfsFileOps>();
    // ...
    fn set_file_ops_from_trait<Ops: FileOperations<A::Arg>, A: FileOpenAdapter = NopFileOpenAdapter>() { ... }

Implementing the `BuildVtable` trait for all implementors of
`FileOperations` is not possible, because there might be multiple types
`A` that satisfy the requirements, thus making a definite choice
impossible.
This macro mirrors the `std::dbg` macro. To view the output, you might
have to set the log level to "info" using `dmesg -n 6`.
Partly abridged from the fat branch

Co-authored-by: Tamino Bauknecht <dev@tb6.eu>
Recent C implementations of ramfs don't implement it either
Co-authored-by: Niklas Mohrin <niklas.mohrin@student.hpi.uni-potsdam.de>
I really tried to write a good FileSystemFlags abstraction. But that
damn const static stuff... argh... So, I guess we have to live with this :/
We already have a macro for flag types in
ec61599

We could also try to make this as one trait that gives you free impls of
BitAnd and BitOr etc., and maybe add a derive(BitFlag) or so (we would
need one method that returns self.0 on these types). We could then also
have an associated type with the trait, so that we can also get rid of
ModeInt. I don't know how much we loose on the "const" side of things
with a trait though.
@ojeda
Copy link
Member

ojeda commented Jul 28, 2021

rebase our branch

When you do it, please follow Documentation/process/submitting-patches.rst for the commit messages.

There seems to be also a few files that shouldn't be in the PR, e.g. the b one, plus some changes too, e.g. the ones to README.md.

split into multiple PRs?

Definitely :)

#[macro_export]
macro_rules! declare_fs_type {
($T:ty, $S:ident) => {
static mut $S: $crate::bindings::file_system_type = $crate::bindings::file_system_type {
Copy link
Member

Choose a reason for hiding this comment

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

Where is this static mutated? Is it implicitly synchronized, or does it need a mutex?

Copy link
Author

Choose a reason for hiding this comment

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

We need a *mut bindings::file_system_type for mount_nodev and I think we faced problems when this was just a static (are normal statics in read-only memory?). In the C impl, this was just a (non-const) global variable and we figured this would be the closest match. The vtable approach from FileOperations doesn't work either because the fields are mutated in C code

Maybe we are missing something though

Choose a reason for hiding this comment

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

IIUC, This is only used for fs registration. It will not be mutated later from fs side.

@foxhlchen
Copy link

Great work!
Finally, we've got the fs part off the ground.

I'm more than happy to work together. I was moving really slow on my own and I'm gonna stop that work and come here to work on this.

What is your workflow?? Is there any work that I can immediately start working on it??

@niklasmohrin
Copy link
Author

@foxhlchen We would start with the rebase in the next couple of days. We usually work in pairs and would meet up every week in a set spot, since the lecture is over (and exams have started), we didn't stick to the schedule the last two weeks or so. I think that we would be fine with going back to that workflow and working in groups of 1-3. I was thinking we could move conversations to a zulip thread and maybe discuss progress in a jitsi or similar if that is okay for you (we are not on zulip yet, gonna request an invitation later today)

I am not really sure which existing parts need work right now, but afaik no one has started working on a FileSystemRegistration yet and since you already have module_fs! figured out, that might be a good fit. I am gonna invite you to our fork in a minute!

@foxhlchen
Copy link

@foxhlchen We would start with the rebase in the next couple of days.

Yes, In my opinion, this should be done first. Some have been changed since you've forked the code (e.g. Error). We need to use what kernel crate provides and catch up with the mainline.

We usually work in pairs and would meet up every week in a set spot, since the lecture is over (and exams have started), we didn't stick to the schedule the last two weeks or so. I think that we would be fine with going back to that workflow and working in groups of 1-3. I was thinking we could move conversations to a zulip thread and maybe discuss progress in a jitsi or similar if that is okay for you (we are not on zulip yet, gonna request an invitation later today)

Sounds great! We can have regular meetups or meetups as needed. Apart from that, the email also works for me foxhlchen@gmail.com as well.

Is jitsi a zoom like product? I think that will work fine for me.

I am not really sure which existing parts need work right now, but afaik no one has started working on a FileSystemRegistration yet and since you already have module_fs! figured out, that might be a good fit. I am gonna invite you to our fork in a minute!

Ok, I will look into it!

@niklasmohrin
Copy link
Author

Sadly, we never got around to properly finishing this, closing :)

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

Successfully merging this pull request may close these issues.

ramfs as an example/benchmark
6 participants