Message-ID: <86cd5566-5f1b-434a-9163-2b2d60a759d1@proton.me>
Date: Fri, 22 Mar 2024 23:40:37 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <nmi@...aspace.dk>, Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>, Keith Busch <kbusch@...nel.org>, Damien Le Moal <Damien.LeMoal@....com>, Bart Van Assche <bvanassche@....org>, Hannes Reinecke <hare@...e.de>, "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>
Cc: Andreas Hindborg <a.hindborg@...sung.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Niklas Cassel <Niklas.Cassel@....com>, Greg KH <gregkh@...uxfoundation.org>, Matthew Wilcox <willy@...radead.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, Chaitanya Kulkarni <chaitanyak@...dia.com>, Luis Chamberlain <mcgrof@...nel.org>, Yexuan Yang <1182282462@...t.edu.cn>, Sergio González Collado <sergio.collado@...il.com>, Joel Granados <j.granados@...sung.com>, "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>, Daniel Gomez <da.gomez@...sung.com>, open list <linux-kernel@...r.kernel.org>, "rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>, "lsf-pc@...ts.linux-foundation.org" <lsf-pc@...ts.linux-foundation.org>, "gost.dev@...sung.com" <gost.dev@...sung.com>
Subject: Re: [RFC PATCH 1/5] rust: block: introduce `kernel::block::mq` module

On 3/13/24 12:05, Andreas Hindborg wrote:
> From: Andreas Hindborg <a.hindborg@...sung.com>
> 
> Add initial abstractions for working with blk-mq.
> 
> This patch is a maintained, refactored subset of code originally published by
> Wedson Almeida Filho <wedsonaf@...il.com> [1].
> 
> [1] https://github.com/wedsonaf/linux/tree/f2cfd2fe0e2ca4e90994f96afe268bbd4382a891/rust/kernel/blk/mq.rs
> 
> Cc: Wedson Almeida Filho <wedsonaf@...il.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@...sung.com>
> ---
>   block/blk-mq.c                     |   3 +-
>   include/linux/blk-mq.h             |   1 +
>   rust/bindings/bindings_helper.h    |   2 +
>   rust/helpers.c                     |  45 ++++
>   rust/kernel/block.rs               |   5 +
>   rust/kernel/block/mq.rs            | 131 +++++++++++
>   rust/kernel/block/mq/gen_disk.rs   | 174 +++++++++++++++
>   rust/kernel/block/mq/operations.rs | 346 +++++++++++++++++++++++++++++
>   rust/kernel/block/mq/raw_writer.rs |  60 +++++
>   rust/kernel/block/mq/request.rs    | 182 +++++++++++++++
>   rust/kernel/block/mq/tag_set.rs    | 117 ++++++++++
>   rust/kernel/error.rs               |   5 +
>   rust/kernel/lib.rs                 |   1 +
>   13 files changed, 1071 insertions(+), 1 deletion(-)

Do you think that it's possible to split this into smaller
patches? It would make review a lot easier.

>   create mode 100644 rust/kernel/block.rs
>   create mode 100644 rust/kernel/block/mq.rs
>   create mode 100644 rust/kernel/block/mq/gen_disk.rs
>   create mode 100644 rust/kernel/block/mq/operations.rs
>   create mode 100644 rust/kernel/block/mq/raw_writer.rs
>   create mode 100644 rust/kernel/block/mq/request.rs
>   create mode 100644 rust/kernel/block/mq/tag_set.rs

[...]

> diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
> new file mode 100644
> index 000000000000..4c93317a568a
> --- /dev/null
> +++ b/rust/kernel/block.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Types for working with the block layer

Missing '.'.

> +
> +pub mod mq;
> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
> new file mode 100644
> index 000000000000..08de1cc114ff
> --- /dev/null
> +++ b/rust/kernel/block/mq.rs
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! This module provides types for implementing block drivers that interface the
> +//! blk-mq subsystem.
> +//!
> +//! To implement a block device driver, a Rust module must do the following:
> +//!
> +//! - Implement [`Operations`] for a type `T`

