Re: [PATCH v6 17/18] rust: block: add remote completion to `Request`

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Andreas,

> On 22 Aug 2025, at 09:14, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Allow users of rust block device driver API to schedule completion of
> requests via `blk_mq_complete_request_remote`.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> drivers/block/rnull/rnull.rs       |  9 +++++++++
> rust/kernel/block/mq.rs            |  6 ++++++
> rust/kernel/block/mq/operations.rs | 19 +++++++++++++++----
> rust/kernel/block/mq/request.rs    | 17 +++++++++++++++++
> 4 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
> index 8255236bc550..a19c55717c4f 100644
> --- a/drivers/block/rnull/rnull.rs
> +++ b/drivers/block/rnull/rnull.rs
> @@ -82,4 +82,13 @@ fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Res
>     }
> 
>     fn commit_rqs(_queue_data: ()) {}
> +
> +    fn complete(rq: ARef<mq::Request<Self>>) {
> +        mq::Request::end_ok(rq)
> +            .map_err(|_e| kernel::error::code::EIO)
> +            // We take no refcounts on the request, so we expect to be able to
> +            // end the request. The request reference must be unique at this
> +            // point, and so `end_ok` cannot fail.
> +            .expect("Fatal error - expected to be able to end request");
> +    }
> }
> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
> index 6e546f4f3d1c..c0ec06b84355 100644
> --- a/rust/kernel/block/mq.rs
> +++ b/rust/kernel/block/mq.rs
> @@ -77,6 +77,12 @@
> //!     }
> //!
> //!     fn commit_rqs(_queue_data: ()) {}
> +//!
> +//!     fn complete(rq: ARef<Request<Self>>) {
> +//!         Request::end_ok(rq)
> +//!             .map_err(|_e| kernel::error::code::EIO)
> +//!             .expect("Fatal error - expected to be able to end request");
> +//!     }
> //! }
> //!
> //! let tagset: Arc<TagSet<MyBlkDevice>> =
> diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
> index 6fb256f55acc..0fece577de7c 100644
> --- a/rust/kernel/block/mq/operations.rs
> +++ b/rust/kernel/block/mq/operations.rs
> @@ -42,6 +42,9 @@ fn queue_rq(
>     /// Called by the kernel to indicate that queued requests should be submitted.
>     fn commit_rqs(queue_data: ForeignBorrowed<'_, Self::QueueData>);
> 
> +    /// Called by the kernel when the request is completed.
> +    fn complete(rq: ARef<Request<Self>>);
> +
>     /// Called by the kernel to poll the device for completed requests. Only
>     /// used for poll queues.
>     fn poll() -> bool {
> @@ -143,13 +146,21 @@ impl<T: Operations> OperationsVTable<T> {
>         T::commit_rqs(queue_data)
>     }
> 
> -    /// This function is called by the C kernel. It is not currently
> -    /// implemented, and there is no way to exercise this code path.
> +    /// This function is called by the C kernel. A pointer to this function is
> +    /// installed in the `blk_mq_ops` vtable for the driver.
>     ///
>     /// # Safety
>     ///
> -    /// This function may only be called by blk-mq C infrastructure.
> -    unsafe extern "C" fn complete_callback(_rq: *mut bindings::request) {}
> +    /// This function may only be called by blk-mq C infrastructure. `rq` must
> +    /// point to a valid request that has been marked as completed. The pointee
> +    /// of `rq` must be valid for write for the duration of this function.
> +    unsafe extern "C" fn complete_callback(rq: *mut bindings::request) {
> +        // SAFETY: This function can only be dispatched through
> +        // `Request::complete`. We leaked a refcount then which we pick back up
> +        // now.
> +        let aref = unsafe { Request::aref_from_raw(rq) };
> +        T::complete(aref);
> +    }
> 
>     /// This function is called by the C kernel. A pointer to this function is
>     /// installed in the `blk_mq_ops` vtable for the driver.
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> index 3848cfe63f77..f7f757f7459f 100644
> --- a/rust/kernel/block/mq/request.rs
> +++ b/rust/kernel/block/mq/request.rs
> @@ -135,6 +135,23 @@ pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> {
>         Ok(())
>     }
> 
> +    /// Complete the request by scheduling `Operations::complete` for
> +    /// execution.
> +    ///
> +    /// The function may be scheduled locally, via SoftIRQ or remotely via IPMI.
> +    /// See `blk_mq_complete_request_remote` in [`blk-mq.c`] for details.
> +    ///
> +    /// [`blk-mq.c`]: srctree/block/blk-mq.c
> +    pub fn complete(this: ARef<Self>) {
> +        let ptr = ARef::into_raw(this).cast::<bindings::request>().as_ptr();
> +        // SAFETY: By type invariant, `self.0` is a valid `struct request`
> +        if !unsafe { bindings::blk_mq_complete_request_remote(ptr) } {
> +            // SAFETY: We released a refcount above that we can reclaim here.
> +            let this = unsafe { Request::aref_from_raw(ptr) };
> +            T::complete(this);
> +        }
> +    }
> +
>     /// Return a pointer to the [`RequestDataWrapper`] stored in the private area
>     /// of the request structure.
>     ///
> 
> -- 
> 2.47.2
> 
> 

I had another look here. While I do trust your reasoning, perhaps we should
remove the call to expect()?

If it is not called ever as you said, great, removing the expect() will not
change the code behavior. If it is, be it because of some minor oversight or
unexpected condition, we should produce some error output instead of crashing
the kernel. Maybe we should use a warn() here instead? Or maybe dev/pr_err as
applicable?

— Daniel