Re: [PATCH 06/79] block: rust: add `Request` private data support

[Andreas Hindborg <a.hindborg@kernel.org>]

Alice Ryhl <aliceryhl@google.com> writes:

> On Mon, Feb 16, 2026 at 12:34:53AM +0100, Andreas Hindborg wrote:
>> C block device drivers can attach private data to a `struct request`. This
>> data is stored next to the request structure and is part of the request
>> allocation set up during driver initialization.
>> 
>> Expose this private request data area to Rust block device drivers.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Overall looks ok. A few nits below:
>
>> ---
>>  drivers/block/rnull/rnull.rs       |  5 +++++
>>  rust/kernel/block/mq.rs            |  6 ++++++
>>  rust/kernel/block/mq/operations.rs | 24 +++++++++++++++++++++++-
>>  rust/kernel/block/mq/request.rs    | 24 +++++++++++++++++++-----
>>  rust/kernel/block/mq/tag_set.rs    |  2 +-
>>  5 files changed, 54 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
>> index 6a7f660d31998..065639fc4f941 100644
>> --- a/drivers/block/rnull/rnull.rs
>> +++ b/drivers/block/rnull/rnull.rs
>> @@ -134,6 +134,11 @@ struct QueueData {
>>  #[vtable]
>>  impl Operations for NullBlkDevice {
>>      type QueueData = KBox<QueueData>;
>> +    type RequestData = ();
>> +
>> +    fn new_request_data() -> impl PinInit<Self::RequestData> {
>> +        pin_init::zeroed::<Self::RequestData>()
>
> Simpler to just return Ok(()) here.

Not sure why I picked zeroed. Will change.

>
>> +    }
>>  
>>      #[inline(always)]
>>      fn queue_rq(queue_data: &QueueData, rq: Owned<mq::Request<Self>>, _is_last: bool) -> Result {
>> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
>> index b8ecd69abe980..a285b753ada88 100644
>> --- a/rust/kernel/block/mq.rs
>> +++ b/rust/kernel/block/mq.rs
>> @@ -69,8 +69,14 @@
>>  //!
>>  //! #[vtable]
>>  //! impl Operations for MyBlkDevice {
>> +//!     type RequestData = ();
>>  //!     type QueueData = ();
>>  //!
>> +//!     fn new_request_data(
>> +//!     ) -> impl PinInit<()> {
>> +//!         pin_init::zeroed::<()>()
>
> Simpler to just return Ok(()) here.

Will change.

>
>> +//!     }
>> +//!
>>  //!     fn queue_rq(_queue_data: (), rq: Owned<Request<Self>>, _is_last: bool) -> Result {
>>  //!         rq.end_ok();
>>  //!         Ok(())
>> diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
>> index 3dea79d647ff7..cd37b939bbf30 100644
>> --- a/rust/kernel/block/mq/operations.rs
>> +++ b/rust/kernel/block/mq/operations.rs
>> @@ -13,6 +13,7 @@
>>      types::{ForeignOwnable, Owned},
>>  };
>>  use core::{marker::PhantomData, ptr::NonNull};
>> +use pin_init::PinInit;
>>  
>>  type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;
>>  
>> @@ -28,10 +29,24 @@
>>  /// [module level documentation]: kernel::block::mq
>>  #[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`.
>> +    ///
>> +    /// The `RequestData` object is initialized when the requests are allocated
>> +    /// during queue initialization, and it is are dropped when the requests are
>> +    /// dropped during queue teardown.
>> +    type RequestData: Sized + Sync;
>
> It was surprising to me that `request` is reused over many requests. I
> think it could be a bit more obvious in the wording.

I will add a bit of info in the `Request` type docs.

>
>>      /// Data associated with the `struct request_queue` that is allocated for
>>      /// the `GenDisk` associated with this `Operations` implementation.
>>      type QueueData: ForeignOwnable;
>>  
>> +    /// Called by the kernel to get an initializer for a `Pin<&mut RequestData>`.
>> +    fn new_request_data() -> impl PinInit<Self::RequestData>;
>> +
>>      /// Called by the kernel to queue a request with the driver. If `is_last` is
>>      /// `false`, the driver is allowed to defer committing the request.
>>      fn queue_rq(
>> @@ -236,6 +251,13 @@ impl<T: Operations> OperationsVTable<T> {
>>              // it is valid for writes.
>>              unsafe { RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(Refcount::new(0)) };
>>  
>> +            let initializer = T::new_request_data();
>> +
>> +            // SAFETY: `pdu` is a valid pointer as established above. We do not
>> +            // touch `pdu` if `__pinned_init` returns an error. We promise ot to
>
> typo
>
>
>> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
>> index c3cf56d52beec..46481754b1335 100644
>> --- a/rust/kernel/block/mq/tag_set.rs
>> +++ b/rust/kernel/block/mq/tag_set.rs
>> @@ -41,7 +41,7 @@ pub fn new(
>>          // SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
>>          // all are allowed to be 0.
>>          let tag_set: bindings::blk_mq_tag_set = unsafe { core::mem::zeroed() };
>> -        let tag_set: Result<_> = core::mem::size_of::<RequestDataWrapper>()
>> +        let tag_set: Result<_> = core::mem::size_of::<RequestDataWrapper<T>>()
>
> size_of in prelude.

Fixed.

Thanks,
Andreas