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

Andreas Hindborg posted 18 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v6 17/18] rust: block: add remote completion to `Request`
Posted by Andreas Hindborg 1 month, 1 week ago
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
Re: [PATCH v6 17/18] rust: block: add remote completion to `Request`
Posted by Daniel Almeida 1 month, 1 week ago
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
Re: [PATCH v6 17/18] rust: block: add remote completion to `Request`
Posted by Andreas Hindborg 1 month ago
"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.

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
Re: [PATCH v6 17/18] rust: block: add remote completion to `Request`
Posted by Daniel Almeida 1 month ago

> 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>