On Thu, 4 Jan 2024 at 02:14, Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 09:25:04AM -0300, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> >
> > Allow Rust file systems to handle typed and ref-counted inodes.
> >
> > This is in preparation for creating new inodes (for example, to create
> > the root inode of a new superblock), which comes in the next patch in
> > the series.
> >
> > Signed-off-by: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> > ---
> > rust/helpers.c | 7 +++++++
> > rust/kernel/fs.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 4c86fe4a7e05..fe45f8ddb31f 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -25,6 +25,7 @@
> > #include <linux/build_bug.h>
> > #include <linux/err.h>
> > #include <linux/errname.h>
> > +#include <linux/fs.h>
> > #include <linux/mutex.h>
> > #include <linux/refcount.h>
> > #include <linux/sched/signal.h>
> > @@ -144,6 +145,12 @@ struct kunit *rust_helper_kunit_get_current_test(void)
> > }
> > EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test);
> >
> > +off_t rust_helper_i_size_read(const struct inode *inode)
>
> i_size_read returns a loff_t (aka __kernel_loff_t (aka long long)),
> but this returns an off_t (aka __kernel_off_t (aka long)).
>
> Won't that cause truncation issues for files larger than 4GB on some
> architectures?
This is indeed a bug, thanks for catching it! Fixed in v2.
> > +{
> > + return i_size_read(inode);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_i_size_read);
> > +
> > /*
> > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> > * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs
> > index 31cf643aaded..30fa1f312f33 100644
> > --- a/rust/kernel/fs.rs
> > +++ b/rust/kernel/fs.rs
> > @@ -7,9 +7,9 @@
> > //! C headers: [`include/linux/fs.h`](../../include/linux/fs.h)
> >
> > use crate::error::{code::*, from_result, to_result, Error, Result};
> > -use crate::types::Opaque;
> > +use crate::types::{AlwaysRefCounted, Opaque};
> > use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule};
> > -use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin};
> > +use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin, ptr};
> > use macros::{pin_data, pinned_drop};
> >
> > /// Maximum size of an inode.
> > @@ -94,6 +94,55 @@ fn drop(self: Pin<&mut Self>) {
> > }
> > }
> >
> > +/// The number of an inode.
> > +pub type Ino = u64;
> > +
> > +/// A node in the file system index (inode).
> > +///
> > +/// Wraps the kernel's `struct inode`.
> > +///
> > +/// # Invariants
> > +///
> > +/// Instances of this type are always ref-counted, that is, a call to `ihold` ensures that the
> > +/// allocation remains valid at least until the matching call to `iput`.
> > +#[repr(transparent)]
> > +pub struct INode<T: FileSystem + ?Sized>(Opaque<bindings::inode>, PhantomData<T>);
> > +
> > +impl<T: FileSystem + ?Sized> INode<T> {
> > + /// Returns the number of the inode.
> > + pub fn ino(&self) -> Ino {
> > + // SAFETY: `i_ino` is immutable, and `self` is guaranteed to be valid by the existence of a
> > + // shared reference (&self) to it.
> > + unsafe { (*self.0.get()).i_ino }
>
> Is "*self.0.get()" the means by which the Rust bindings get at the
> actual C object?
It gets a pointer to the C object.
self's type is INode, which is a pair. `self.0` get the first element
of the pair, which has type `Opaque<bindings::inode>`. It has a method
called `get` that returns a pointer to the object it wraps.
> (Forgive me, I've barely finished drying the primer coat on my rust-fu.)
>
> > + }
> > +
> > + /// Returns the super-block that owns the inode.
> > + pub fn super_block(&self) -> &SuperBlock<T> {
> > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a
> > + // shared reference (&self) to it.
> > + unsafe { &*(*self.0.get()).i_sb.cast() }
> > + }
> > +
> > + /// Returns the size of the inode contents.
> > + pub fn size(&self) -> i64 {
>
> I'm a little surprised I didn't see a
>
> pub type loff_t = i64
>
> followed by this function returning a loff_t. Or maybe it would be
> better to define it as:
This was suggested by Matthew Wilcox as well, which I did for v2,
though I call it `Offset`.
> struct loff_t(i64);
>
> So that dopey fs developers like me cannot so easily assign a file
> position (bytes) to a pgoff_t (page index) without either supplying an
> actual conversion operator or seeing complaints from the compiler.
We may want to eventually do this, but for now I'm only doing the type alias.
The disadvantage of doing a new type is that we lose all arithmetic
operators as well, though we can redefine them by implementing the
appropriate traits.
Thanks,
-Wedson
