Re: [RFC PATCH 05/19] rust: fs: introduce `INode<T>`

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Oct 18, 2023 at 09:25:04AM -0300, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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@microsoft.com>
> ---
>  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?

> +{
> +	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?

(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:

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.

> +        // SAFETY: `self` is guaranteed to be valid by the existence of a shared reference.
> +        unsafe { bindings::i_size_read(self.0.get()) }

It's confusing that rust_i_size_read returns a long but on the rust side,
INode::size returns an i64.

--D

> +    }
> +}
> +
> +// SAFETY: The type invariants guarantee that `INode` is always ref-counted.
> +unsafe impl<T: FileSystem + ?Sized> AlwaysRefCounted for INode<T> {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::ihold(self.0.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::iput(obj.cast().as_ptr()) }
> +    }
> +}
> +
>  /// A file system super block.
>  ///
>  /// Wraps the kernel's `struct super_block`.
> -- 
> 2.34.1
> 
>