Re: [RFC PATCH 04/11] rust: block: introduce `kernel::block::bio` module

From: "Andreas Hindborg (Samsung)" <nmi@metaspace.dk>
To: Benno Lossin <benno.lossin@proton.me>
Cc: "Jens Axboe" <axboe@kernel.dk>, "Christoph Hellwig" <hch@lst.de>,
	"Keith Busch" <kbusch@kernel.org>,
	"Damien Le Moal" <Damien.LeMoal@wdc.com>,
	"Hannes Reinecke" <hare@suse.de>,
	lsf-pc@lists.linux-foundation.org,
	rust-for-linux@vger.kernel.org, linux-block@vger.kernel.org,
	"Matthew Wilcox" <willy@infradead.org>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	linux-kernel@vger.kernel.org, gost.dev@samsung.com
Subject: Re: [RFC PATCH 04/11] rust: block: introduce `kernel::block::bio` module
Date: Thu, 11 Jan 2024 13:49:53 +0100	[thread overview]
Message-ID: <87a5pcyqf8.fsf@metaspace.dk> (raw)
In-Reply-To: <-SiJ5paRDIUkH1WEWhGhEjhIgFbSo5PJAvac53bTnBZ5o41DR-kNWZEQBsnKeW1FRJh35siVFRrx54L0M6ebSzl0rzecgcDjqZFGRa9uypE=@proton.me>

Benno Lossin <benno.lossin@proton.me> writes:

>> +/// 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`?

Yes, it feels better with a &Bio reference, thanks.

>> +);
>> +
>> +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?

No, the compiler would not inline into modules without them. I'll check
again if that is still the case.

>
>> +    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`?

Makes sense to rename it, thanks.

>> +        // 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?

Thanks, will change that.

>
>> +    }
>> +
>> +    /// 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 }`.
>

Sure 👍

>> +    }
>> +}
>> +
>> +/// 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)
>

Thanks 👍

>> +    }
>> +}
>> 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`.

Yes, you are right. I will move them to `Segment` and document them
better. They definitely need to be unsafe because they rely on C API
contract that the iterator is not indexing out of bounds.

[...]

>> +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?

Yes, will fix, thanks.

Thanks for the comments!

BR Andreas