I think it would be nicer to use `Driver` (or `MyBlkDevice`) instead of
`T`.

> +//! - Create a [`TagSet<T>`]
> +//! - Create a [`GenDisk<T>`], passing in the `TagSet` reference
> +//! - Add the disk to the system by calling [`GenDisk::add`]
> +//!
> +//! The types available in this module that have direct C counterparts are:
> +//!
> +//! - The `TagSet` type that abstracts the C type `struct tag_set`.
> +//! - The `GenDisk` type that abstracts the C type `struct gendisk`.
> +//! - The `Request` type that abstracts the C type `struct request`.
> +//!
> +//! Many of the C types that this module abstracts allow a driver to carry
> +//! private data, either embedded in the stuct directly, or as a C `void*`. In
> +//! these abstractions, this data is typed. The types of the data is defined by
> +//! associated types in `Operations`, see [`Operations::RequestData`] for an
> +//! example.
> +//!
> +//! The kernel will interface with the block evice driver by calling the method

Typo: "block evice driver"

> +//! implementations of the `Operations` trait.
> +//!
> +//! IO requests are passed to the driver as [`Request`] references. The
> +//! `Request` type is a wrapper around the C `struct request`. The driver must
> +//! mark start of request processing by calling [`Request::start`] and end of
> +//! processing by calling one of the [`Request::end`], methods. Failure to do so
> +//! can lead to IO failures.

I am unfamiliar with this, what are "IO failures"?
Do you think that it might be better to change the API to use a
callback? So instead of calling start and end, you would do

     request.handle(|req| {
         // do the stuff that would be done between start and end
     });

I took a quick look at the rnull driver and there you are calling
`Request::end_ok` from a different function. So my suggestion might not
be possible, since you really need the freedom.

Do you think that a guard approach might work better? ie `start` returns
a guard that when dropped will call `end` and you need the guard to
operate on the request.

> +//!
> +//! The `TagSet` is responsible for creating and maintaining a mapping between
> +//! `Request`s and integer ids as well as carrying a pointer to the vtable
> +//! generated by `Operations`. This mapping is useful for associating
> +//! completions from hardware with the correct `Request` instance. The `TagSet`
> +//! determines the maximum queue depth by setting the number of `Request`
> +//! instances available to the driver, and it determines the number of queues to
> +//! instantiate for the driver. If possible, a driver should allocate one queue
> +//! per core, to keep queue data local to a core.
> +//!
> +//! One `TagSet` instance can be shared between multiple `GenDisk` instances.
> +//! This can be useful when implementing drivers where one piece of hardware
> +//! with one set of IO resources are represented to the user as multiple disks.
> +//!
> +//! One significant difference between block device drivers implemented with
> +//! these Rust abstractions and drivers implemented in C, is that the Rust
> +//! drivers have to own a reference count on the `Request` type when the IO is
> +//! in flight. This is to ensure that the C `struct request` instances backing
> +//! the Rust `Request` instances are live while the Rust driver holds a
> +//! reference to the `Request`. In addition, the conversion of an ineger tag to

Typo: "of an ineger tag"

