Message-ID: <-SiJ5paRDIUkH1WEWhGhEjhIgFbSo5PJAvac53bTnBZ5o41DR-kNWZEQBsnKeW1FRJh35siVFRrx54L0M6ebSzl0rzecgcDjqZFGRa9uypE=@proton.me>
Date:   Mon, 08 May 2023 12:58:45 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Andreas Hindborg <nmi@...aspace.dk>
Cc:     Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
        Keith Busch <kbusch@...nel.org>,
        Damien Le Moal <Damien.LeMoal@....com>,
        Hannes Reinecke <hare@...e.de>,
        lsf-pc@...ts.linux-foundation.org, rust-for-linux@...r.kernel.org,
        linux-block@...r.kernel.org,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Matthew Wilcox <willy@...radead.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org, gost.dev@...sung.com
Subject: Re: [RFC PATCH 04/11] rust: block: introduce `kernel::block::bio` module

On Wednesday, May 3rd, 2023 at 11:07, Andreas Hindborg <nmi@...aspace.dk> wrote:
> From: Andreas Hindborg <a.hindborg@...sung.com>
> 
> Add abstractions for working with `struct bio`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@...sung.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

Powered by blists - more mailing lists