"Daniel Almeida" <daniel.almeida@xxxxxxxxxxxxx> writes:
> Hi Andreas,
>
>> On 22 Aug 2025, at 09:14, Andreas Hindborg <a.hindborg@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Reviewed-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
>> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>> ---
>> 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.
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.
Best regards,
Andreas Hindborg
