lkml.org 
[lkml]   [2023]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH 04/11] rust: block: introduce `kernel::block::bio` module
    On Wednesday, May 3rd, 2023 at 11:07, Andreas Hindborg <nmi@metaspace.dk> wrote:
    > From: Andreas Hindborg <a.hindborg@samsung.com>
    >
    > Add abstractions for working with `struct bio`.
    >
    > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
    > ---
    > rust/kernel/block.rs | 1 +
    > rust/kernel/block/bio.rs | 93 ++++++++++++++++
    > rust/kernel/block/bio/vec.rs | 181 ++++++++++++++++++++++++++++++++
    > rust/kernel/block/mq/request.rs | 16 +++
    > 4 files changed, 291 insertions(+)
    > create mode 100644 rust/kernel/block/bio.rs
    > create mode 100644 rust/kernel/block/bio/vec.rs
    >
    > diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
    > index 4c93317a568a..1797859551fd 100644
    > --- a/rust/kernel/block.rs
    > +++ b/rust/kernel/block.rs
    > @@ -2,4 +2,5 @@
    >
    > //! Types for working with the block layer
    >
    > +pub mod bio;
    > pub mod mq;
    > diff --git a/rust/kernel/block/bio.rs b/rust/kernel/block/bio.rs
    > new file mode 100644
    > index 000000000000..6e93e4420105
    > --- /dev/null
    > +++ b/rust/kernel/block/bio.rs
    > @@ -0,0 +1,93 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +
    > +//! Types for working with the bio layer.
    > +//!
    > +//! C header: [`include/linux/blk_types.h`](../../include/linux/blk_types.h)
    > +
    > +use core::fmt;
    > +use core::ptr::NonNull;
    > +
    > +mod vec;
    > +
    > +pub use vec::BioSegmentIterator;
    > +pub use vec::Segment;
    > +
    > +/// A wrapper around a `struct bio` pointer
    > +///
    > +/// # Invariants
    > +///
    > +/// First field must alwyas be a valid pointer to a valid `struct bio`.
    > +pub struct Bio<'a>(
    > + NonNull<crate::bindings::bio>,
    > + core::marker::PhantomData<&'a ()>,

    Please make this a struct with named fields. Also this is rather a
    `BioRef`, right? Why can't `&Bio` be used and `Bio` embeds
    `bindings::bio`?

    > +);
    > +
    > +impl<'a> Bio<'a> {
    > + /// Returns an iterator over segments in this `Bio`. Does not consider
    > + /// segments of other bios in this bio chain.
    > + #[inline(always)]

    Why are these `inline(always)`? The compiler should inline them
    automatically?

    > + pub fn segment_iter(&'a self) -> BioSegmentIterator<'a> {
    > + BioSegmentIterator::new(self)
    > + }
    > +
    > + /// Get a pointer to the `bio_vec` off this bio
    > + #[inline(always)]
    > + fn io_vec(&self) -> *const bindings::bio_vec {
    > + // SAFETY: By type invariant, get_raw() returns a valid pointer to a
    > + // valid `struct bio`
    > + unsafe { (*self.get_raw()).bi_io_vec }
    > + }
    > +
    > + /// Return a copy of the `bvec_iter` for this `Bio`
    > + #[inline(always)]
    > + fn iter(&self) -> bindings::bvec_iter {

    Why does this return the bindings iter? Maybe rename to `raw_iter`?

    > + // SAFETY: self.0 is always a valid pointer
    > + unsafe { (*self.get_raw()).bi_iter }
    > + }
    > +
    > + /// Get the next `Bio` in the chain
    > + #[inline(always)]
    > + fn next(&self) -> Option<Bio<'a>> {
    > + // SAFETY: self.0 is always a valid pointer
    > + let next = unsafe { (*self.get_raw()).bi_next };
    > + Some(Self(NonNull::new(next)?, core::marker::PhantomData))

    Missing `INVARIANT` explaining why `next` is valid or null. Also why not
    use `Self::from_raw` here?

    > + }
    > +
    > + /// Return the raw pointer of the wrapped `struct bio`
    > + #[inline(always)]
    > + fn get_raw(&self) -> *const bindings::bio {
    > + self.0.as_ptr()
    > + }
    > +
    > + /// Create an instance of `Bio` from a raw pointer. Does check that the
    > + /// pointer is not null.
    > + #[inline(always)]
    > + pub(crate) unsafe fn from_raw(ptr: *mut bindings::bio) -> Option<Bio<'a>> {
    > + Some(Self(NonNull::new(ptr)?, core::marker::PhantomData))
    > + }
    > +}
    > +
    > +impl core::fmt::Display for Bio<'_> {
    > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
    > + write!(f, "Bio {:?}", self.0.as_ptr())

    this will display as `Bio 0x7ff0654..` I think there should be some
    symbol wrapping the pointer like `Bio(0x7ff0654)` or `Bio { ptr: 0x7ff0654 }`.

    > + }
    > +}
    > +
    > +/// An iterator over `Bio`
    > +pub struct BioIterator<'a> {
    > + pub(crate) bio: Option<Bio<'a>>,
    > +}
    > +
    > +impl<'a> core::iter::Iterator for BioIterator<'a> {
    > + type Item = Bio<'a>;
    > +
    > + #[inline(always)]
    > + fn next(&mut self) -> Option<Bio<'a>> {
    > + if let Some(current) = self.bio.take() {
    > + self.bio = current.next();
    > + Some(current)
    > + } else {
    > + None
    > + }

    Can be rewritten as:
    let current = self.bio.take()?;
    self.bio = current.next();
    Some(cur)

    > + }
    > +}
    > diff --git a/rust/kernel/block/bio/vec.rs b/rust/kernel/block/bio/vec.rs
    > new file mode 100644
    > index 000000000000..acd328a6fe54
    > --- /dev/null
    > +++ b/rust/kernel/block/bio/vec.rs
    > @@ -0,0 +1,181 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +
    > +//! Types for working with `struct bio_vec` IO vectors
    > +//!
    > +//! C header: [`include/linux/bvec.h`](../../include/linux/bvec.h)
    > +
    > +use super::Bio;
    > +use crate::error::Result;
    > +use crate::pages::Pages;
    > +use core::fmt;
    > +use core::mem::ManuallyDrop;
    > +
    > +#[inline(always)]
    > +fn mp_bvec_iter_offset(bvec: *const bindings::bio_vec, iter: &bindings::bvec_iter) -> u32 {
    > + (unsafe { (*bvec_iter_bvec(bvec, iter)).bv_offset }) + iter.bi_bvec_done
    > +}
    > +
    > +#[inline(always)]
    > +fn mp_bvec_iter_page(
    > + bvec: *const bindings::bio_vec,
    > + iter: &bindings::bvec_iter,
    > +) -> *mut bindings::page {
    > + unsafe { (*bvec_iter_bvec(bvec, iter)).bv_page }
    > +}
    > +
    > +#[inline(always)]
    > +fn mp_bvec_iter_page_idx(bvec: *const bindings::bio_vec, iter: &bindings::bvec_iter) -> usize {
    > + (mp_bvec_iter_offset(bvec, iter) / crate::PAGE_SIZE) as usize
    > +}
    > +
    > +#[inline(always)]
    > +fn mp_bvec_iter_len(bvec: *const bindings::bio_vec, iter: &bindings::bvec_iter) -> u32 {
    > + iter.bi_size
    > + .min(unsafe { (*bvec_iter_bvec(bvec, iter)).bv_len } - iter.bi_bvec_done)
    > +}
    > +
    > +#[inline(always)]
    > +fn bvec_iter_bvec(
    > + bvec: *const bindings::bio_vec,
    > + iter: &bindings::bvec_iter,
    > +) -> *const bindings::bio_vec {
    > + unsafe { bvec.add(iter.bi_idx as usize) }
    > +}
    > +
    > +#[inline(always)]
    > +fn bvec_iter_page(
    > + bvec: *const bindings::bio_vec,
    > + iter: &bindings::bvec_iter,
    > +) -> *mut bindings::page {
    > + unsafe { mp_bvec_iter_page(bvec, iter).add(mp_bvec_iter_page_idx(bvec, iter)) }
    > +}
    > +
    > +#[inline(always)]
    > +fn bvec_iter_len(bvec: *const bindings::bio_vec, iter: &bindings::bvec_iter) -> u32 {
    > + mp_bvec_iter_len(bvec, iter).min(crate::PAGE_SIZE - bvec_iter_offset(bvec, iter))
    > +}
    > +
    > +#[inline(always)]
    > +fn bvec_iter_offset(bvec: *const bindings::bio_vec, iter: &bindings::bvec_iter) -> u32 {
    > + mp_bvec_iter_offset(bvec, iter) % crate::PAGE_SIZE
    > +}
    Why are these functions:
    - not marked as `unsafe`?
    - undocumented,
    - free functions.

    Can't these be directly implemented on `Segment<'_>`? If not,
    I think we should find some better way or make them `unsafe`.

    > +
    > +/// A wrapper around a `strutct bio_vec` - a contiguous range of physical memory addresses
    > +///
    > +/// # Invariants
    > +///
    > +/// `bio_vec` must always be initialized and valid
    > +pub struct Segment<'a> {
    > + bio_vec: bindings::bio_vec,
    > + _marker: core::marker::PhantomData<&'a ()>,
    > +}
    > +
    > +impl Segment<'_> {
    > + /// Get he lenght of the segment in bytes
    > + #[inline(always)]
    > + pub fn len(&self) -> usize {
    > + self.bio_vec.bv_len as usize
    > + }
    > +
    > + /// Returns true if the length of the segment is 0
    > + #[inline(always)]
    > + pub fn is_empty(&self) -> bool {
    > + self.len() == 0
    > + }
    > +
    > + /// Get the offset field of the `bio_vec`
    > + #[inline(always)]
    > + pub fn offset(&self) -> usize {
    > + self.bio_vec.bv_offset as usize
    > + }
    > +
    > + /// Copy data of this segment into `page`.
    > + #[inline(always)]
    > + pub fn copy_to_page_atomic(&self, page: &mut Pages<0>) -> Result {
    > + // SAFETY: self.bio_vec is valid and thus bv_page must be a valid
    > + // pointer to a `struct page`. We do not own the page, but we prevent
    > + // drop by wrapping the `Pages` in `ManuallyDrop`.
    > + let our_page = ManuallyDrop::new(unsafe { Pages::<0>::from_raw(self.bio_vec.bv_page) });
    > + let our_map = our_page.kmap_atomic();
    > +
    > + // TODO: Checck offset is within page - what guarantees does `bio_vec` provide?
    > + let ptr = unsafe { (our_map.get_ptr() as *const u8).add(self.offset()) };
    > +
    > + unsafe { page.write_atomic(ptr, self.offset(), self.len()) }
    > + }
    > +
    > + /// Copy data from `page` into this segment
    > + #[inline(always)]
    > + pub fn copy_from_page_atomic(&mut self, page: &Pages<0>) -> Result {
    > + // SAFETY: self.bio_vec is valid and thus bv_page must be a valid
    > + // pointer to a `struct page`. We do not own the page, but we prevent
    > + // drop by wrapping the `Pages` in `ManuallyDrop`.
    > + let our_page = ManuallyDrop::new(unsafe { Pages::<0>::from_raw(self.bio_vec.bv_page) });
    > + let our_map = our_page.kmap_atomic();
    > +
    > + // TODO: Checck offset is within page
    > + let ptr = unsafe { (our_map.get_ptr() as *mut u8).add(self.offset()) };
    > +
    > + unsafe { page.read_atomic(ptr, self.offset(), self.len()) }
    > + }
    > +}
    > +
    > +impl core::fmt::Display for Segment<'_> {
    > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
    > + write!(
    > + f,
    > + "Segment {:?} len: {}",
    > + self.bio_vec.bv_page, self.bio_vec.bv_len
    > + )
    > + }
    > +}
    > +
    > +/// An iterator over `Segment`
    > +pub struct BioSegmentIterator<'a> {
    > + bio: &'a Bio<'a>,
    > + iter: bindings::bvec_iter,
    > +}
    > +
    > +impl<'a> BioSegmentIterator<'a> {
    > + #[inline(always)]
    > + pub(crate) fn new(bio: &'a Bio<'a>) -> BioSegmentIterator<'_> {
    > + Self {
    > + bio,
    > + iter: bio.iter(),
    > + }
    > + }
    > +}
    > +
    > +impl<'a> core::iter::Iterator for BioSegmentIterator<'a> {
    > + type Item = Segment<'a>;
    > +
    > + #[inline(always)]
    > + fn next(&mut self) -> Option<Self::Item> {
    > + if self.iter.bi_size == 0 {
    > + return None;
    > + }
    > +
    > + // Macro
    > + // bio_vec = bio_iter_iovec(bio, self.iter)
    > + // bio_vec = bvec_iter_bvec(bio.bi_io_vec, self.iter);

    Weird comment?

    > + let bio_vec_ret = bindings::bio_vec {
    > + bv_page: bvec_iter_page(self.bio.io_vec(), &self.iter),
    > + bv_len: bvec_iter_len(self.bio.io_vec(), &self.iter),
    > + bv_offset: bvec_iter_offset(self.bio.io_vec(), &self.iter),
    > + };
    > +
    > + // Static C function
    > + unsafe {
    > + bindings::bio_advance_iter_single(
    > + self.bio.get_raw(),
    > + &mut self.iter as *mut bindings::bvec_iter,
    > + bio_vec_ret.bv_len,
    > + )
    > + };
    > +
    > + Some(Segment {
    > + bio_vec: bio_vec_ret,
    > + _marker: core::marker::PhantomData,
    > + })
    > + }
    > +}
    > diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
    > index e95ae3fd71ad..ccb1033b64b6 100644
    > --- a/rust/kernel/block/mq/request.rs
    > +++ b/rust/kernel/block/mq/request.rs
    > @@ -11,6 +11,9 @@ use crate::{
    > };
    > use core::marker::PhantomData;
    >
    > +use crate::block::bio::Bio;
    > +use crate::block::bio::BioIterator;
    > +
    > /// A wrapper around a blk-mq `struct request`. This represents an IO request.
    > pub struct Request<T: Operations> {
    > ptr: *mut bindings::request,
    > @@ -63,6 +66,19 @@ impl<T: Operations> Request<T> {
    > }
    > }
    >
    > + /// Get a wrapper for the first Bio in this request
    > + #[inline(always)]
    > + pub fn bio(&self) -> Option<Bio<'_>> {
    > + let ptr = unsafe { (*self.ptr).bio };
    > + unsafe { Bio::from_raw(ptr) }
    > + }
    > +
    > + /// Get an iterator over all bio structurs in this request
    > + #[inline(always)]
    > + pub fn bio_iter(&self) -> BioIterator<'_> {
    > + BioIterator { bio: self.bio() }
    > + }
    > +
    > /// Get the target sector for the request
    > #[inline(always)]
    > pub fn sector(&self) -> usize {
    > --
    > 2.40.0
    >

    --
    Cheers,
    Benno

    \
     
     \ /
      Last update: 2023-05-08 15:00    [W:6.710 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site