> +//! a `Request` via the `TagSet` would not be sound without this bookkeeping.
> +//!
> +//! # ⚠ Note
> +//!
> +//! For Rust block device drivers, the point in time where a request
> +//! is freed and made available for recycling is usualy at the point in time
> +//! when the last `ARef<Request>` is dropped. For C drivers, this event usually
> +//! occurs when `bindings::blk_mq_end_request` is called.
> +//!
> +//! # Example
> +//!
> +//! ```rust
> +//! use kernel::{
> +//!     block::mq::*,
> +//!     new_mutex,
> +//!     prelude::*,
> +//!     sync::{Arc, Mutex},
> +//!     types::{ARef, ForeignOwnable},
> +//! };
> +//!
> +//! struct MyBlkDevice;
> +//!
> +//! #[vtable]
> +//! impl Operations for MyBlkDevice {
> +//!     type RequestData = ();
> +//!     type RequestDataInit = impl PinInit<()>;
> +//!     type QueueData = ();
> +//!     type HwData = ();
> +//!     type TagSetData = ();
> +//!
> +//!     fn new_request_data(
> +//!         _tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,
> +//!     ) -> Self::RequestDataInit {
> +//!         kernel::init::zeroed()
> +//!     }
> +//!
> +//!     fn queue_rq(_hw_data: (), _queue_data: (), rq: ARef<Request<Self>>, _is_last: bool) -> Result {
> +//!         rq.start();
> +//!         rq.end_ok();
> +//!         Ok(())
> +//!     }
> +//!
> +//!     fn commit_rqs(
> +//!         _hw_data: <Self::HwData as ForeignOwnable>::Borrowed<'_>,
> +//!         _queue_data: <Self::QueueData as ForeignOwnable>::Borrowed<'_>,
> +//!     ) {
> +//!     }
> +//!
> +//!     fn complete(rq: &Request<Self>) {
> +//!         rq.end_ok();
> +//!     }
> +//!
> +//!     fn init_hctx(
> +//!         _tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,
> +//!         _hctx_idx: u32,
> +//!     ) -> Result<Self::HwData> {
> +//!         Ok(())
> +//!     }
> +//! }
> +//!
> +//! let tagset: Arc<TagSet<MyBlkDevice>> = Arc::pin_init(TagSet::try_new(1, (), 256, 1))?;
> +//! let mut disk = GenDisk::try_new(tagset, ())?;
> +//! disk.set_name(format_args!("myblk"))?;
> +//! disk.set_capacity_sectors(4096);
> +//! disk.add()?;
> +//!
> +//! # Ok::<(), kernel::error::Error>(())
> +//! ```

This piece of documentation is **really** valuable, thanks a lot for
taking the time to write it.

