Benno Lossin <benno.lossin@xxxxxxxxx> 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
