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

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

> On 29 Aug 2025, at 08:12, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> 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?
> 
> I think for the example, I would like to keep the `expect`. For
> demonstration purposes.

I think examples is definitely one place we don’t want to teach people to
use unwrap and expect because that will kill the system IIUC.

> 
> We could do `warn!` instead for the rnull driver I guess. But the IO
> queue that would hit this code would start to hang pretty fast, since no
> IO would complete. I don't think the kernel can recover from this hang.
> 

No I/O would complete for that device, but that doesn't mean that the system is
unusable IIUC, specially if other devices are available?

> 
> Best regards,
> Andreas Hindborg

In any case, perhaps this is only my opinion and others may see this
differently, so I won't complain if you want to keep it.


Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>