Re: [RFC PATCH 16/19] rust: fs: allow file systems backed by a block device

[Wedson Almeida Filho <wedsonaf@gmail.com>]

On Sat, Oct 21, 2023 at 01:39:20PM +0000, Benno Lossin wrote:
> On 18.10.23 14:25, Wedson Almeida Filho wrote:
> > +    /// Reads a block from the block device.
> > +    #[cfg(CONFIG_BUFFER_HEAD)]
> > +    pub fn bread(&self, block: u64) -> Result<ARef<buffer::Head>> {
> > +        // Fail requests for non-blockdev file systems. This is a compile-time check.
> > +        match T::SUPER_TYPE {
> > +            Super::BlockDev => {}
> > +            _ => return Err(EIO),
> > +        }
> 
> Would it make sense to use `build_error` instead of returning an error
> here?

Yes, I changed these to `build_errors` in v2.

> Also, do you think that separating this into a trait, `BlockDevFS` would
> make sense?

We actually have several types; we happen to only support two cases now, but will add more over time.

> > +
> > +        // SAFETY: This function is only valid after the `NeedsInit` typestate, so the block size
> > +        // is known and the superblock can be used to read blocks.
> 
> Stale SAFETY comment, there are not typestates in this patch?

Fixed in v2.

> 
> > +        let ptr =
> > +            ptr::NonNull::new(unsafe { bindings::sb_bread(self.0.get(), block) }).ok_or(EIO)?;
> > +        // SAFETY: `sb_bread` returns a referenced buffer head. Ownership of the increment is
> > +        // passed to the `ARef` instance.
> > +        Ok(unsafe { ARef::from_raw(ptr.cast()) })
> > +    }
> > +
> > +    /// Reads `size` bytes starting from `offset` bytes.
> > +    ///
> > +    /// Returns an iterator that returns slices based on blocks.
> > +    #[cfg(CONFIG_BUFFER_HEAD)]
> > +    pub fn read(
> > +        &self,
> > +        offset: u64,
> > +        size: u64,
> > +    ) -> Result<impl Iterator<Item = Result<buffer::View>> + '_> {
> > +        struct BlockIter<'a, T: FileSystem + ?Sized> {
> > +            sb: &'a SuperBlock<T>,
> > +            next_offset: u64,
> > +            end: u64,
> > +        }
> > +        impl<'a, T: FileSystem + ?Sized> Iterator for BlockIter<'a, T> {
> > +            type Item = Result<buffer::View>;
> > +
> > +            fn next(&mut self) -> Option<Self::Item> {
> > +                if self.next_offset >= self.end {
> > +                    return None;
> > +                }
> > +
> > +                // SAFETY: The superblock is valid and has had its block size initialised.
> > +                let block_size = unsafe { (*self.sb.0.get()).s_blocksize };
> > +                let bh = match self.sb.bread(self.next_offset / block_size) {
> > +                    Ok(bh) => bh,
> > +                    Err(e) => return Some(Err(e)),
> > +                };
> > +                let boffset = self.next_offset & (block_size - 1);
> > +                let bsize = core::cmp::min(self.end - self.next_offset, block_size - boffset);
> > +                self.next_offset += bsize;
> > +                Some(Ok(buffer::View::new(bh, boffset as usize, bsize as usize)))
> > +            }
> > +        }
> > +        Ok(BlockIter {
> > +            sb: self,
> > +            next_offset: offset,
> > +            end: offset.checked_add(size).ok_or(ERANGE)?,
> > +        })
> > +    }
> >   }
> > 
> >   /// Required superblock parameters.
> > @@ -511,6 +591,70 @@ pub struct SuperParams<T: ForeignOwnable + Send + Sync> {
> >   #[repr(transparent)]
> >   pub struct NewSuperBlock<T: FileSystem + ?Sized>(bindings::super_block, PhantomData<T>);
> > 
> > +impl<T: FileSystem + ?Sized> NewSuperBlock<T> {
> > +    /// Reads sectors.
> > +    ///
> > +    /// `count` must be such that the total size doesn't exceed a page.
> > +    pub fn sread(&self, sector: u64, count: usize, folio: &mut UniqueFolio) -> Result {
> > +        // Fail requests for non-blockdev file systems. This is a compile-time check.
> > +        match T::SUPER_TYPE {
> > +            // The superblock is valid and given that it's a blockdev superblock it must have a
> > +            // valid `s_bdev`.
> > +            Super::BlockDev => {}
> > +            _ => return Err(EIO),
> > +        }
> > +
> > +        crate::build_assert!(count * (bindings::SECTOR_SIZE as usize) <= bindings::PAGE_SIZE);
> 
> Maybe add an error message that explains why this is not ok?
> 
> > +
> > +        // Read the sectors.
> > +        let mut bio = bindings::bio::default();
> > +        let bvec = Opaque::<bindings::bio_vec>::uninit();
> > +
> > +        // SAFETY: `bio` and `bvec` are allocated on the stack, they're both valid.
> > +        unsafe {
> > +            bindings::bio_init(
> > +                &mut bio,
> > +                self.0.s_bdev,
> > +                bvec.get(),
> > +                1,
> > +                bindings::req_op_REQ_OP_READ,
> > +            )
> > +        };
> > +
> > +        // SAFETY: `bio` was just initialised with `bio_init` above, so it's safe to call
> > +        // `bio_uninit` on the way out.
> > +        let mut bio =
> > +            ScopeGuard::new_with_data(bio, |mut b| unsafe { bindings::bio_uninit(&mut b) });
> > +
> > +        // SAFETY: We have one free `bvec` (initialsied above). We also know that size won't exceed
> > +        // a page size (build_assert above).
> 
> I think you should move the `build_assert` above this line.

Sure, moved in v2.

Thanks,
-Wedson