> +
> +mod gen_disk;
> +mod operations;
> +mod raw_writer;
> +mod request;
> +mod tag_set;
> +
> +pub use gen_disk::GenDisk;
> +pub use operations::Operations;
> +pub use request::{Request, RequestDataRef};
> +pub use tag_set::TagSet;
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> new file mode 100644
> index 000000000000..b7845fc9e39f
> --- /dev/null
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic disk abstraction.
> +//!
> +//! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
> +//! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
> +
> +use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> +use crate::{
> +    bindings, error::from_err_ptr, error::Result, sync::Arc, types::ForeignOwnable,
> +    types::ScopeGuard,
> +};
> +use core::fmt::{self, Write};
> +
> +/// A generic block device
> +///
> +/// # Invariants
> +///
> +///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
> +pub struct GenDisk<T: Operations> {
> +    _tagset: Arc<TagSet<T>>,
> +    gendisk: *mut bindings::gendisk,
> +}
> +
> +// SAFETY: `GenDisk` is an owned pointer to a `struct gendisk` and an `Arc` to a
> +// `TagSet` It is safe to send this to other threads as long as T is Send.
> +unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
> +
> +impl<T: Operations> GenDisk<T> {
> +    /// Try to create a new `GenDisk`
> +    pub fn try_new(tagset: Arc<TagSet<T>>, queue_data: T::QueueData) -> Result<Self> {
> +        let data = queue_data.into_foreign();
> +        let recover_data = ScopeGuard::new(|| {
> +            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
> +            unsafe { T::QueueData::from_foreign(data) };
> +        });
> +
> +        let lock_class_key = crate::sync::LockClassKey::new();
> +
> +        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
> +        let gendisk = from_err_ptr(unsafe {
> +            bindings::__blk_mq_alloc_disk(
> +                tagset.raw_tag_set(),
> +                data.cast_mut(),
> +                lock_class_key.as_ptr(),
> +            )
> +        })?;
> +
> +        const TABLE: bindings::block_device_operations = bindings::block_device_operations {
> +            submit_bio: None,
> +            open: None,
> +            release: None,
> +            ioctl: None,
> +            compat_ioctl: None,
> +            check_events: None,
> +            unlock_native_capacity: None,
> +            getgeo: None,
> +            set_read_only: None,
> +            swap_slot_free_notify: None,
> +            report_zones: None,
> +            devnode: None,
> +            alternative_gpt_sector: None,
> +            get_unique_id: None,
> +            // TODO: Set to THIS_MODULE. Waiting for const_refs_to_static feature to be merged
> +            // https://github.com/rust-lang/rust/issues/119618
> +            owner: core::ptr::null_mut(),
> +            pr_ops: core::ptr::null_mut(),
> +            free_disk: None,
> +            poll_bio: None,
> +        };
> +
> +        // SAFETY: gendisk is a valid pointer as we initialized it above
> +        unsafe { (*gendisk).fops = &TABLE };
> +
> +        recover_data.dismiss();
> +        Ok(Self {
> +            _tagset: tagset,
> +            gendisk,

Missing INVARIANT comment.

> +        })
> +    }
> +
> +    /// Set the name of the device

Missing '.'.

> +    pub fn set_name(&mut self, args: fmt::Arguments<'_>) -> Result {
> +        let mut raw_writer = RawWriter::from_array(
> +            // SAFETY: By type invariant `self.gendisk` points to a valid and initialized instance
> +            unsafe { &mut (*self.gendisk).disk_name },

To create a `&mut` reference, you need exclusive access, it should be
sufficient to add to the invariant that `gendisk` is owned/unique.

> +        );
> +        raw_writer.write_fmt(args)?;
> +        raw_writer.write_char('\0')?;
> +        Ok(())
> +    }

[...]

> +impl<T: Operations> Drop for GenDisk<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
> +        // and initialized instance of `struct gendisk`. As such, `queuedata`
> +        // was initialized by the initializer returned by `try_new` with a call
> +        // to `ForeignOwnable::into_foreign`.

This should also be an invariant of `GenDisk`.

> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
> +
> +        // SAFETY: By type invariant, `self.gendisk` points to a valid and
> +        // initialized instance of `struct gendisk`
> +        unsafe { bindings::del_gendisk(self.gendisk) };
> +
> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
> +        // `ForeignOwnable::from_foreign()` is only called here.
> +        let _queue_data = unsafe { T::QueueData::from_foreign(queue_data) };
> +    }
> +}
> diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
> new file mode 100644
> index 000000000000..53c6ad663208
> --- /dev/null
> +++ b/rust/kernel/block/mq/operations.rs
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! This module provides an interface for blk-mq drivers to implement.
> +//!
> +//! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h)
> +
> +use crate::{
> +    bindings,
> +    block::mq::Request,
> +    error::{from_result, Result},
> +    init::PinInit,
> +    types::{ARef, ForeignOwnable},
> +};
> +use core::{marker::PhantomData, ptr::NonNull};
> +
> +use super::TagSet;
> +
> +/// Implement this trait to interface blk-mq as block devices
> +#[macros::vtable]
> +pub trait Operations: Sized {
> +    /// Data associated with a request. This data is located next to the request
> +    /// structure.
> +    ///
> +    /// To be able to handle accessing this data from interrupt context, this
> +    /// data must be `Sync`.
> +    type RequestData: Sized + Sync;
> +
> +    /// Initializer for `Self::RequestDta`. Used to initialize private data area
> +    /// when requst structure is allocated.
> +    type RequestDataInit: PinInit<Self::RequestData>;

Just to let you know, this dance with the associated types is not needed
any longer. RPITIT (return position impl trait in trait) has been
stabilized in 1.75 and you should be able to just write this:

     fn new_request_data(
         //rq: ARef<Request<Self>>,
         tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,
     ) -> impl PinInit<Self::RequestData>;


> +
> +    /// Data associated with the `struct request_queue` that is allocated for
> +    /// the `GenDisk` associated with this `Operations` implementation.
> +    type QueueData: ForeignOwnable;
> +
> +    /// Data associated with a dispatch queue. This is stored as a pointer in
> +    /// the C `struct blk_mq_hw_ctx` that represents a hardware queue.
> +    type HwData: ForeignOwnable;
> +
> +    /// Data associated with a `TagSet`. This is stored as a pointer in `struct
> +    /// blk_mq_tag_set`.
> +    type TagSetData: ForeignOwnable;
> +
> +    /// Called by the kernel to get an initializer for a `Pin<&mut RequestData>`.
> +    fn new_request_data(
> +        //rq: ARef<Request<Self>>,
> +        tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,

Since you need to use this pattern a lot, it might be a good idea to
introduce a type alias to help with this:

     type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;

What do the others think?

The function would then become (with the RPITIT improvement as well):

     fn new_request_data(
         //rq: ARef<Request<Self>>,
         tagset_data: ForeignBorrowed<'_, Self::TagSetData>,
     ) -> impl PinInit<Self::RequestData>;


> +    ) -> Self::RequestDataInit;
> +
> +    /// Called by the kernel to queue a request with the driver. If `is_last` is
> +    /// `false`, the driver is allowed to defer commiting the request.
> +    fn queue_rq(
> +        hw_data: <Self::HwData as ForeignOwnable>::Borrowed<'_>,
> +        queue_data: <Self::QueueData as ForeignOwnable>::Borrowed<'_>,
> +        rq: ARef<Request<Self>>,
> +        is_last: bool,
> +    ) -> Result;
> +
> +    /// Called by the kernel to indicate that queued requests should be submitted
> +    fn commit_rqs(
> +        hw_data: <Self::HwData as ForeignOwnable>::Borrowed<'_>,
> +        queue_data: <Self::QueueData as ForeignOwnable>::Borrowed<'_>,
> +    );
> +
> +    /// Called by the kernel when the request is completed
> +    fn complete(_rq: &Request<Self>);
> +
> +    /// Called by the kernel to allocate and initialize a driver specific hardware context data
> +    fn init_hctx(
> +        tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,
> +        hctx_idx: u32,
> +    ) -> Result<Self::HwData>;
> +
> +    /// Called by the kernel to poll the device for completed requests. Only
> +    /// used for poll queues.
> +    fn poll(_hw_data: <Self::HwData as ForeignOwnable>::Borrowed<'_>) -> bool {
> +        crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    /// Called by the kernel to map submission queues to CPU cores.
> +    fn map_queues(_tag_set: &TagSet<Self>) {
> +        crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    // There is no need for exit_request() because `drop` will be called.

I think it would be a good idea to mention this in the documentation of
the trait.

> +}

[...]

> diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
> new file mode 100644
> index 000000000000..f7857740af29
> --- /dev/null
> +++ b/rust/kernel/block/mq/raw_writer.rs
> @@ -0,0 +1,60 @@
> +use core::{
> +    fmt::{self, Write},
> +    marker::PhantomData,
> +};
> +
> +/// A mutable reference to a byte buffer where a string can be written into
> +///
> +/// # Invariants
> +///
> +/// * `ptr` is not aliased and valid for read and write for `len` bytes

You probably also want to add "for the duration of `'a`".

> +///
> +pub(crate) struct RawWriter<'a> {
> +    ptr: *mut u8,
> +    len: usize,
> +    _p: PhantomData<&'a ()>,
> +}
> +
> +impl<'a> RawWriter<'a> {
> +    /// Create a new `RawWriter` instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * `ptr` must be valid for read and write for `len` consecutive `u8` elements
> +    /// * `ptr` must not be aliased
> +    unsafe fn new(ptr: *mut u8, len: usize) -> RawWriter<'a> {
> +        Self {
> +            ptr,
> +            len,
> +            _p: PhantomData,
> +        }
> +    }

Since this function is not used (except in the function below), what is
the reason for using a raw pointer?
I looked in your other patches, but did not find another user, so could
this be improved by using mutable references?

> +
> +    pub(crate) fn from_array<const N: usize>(a: &'a mut [core::ffi::c_char; N]) -> RawWriter<'a> {
> +        // SAFETY: the buffer of `a` is valid for read and write for at least `N` bytes
> +        unsafe { Self::new(a.as_mut_ptr().cast::<u8>(), N) }
> +    }
> +}
> +
> +impl Write for RawWriter<'_> {
> +    fn write_str(&mut self, s: &str) -> fmt::Result {
> +        let bytes = s.as_bytes();
> +        let len = bytes.len();
> +        if len > self.len {
> +            return Err(fmt::Error);
> +        }
> +
> +        // SAFETY:
> +        // * `bytes` is valid for reads of `bytes.len()` size because we hold a shared reference to `s`
> +        // * By type invariant `self.ptr` is valid for writes for at lest `self.len` bytes
> +        // * The regions are not overlapping as `ptr` is not aliased
> +        unsafe { core::ptr::copy_nonoverlapping(&bytes[0], self.ptr, len) };
> +
> +        // SAFETY: By type invariant of `Self`, `ptr` is in bounds of an
> +        // allocation. Also by type invariant, the pointer resulting from this
> +        // addition is also in bounds.
> +        self.ptr = unsafe { self.ptr.add(len) };
> +        self.len -= len;
> +        Ok(())
> +    }
> +}
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> new file mode 100644
> index 000000000000..b4dacac5e091
> --- /dev/null
> +++ b/rust/kernel/block/mq/request.rs
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! This module provides a wrapper for the C `struct request` type.
> +//!
> +//! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h)
> +
> +use crate::{
> +    bindings,
> +    block::mq::Operations,
> +    error::{Error, Result},
> +    types::{ARef, AlwaysRefCounted, Opaque},
> +};
> +use core::{ffi::c_void, marker::PhantomData, ops::Deref};
> +
> +/// A wrapper around a blk-mq `struct request`. This represents an IO request.
> +///
> +/// # Invariants
> +///
> +/// * `self.0` is a valid `struct request` created by the C portion of the kernel
> +/// * `self` is reference counted. a call to `req_ref_inc_not_zero` keeps the
> +///    instance alive at least until a matching call to `req_ref_put_and_test`
> +///
> +#[repr(transparent)]
> +pub struct Request<T: Operations>(Opaque<bindings::request>, PhantomData<T>);
> +
> +impl<T: Operations> Request<T> {
> +    /// Create a `&mut Request` from a `bindings::request` pointer
> +    ///
> +    /// # Safety
> +    ///
> +    /// * `ptr` must be aligned and point to a valid `bindings::request` instance
> +    /// * Caller must ensure that the pointee of `ptr` is live and owned
> +    ///   exclusively by caller for at least `'a`
> +    ///
> +    pub(crate) unsafe fn from_ptr_mut<'a>(ptr: *mut bindings::request) -> &'a mut Self {
> +        // SAFETY:
> +        // * The cast is valid as `Self` is transparent.
> +        // * By safety requirements of this function, the reference will be
> +        //   valid for 'a.
> +        unsafe { &mut *(ptr.cast::<Self>()) }
> +    }
> +
> +    /// Get the command identifier for the request
> +    pub fn command(&self) -> u32 {
> +        // SAFETY: By C API contract and type invariant, `cmd_flags` is valid for read
> +        unsafe { (*self.0.get()).cmd_flags & ((1 << bindings::REQ_OP_BITS) - 1) }
> +    }
> +
> +    /// Call this to indicate to the kernel that the request has been issued by the driver

I am a bit confused, is this not supposed to signal that the processing
of the request is going to start now? cf C documentation:

     /**
      * blk_mq_start_request - Start processing a request
      * @rq: Pointer to request to be started
      *
      * Function used by device drivers to notify the block layer that a request
      * is going to be processed now, so blk layer can do proper initializations
      * such as starting the timeout timer.
      */

> +    pub fn start(&self) {
> +        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
> +        // existence of `&mut self` we have exclusive access.
> +        unsafe { bindings::blk_mq_start_request(self.0.get()) };
> +    }
> +
> +    /// Call this to indicate to the kernel that the request has been completed without errors

I dislike the "Call this", it feels redundant, what about "Signal the
block layer that the request has been completed without errors.".

Also with the other functions below.

> +    pub fn end_ok(&self) {
> +        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
> +        // existence of `&mut self` we have exclusive access.
> +        unsafe { bindings::blk_mq_end_request(self.0.get(), bindings::BLK_STS_OK as _) };
> +    }
> +
> +    /// Call this to indicate to the kernel that the request completed with an error
> +    pub fn end_err(&self, err: Error) {
> +        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
> +        // existence of `&mut self` we have exclusive access.
> +        unsafe { bindings::blk_mq_end_request(self.0.get(), err.to_blk_status()) };
> +    }
> +
> +    /// Call this to indicate that the request completed with the status indicated by `status`
> +    pub fn end(&self, status: Result) {
> +        if let Err(e) = status {
> +            self.end_err(e);
> +        } else {
> +            self.end_ok();
> +        }
> +    }
> +
> +    /// Call this to schedule defered completion of the request
> +    pub fn complete(&self) {
> +        // SAFETY: By type invariant, `self.0` is a valid `struct request`
> +        if !unsafe { bindings::blk_mq_complete_request_remote(self.0.get()) } {
> +            T::complete(self);
> +        }
> +    }
> +
> +    /// Get the target sector for the request
> +    #[inline(always)]
> +    pub fn sector(&self) -> usize {
> +        // SAFETY: By type invariant of `Self`, `self.0` is valid and live.
> +        unsafe { (*self.0.get()).__sector as usize }
> +    }
> +
> +    /// Returns an owned reference to the per-request data associated with this
> +    /// request
> +    pub fn owned_data_ref(request: ARef<Self>) -> RequestDataRef<T> {
> +        RequestDataRef::new(request)
> +    }
> +
> +    /// Returns a reference to the oer-request data associated with this request

Typo: "the oer-request"

> +    pub fn data_ref(&self) -> &T::RequestData {
> +        let request_ptr = self.0.get().cast::<bindings::request>();
> +
> +        // SAFETY: `request_ptr` is a valid `struct request` because `ARef` is
> +        // `repr(transparent)`
> +        let p: *mut c_void = unsafe { bindings::blk_mq_rq_to_pdu(request_ptr) };
> +
> +        let p = p.cast::<T::RequestData>();
> +
> +        // SAFETY: By C API contract, `p` is initialized by a call to
> +        // `OperationsVTable::init_request_callback()`. By existence of `&self`
> +        // it must be valid for use as a shared reference.
> +        unsafe { &*p }
> +    }
> +}
> +
> +// SAFETY: It is impossible to obtain an owned or mutable `Request`, so we can

