[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-fsdevel
Subject:    Re: [RFC PATCH 00/19] Rust abstractions for VFS
From:       Wedson Almeida Filho <wedsonaf () gmail ! com>
Date:       2023-10-31 20:14:08
Message-ID: CANeycqrm1KCH=hOf2WyCg8BVZkX3DnPpaA3srrajgRfz0x=PiQ () mail ! gmail ! com
[Download RAW message or body]

On Sun, 29 Oct 2023 at 17:32, Matthew Wilcox <willy@infradead.org> wrote:
> > impl FileSystem for MyFS {
> >     fn super_params(sb: &NewSuperBlock<Self>) -> Result<SuperParams<Self::Data>>;
> >     fn init_root(sb: &SuperBlock<Self>) -> Result<ARef<INode<Self>>>;
> >     fn read_dir(inode: &INode<Self>, emitter: &mut DirEmitter) -> Result;
> >     fn lookup(parent: &INode<Self>, name: &[u8]) -> Result<ARef<INode<Self>>>;
> >     fn read_folio(inode: &INode<Self>, folio: LockedFolio<'_>) -> Result;
> > }
>
> Does it make sense to describe filesystem methods like this?  As I
> understand (eg) how inodes are laid out, we typically malloc a
>
> foofs_inode {
>         x; y; z;
>         struct inode vfs_inode;
> };
>
> and then the first line of many functions that take an inode is:
>
>         struct ext2_inode_info *ei = EXT2_I(dir);
>
> That feels like unnecessary boilerplate, and might lead to questions like
> "What if I'm passed an inode that isn't an ext2 inode".  Do we want to
> always pass in the foofs_inode instead of the inode?

We're well aligned here. :)

Note that the type is `&INode<Self>` -- `Self` is an alias for the
type implementing this filesystem. For example, in tarfs, the type is
really `&INode<TarFs>`, so it is what you're asking for: the TarFs
filesystem only sees TarFs inodes and superblocks (through the
FileSystem trait, maybe they have to deal with other inodes for other
reasons).

In fact, when you have inode of type `INode<TarFs>`, and you have a call like:

let d = inode.data();

What you get back has the type declared in `TarFs::INodeData`.
Similarly, if you do:

let d = inode.super_block().data();

What you get back has the type declared in `TarFs::Data`.

So all `container_of` calls are hidden away, and we store super-block
data in `s_fs_info` and inode data by having a new struct that
contains the data the fs wants plus a struct inode (this is done with
generics, it's called `INodeWithData`). This is required for type
safety: you always get the right type. If someone changes the type in
one place but forgets to change it in another place, they'll get a
compilation error.

> Also, I see you're passing an inode to read_dir.  Why did you decide to
> do that?  There's information in the struct file that's either necessary
> or useful to have in the filesystem.  Maybe not in toy filesystems, but eg
> network filesystems need user credentials to do readdir, which are stored
> in struct file.  Block filesystems store readahead data in struct file.

Because the two file systems we have don't use anything from `struct
file` beyond the inode.

Passing a `file` to `read_dir` would require us to introduce an
unnecessary abstraction that no one uses, which we've been told not to
do.

There is no technical reason that makes it impractical though. We can
add it when the need arises.

[prev in list] [next in list] [prev in thread] [next in thread]