Wedson Almeida Filho <wedsonaf@xxxxxxxxx> writes:
[...]
>
> +/// An inode that is locked and hasn't been initialised yet.
> +#[repr(transparent)]
> +pub struct NewINode<T: FileSystem + ?Sized>(ARef<INode<T>>);
> +
> +impl<T: FileSystem + ?Sized> NewINode<T> {
> + /// Initialises the new inode with the given parameters.
> + pub fn init(self, params: INodeParams) -> Result<ARef<INode<T>>> {
> + // SAFETY: This is a new inode, so it's safe to manipulate it mutably.
> + let inode = unsafe { &mut *self.0 .0.get() };
Perhaps it would make sense with a `UniqueARef` that guarantees
uniqueness, in line with `alloc::UniqueRc`?
[...]
>
> +impl<T: FileSystem + ?Sized> SuperBlock<T> {
> + /// Tries to get an existing inode or create a new one if it doesn't exist yet.
> + pub fn get_or_create_inode(&self, ino: Ino) -> Result<Either<ARef<INode<T>>, NewINode<T>>> {
> + // SAFETY: The only initialisation missing from the superblock is the root, and this
> + // function is needed to create the root, so it's safe to call it.
> + let inode =
> + ptr::NonNull::new(unsafe { bindings::iget_locked(self.0.get(), ino) }).ok_or(ENOMEM)?;
I can't parse this safety comment properly.
> +
> + // SAFETY: `inode` is valid for read, but there could be concurrent writers (e.g., if it's
> + // an already-initialised inode), so we use `read_volatile` to read its current state.
> + let state = unsafe { ptr::read_volatile(ptr::addr_of!((*inode.as_ptr()).i_state)) };
> + if state & u64::from(bindings::I_NEW) == 0 {
> + // The inode is cached. Just return it.
> + //
> + // SAFETY: `inode` had its refcount incremented by `iget_locked`; this increment is now
> + // owned by `ARef`.
> + Ok(Either::Left(unsafe { ARef::from_raw(inode.cast()) }))
> + } else {
> + // SAFETY: The new inode is valid but not fully initialised yet, so it's ok to create a
> + // `NewINode`.
> + Ok(Either::Right(NewINode(unsafe {
> + ARef::from_raw(inode.cast())
I would suggest making the destination type explicit for the cast.
> + })))
> + }
> + }
> +}
> +
> /// Required superblock parameters.
> ///
> /// This is returned by implementations of [`FileSystem::super_params`].
> @@ -215,41 +345,28 @@ impl<T: FileSystem + ?Sized> Tables<T> {
> sb.0.s_blocksize = 1 << sb.0.s_blocksize_bits;
> sb.0.s_flags |= bindings::SB_RDONLY;
>
> - // The following is scaffolding code that will be removed in a subsequent patch. It is
> - // needed to build a root dentry, otherwise core code will BUG().
> - // SAFETY: `sb` is the superblock being initialised, it is valid for read and write.
> - let inode = unsafe { bindings::new_inode(&mut sb.0) };
> - if inode.is_null() {
> - return Err(ENOMEM);
> - }
> -
> - // SAFETY: `inode` is valid for write.
> - unsafe { bindings::set_nlink(inode, 2) };
> -
> - {
> - // SAFETY: This is a newly-created inode. No other references to it exist, so it is
> - // safe to mutably dereference it.
> - let inode = unsafe { &mut *inode };
> - inode.i_ino = 1;
> - inode.i_mode = (bindings::S_IFDIR | 0o755) as _;
> -
> - // SAFETY: `simple_dir_operations` never changes, it's safe to reference it.
> - inode.__bindgen_anon_3.i_fop = unsafe { &bindings::simple_dir_operations };
> + // SAFETY: The callback contract guarantees that `sb_ptr` is a unique pointer to a
> + // newly-created (and initialised above) superblock.
> + let sb = unsafe { &mut *sb_ptr.cast() };
Again, I would suggest an explicit destination type for the cast.
> + let root = T::init_root(sb)?;
>
> - // SAFETY: `simple_dir_inode_operations` never changes, it's safe to reference it.
> - inode.i_op = unsafe { &bindings::simple_dir_inode_operations };
> + // Reject root inode if it belongs to a different superblock.
I am curious how this would happen?
BR Andreas