What do you mean by "mutable `Request`"? There is the function to obtain
a `&mut Request`.
Also this should probably be an invariant if you use it as a
justification here.

> +// mark it `Send`.
> +unsafe impl<T: Operations> Send for Request<T> {}
> +
> +// SAFETY: `Request` references can be shared across threads.

Does not explain why that is the case.

> +unsafe impl<T: Operations> Sync for Request<T> {}
> +
> +/// An owned reference to a `Request<T>`
> +#[repr(transparent)]
> +pub struct RequestDataRef<T: Operations> {
> +    request: ARef<Request<T>>,
> +}

Is this extra type really needed? I have not yet taken a look at patch 3,
which uses this. But would it hurt if you implemented the traits
there directly on `ARef<Request<T>>`?

> +
> +impl<T> RequestDataRef<T>
> +where
> +    T: Operations,
> +{
> +    /// Create a new instance.
> +    fn new(request: ARef<Request<T>>) -> Self {
> +        Self { request }
> +    }
> +
> +    /// Get a reference to the underlying request
> +    pub fn request(&self) -> &Request<T> {
> +        &self.request
> +    }
> +}

I really like how you improved the safety comments and documentation. It
was a lot easier to wrap my head around this time (might also me getting
more experienced, but I think it helped a lot).

-- 
Cheers,
Benno

Powered by blists - more mailing lists