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
