[PATCH] rust: Add functions to create and modify an Inode

[PATCH] rust: Add functions to create and modify an Inode

[Ariel Miculas]

Add try_new_inode function associated to SuperBlock which allocates a
new inode and implement a few setter functions for INode.

Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/kernel/fs.rs | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs
index 1ba01629e9cd..58c281123c28 100644
--- a/rust/kernel/fs.rs
+++ b/rust/kernel/fs.rs
@@ -707,6 +707,18 @@ pub struct SuperBlock<T: Type + ?Sized>(
     PhantomData<T>,
 );
 
+impl<T: Type + ?Sized> SuperBlock<T> {
+    /// Create a new inode
+    pub fn try_new_inode(&self) -> Result<INode> {
+        let inode = unsafe { bindings::new_inode(self.0.get()) };
+        if inode.is_null() {
+            return Err(ENOMEM);
+        }
+        // SAFETY: We've just allocated a new inode and checked that it's not null
+        Ok(INode(UnsafeCell::new(unsafe { *inode })))
+    }
+}
+
 /// Wraps the kernel's `struct inode`.
 ///
 /// # Invariants
@@ -716,6 +728,35 @@ pub struct SuperBlock<T: Type + ?Sized>(
 #[repr(transparent)]
 pub struct INode(pub(crate) UnsafeCell<bindings::inode>);
 
+impl INode {
+    /// set the field i_mode of the inode
+    pub fn set_mode(&mut self, mode: u16) {
+        self.0.get_mut().i_mode = mode;
+    }
+    /// set the field i_ino of the inode
+    pub fn set_ino(&mut self, ino: u64) {
+        self.0.get_mut().i_ino = ino;
+    }
+
+    /// set the current time in the atime, ctime and mtime fields of the inode
+    pub fn set_current_time(&mut self) {
+        let time = unsafe { bindings::current_time(self.0.get()) };
+        self.0.get_mut().i_atime = time;
+        self.0.get_mut().i_ctime = time;
+        self.0.get_mut().i_mtime = time;
+    }
+
+    /// set the field i_fop of the inode
+    pub fn set_fop(&mut self, fop: *const bindings::file_operations) {
+        self.0.get_mut().__bindgen_anon_3.i_fop = fop;
+    }
+
+    /// set the field i_op of the inode
+    pub fn set_op(&mut self, fop: *const bindings::inode_operations) {
+        self.0.get_mut().i_op = fop;
+    }
+}
+
 // SAFETY: The type invariants guarantee that `INode` is always ref-counted.
 unsafe impl AlwaysRefCounted for INode {
     fn inc_ref(&self) {
-- 
2.40.0

Re: [PATCH] rust: Add functions to create and modify an Inode

[Miguel Ojeda]

Hi Ariel,

On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> Add try_new_inode function associated to SuperBlock which allocates a
> new inode and implement a few setter functions for INode.

Thanks for the patch!

This seems to be based on the `rust` branch: the filesystem
abstractions we have there are not yet in mainline, so we can't take
this now since `fs.rs` does not exist yet. Please see
https://rust-for-linux.com/branches.

Also, I haven't reviewed the patch, but some quick notes below that
you may find useful.

> +        let inode = unsafe { bindings::new_inode(self.0.get()) };

Please follow the coding conventions / guidelines of the rest of other
code. For instance, we require `// SAFETY` comments on all `unsafe`
blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html
for some details.

> +    /// set the field i_mode of the inode

Ditto, for docs.

> +    pub fn set_fop(&mut self, fop: *const bindings::file_operations) {

If this is intended to be used by modules (as it appears to be since
it is public in a public module), then you will likely need to avoid
referring to `bindings::` so that modules do not need to deal directly
with the C types.

Moreover, the fact that a safe function takes a raw pointer raises
some questions -- what happens if somebody passes a dangling pointer,
for instance?

Cheers,
Miguel

Re: [PATCH] rust: Add functions to create and modify an Inode

[Ariel Miculas]

Thanks Miguel, you've clarified some questions I had about the
branches and the development process.
Regarding the rust branch, it says:
> However, please feel free to submit new PRs for this branch.

Does it make sense to open PRs against the rust branch in github and
ask for feedback that way or should the existing fs.rs from the rust
branch be merged into rust-next instead (and then I could submit
patches)?
For context, I'm trying to add the necessary fs abstractions in order
to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust.
I want to ask for feedback early because I'm not sure the abstractions
I come up with are the right ones.

Regards,
Ariel


On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Ariel,
>
> On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
> >
> > Add try_new_inode function associated to SuperBlock which allocates a
> > new inode and implement a few setter functions for INode.
>
> Thanks for the patch!
>
> This seems to be based on the `rust` branch: the filesystem
> abstractions we have there are not yet in mainline, so we can't take
> this now since `fs.rs` does not exist yet. Please see
> https://rust-for-linux.com/branches.
>
> Also, I haven't reviewed the patch, but some quick notes below that
> you may find useful.
>
> > +        let inode = unsafe { bindings::new_inode(self.0.get()) };
>
> Please follow the coding conventions / guidelines of the rest of other
> code. For instance, we require `// SAFETY` comments on all `unsafe`
> blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html
> for some details.
>
> > +    /// set the field i_mode of the inode
>
> Ditto, for docs.
>
> > +    pub fn set_fop(&mut self, fop: *const bindings::file_operations) {
>
> If this is intended to be used by modules (as it appears to be since
> it is public in a public module), then you will likely need to avoid
> referring to `bindings::` so that modules do not need to deal directly
> with the C types.
>
> Moreover, the fact that a safe function takes a raw pointer raises
> some questions -- what happens if somebody passes a dangling pointer,
> for instance?
>
> Cheers,
> Miguel

Re: [PATCH] rust: Add functions to create and modify an Inode

[Wedson Almeida Filho]

On Thu, 13 Apr 2023 at 16:05, Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> Thanks Miguel, you've clarified some questions I had about the
> branches and the development process.
> Regarding the rust branch, it says:
> > However, please feel free to submit new PRs for this branch.
>
> Does it make sense to open PRs against the rust branch in github and
> ask for feedback that way or should the existing fs.rs from the rust
> branch be merged into rust-next instead (and then I could submit
> patches)?
> For context, I'm trying to add the necessary fs abstractions in order
> to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust.
> I want to ask for feedback early because I'm not sure the abstractions
> I come up with are the right ones.

Please checkout https://github.com/wedsonaf/linux/commits/fs

This is the continuation of the code currently in the `rust` branch.
It allows the creation of a read-only in-memory file system as you can
see in the sample code.

I was working on it with the intent of merging it to `rust` but then
we decided to cut down what was upstreamed to a bare minimum, so I put
this on the back burner while we push other pieces (on which this
depends) upstream first. We also heard from Matthew Wilcox (the page
cache maintainer) on LPC that he has ideas on what the interface
should (or shouldn't) look like. So we'll get him involved [if he has
the time] once we have the dependencies upstreamed.

Some of the code we know will change for the better now that we have pin-init.

Anyway, you're welcome to join us if you're interested.

Cheers,
-Wedson

> Regards,
> Ariel
>
>
> On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > Hi Ariel,
> >
> > On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
> > >
> > > Add try_new_inode function associated to SuperBlock which allocates a
> > > new inode and implement a few setter functions for INode.
> >
> > Thanks for the patch!
> >
> > This seems to be based on the `rust` branch: the filesystem
> > abstractions we have there are not yet in mainline, so we can't take
> > this now since `fs.rs` does not exist yet. Please see
> > https://rust-for-linux.com/branches.
> >
> > Also, I haven't reviewed the patch, but some quick notes below that
> > you may find useful.
> >
> > > +        let inode = unsafe { bindings::new_inode(self.0.get()) };
> >
> > Please follow the coding conventions / guidelines of the rest of other
> > code. For instance, we require `// SAFETY` comments on all `unsafe`
> > blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html
> > for some details.
> >
> > > +    /// set the field i_mode of the inode
> >
> > Ditto, for docs.
> >
> > > +    pub fn set_fop(&mut self, fop: *const bindings::file_operations) {
> >
> > If this is intended to be used by modules (as it appears to be since
> > it is public in a public module), then you will likely need to avoid
> > referring to `bindings::` so that modules do not need to deal directly
> > with the C types.
> >
> > Moreover, the fact that a safe function takes a raw pointer raises
> > some questions -- what happens if somebody passes a dangling pointer,
> > for instance?
> >
> > Cheers,
> > Miguel

Re: [PATCH] rust: Add functions to create and modify an Inode

[Ariel Miculas]

Thanks, Wedson, I wasn't aware of your branch. I'm definitely
interested in helping out, let me know what I can do. I'm new to
rust-for-linux, though, so don't expect too much.

Regards,
Ariel


On Thu, Apr 13, 2023 at 10:41 PM Wedson Almeida Filho
<wedsonaf@gmail.com> wrote:
>
> On Thu, 13 Apr 2023 at 16:05, Ariel Miculas <ariel.miculas@gmail.com> wrote:
> >
> > Thanks Miguel, you've clarified some questions I had about the
> > branches and the development process.
> > Regarding the rust branch, it says:
> > > However, please feel free to submit new PRs for this branch.
> >
> > Does it make sense to open PRs against the rust branch in github and
> > ask for feedback that way or should the existing fs.rs from the rust
> > branch be merged into rust-next instead (and then I could submit
> > patches)?
> > For context, I'm trying to add the necessary fs abstractions in order
> > to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust.
> > I want to ask for feedback early because I'm not sure the abstractions
> > I come up with are the right ones.
>
> Please checkout https://github.com/wedsonaf/linux/commits/fs
>
> This is the continuation of the code currently in the `rust` branch.
> It allows the creation of a read-only in-memory file system as you can
> see in the sample code.
>
> I was working on it with the intent of merging it to `rust` but then
> we decided to cut down what was upstreamed to a bare minimum, so I put
> this on the back burner while we push other pieces (on which this
> depends) upstream first. We also heard from Matthew Wilcox (the page
> cache maintainer) on LPC that he has ideas on what the interface
> should (or shouldn't) look like. So we'll get him involved [if he has
> the time] once we have the dependencies upstreamed.
>
> Some of the code we know will change for the better now that we have pin-init.
>
> Anyway, you're welcome to join us if you're interested.
>
> Cheers,
> -Wedson
>
> > Regards,
> > Ariel
> >
> >
> > On Thu, Apr 13, 2023 at 9:36 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > Hi Ariel,
> > >
> > > On Thu, Apr 13, 2023 at 4:15 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
> > > >
> > > > Add try_new_inode function associated to SuperBlock which allocates a
> > > > new inode and implement a few setter functions for INode.
> > >
> > > Thanks for the patch!
> > >
> > > This seems to be based on the `rust` branch: the filesystem
> > > abstractions we have there are not yet in mainline, so we can't take
> > > this now since `fs.rs` does not exist yet. Please see
> > > https://rust-for-linux.com/branches.
> > >
> > > Also, I haven't reviewed the patch, but some quick notes below that
> > > you may find useful.
> > >
> > > > +        let inode = unsafe { bindings::new_inode(self.0.get()) };
> > >
> > > Please follow the coding conventions / guidelines of the rest of other
> > > code. For instance, we require `// SAFETY` comments on all `unsafe`
> > > blocks. Please see https://docs.kernel.org/rust/coding-guidelines.html
> > > for some details.
> > >
> > > > +    /// set the field i_mode of the inode
> > >
> > > Ditto, for docs.
> > >
> > > > +    pub fn set_fop(&mut self, fop: *const bindings::file_operations) {
> > >
> > > If this is intended to be used by modules (as it appears to be since
> > > it is public in a public module), then you will likely need to avoid
> > > referring to `bindings::` so that modules do not need to deal directly
> > > with the C types.
> > >
> > > Moreover, the fact that a safe function takes a raw pointer raises
> > > some questions -- what happens if somebody passes a dangling pointer,
> > > for instance?
> > >
> > > Cheers,
> > > Miguel

Re: [PATCH] rust: Add functions to create and modify an Inode

[Miguel Ojeda]

On Thu, Apr 13, 2023 at 8:55 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> Thanks Miguel, you've clarified some questions I had about the branches and the development process.

My pleasure!

> Regarding the rust branch, it says:
> > However, please feel free to submit new PRs for this branch.
>
> Does it make sense to open PRs against the rust branch in github and ask for feedback that way or should the existing fs.rs from the rust branch be merged into rust-next instead (and then I could submit patches)?
> For context, I'm trying to add the necessary fs abstractions in order to be able to rewrite lwnfs (https://lwn.net/Articles/57369/) in rust. I want to ask for feedback early because I'm not sure the abstractions I come up with are the right ones.

Yes, in general, please feel free to submit PRs there. The PRs will
not be merged, because the branch is effectively frozen and will
eventually go away / get archived, but you may still find it useful to
get early feedback. The mailing list is meant for mainline development
instead.

As for mainline, anyone can also send patches, of course, as long as
they are ready to be applied (again, in general, e.g. for filesystem
APIs, please wait until things get a bit more settled down as Wedson
mentioned -- when in doubt, you can always ask in Zulip).

Cheers,
Miguel