Re: [PATCH v6 15/18] rust: block: add `GenDisk` private data support

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

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 22 Aug 2025, at 09:14, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Allow users of the rust block device driver API to install private data in
>> the `GenDisk` structure.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> drivers/block/rnull/rnull.rs       |  8 ++++---
>> rust/kernel/block/mq.rs            |  7 +++---
>> rust/kernel/block/mq/gen_disk.rs   | 32 ++++++++++++++++++++++----
>> rust/kernel/block/mq/operations.rs | 46 ++++++++++++++++++++++++++++++--------
>> 4 files changed, 74 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
>> index 8690ff5f974f..8255236bc550 100644
>> --- a/drivers/block/rnull/rnull.rs
>> +++ b/drivers/block/rnull/rnull.rs
>> @@ -61,14 +61,16 @@ fn new(
>>             .logical_block_size(block_size)?
>>             .physical_block_size(block_size)?
>>             .rotational(rotational)
>> -            .build(fmt!("{}", name.to_str()?), tagset)
>> +            .build(fmt!("{}", name.to_str()?), tagset, ())
>>     }
>> }
>>
>> #[vtable]
>> impl Operations for NullBlkDevice {
>> +    type QueueData = ();
>> +
>>     #[inline(always)]
>> -    fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>> +    fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>>         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
>> @@ -79,5 +81,5 @@ fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>>         Ok(())
>>     }
>>
>> -    fn commit_rqs() {}
>> +    fn commit_rqs(_queue_data: ()) {}
>> }
>> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
>> index 98fa0d6bc8f7..6e546f4f3d1c 100644
>> --- a/rust/kernel/block/mq.rs
>> +++ b/rust/kernel/block/mq.rs
>> @@ -69,20 +69,21 @@
>> //!
>> //! #[vtable]
>> //! impl Operations for MyBlkDevice {
>> +//!     type QueueData = ();
>> //!
>> -//!     fn queue_rq(rq: ARef<Request<Self>>, _is_last: bool) -> Result {
>> +//!     fn queue_rq(_queue_data: (), rq: ARef<Request<Self>>, _is_last: bool) -> Result {
>> //!         Request::end_ok(rq);
>> //!         Ok(())
>> //!     }
>> //!
>> -//!     fn commit_rqs() {}
>> +//!     fn commit_rqs(_queue_data: ()) {}
>> //! }
>> //!
>> //! let tagset: Arc<TagSet<MyBlkDevice>> =
>> //!     Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
>> //! let mut disk = gen_disk::GenDiskBuilder::new()
>> //!     .capacity_sectors(4096)
>> -//!     .build(format_args!("myblk"), tagset)?;
>> +//!     .build(format_args!("myblk"), tagset, ())?;
>> //!
>> //! # Ok::<(), kernel::error::Error>(())
>> //! ```
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index 6b1b846874db..46ec80269970 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -13,6 +13,7 @@
>>     static_lock_class,
>>     str::NullTerminatedFormatter,
>>     sync::Arc,
>> +    types::{ForeignOwnable, ScopeGuard},
>> };
>> use core::fmt::{self, Write};
>>
>> @@ -98,7 +99,14 @@ pub fn build<T: Operations>(
>>         self,
>>         name: fmt::Arguments<'_>,
>>         tagset: Arc<TagSet<T>>,
>> +        queue_data: T::QueueData,
>>     ) -> Result<GenDisk<T>> {
>> +        let data = queue_data.into_foreign();
>> +        let recover_data = ScopeGuard::new(|| {
>> +            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> +            drop(unsafe { T::QueueData::from_foreign(data) });
>> +        });
>> +
>>         // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>>         let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>>
>> @@ -113,7 +121,7 @@ pub fn build<T: Operations>(
>>             bindings::__blk_mq_alloc_disk(
>>                 tagset.raw_tag_set(),
>>                 &mut lim,
>> -                core::ptr::null_mut(),
>> +                data,
>>                 static_lock_class!().as_ptr(),
>>             )
>>         })?;
>> @@ -167,8 +175,12 @@ pub fn build<T: Operations>(
>>             },
>>         )?;
>>
>> +        recover_data.dismiss();
>> +
>>         // INVARIANT: `gendisk` was initialized above.
>>         // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
>> +        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
>> +        // `__blk_mq_alloc_disk` above.
>>         Ok(GenDisk {
>>             _tagset: tagset,
>>             gendisk,
>> @@ -180,9 +192,10 @@ pub fn build<T: Operations>(
>> ///
>> /// # Invariants
>> ///
>> -/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> -/// - `gendisk` was added to the VFS through a call to
>> -///   `bindings::device_add_disk`.
>> +///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> +///  - `gendisk` was added to the VFS through a call to
>> +///    `bindings::device_add_disk`.
>> +///  - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
>> pub struct GenDisk<T: Operations> {
>>     _tagset: Arc<TagSet<T>>,
>>     gendisk: *mut bindings::gendisk,
>> @@ -194,9 +207,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
>>
>> impl<T: Operations> Drop for GenDisk<T> {
>>     fn drop(&mut self) {
>> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
>> +        // and initialized instance of `struct gendisk`, and, `queuedata` was
>> +        // initialized with the result of a call to
>> +        // `ForeignOwnable::into_foreign`.
>> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
>> +
>>         // SAFETY: By type invariant, `self.gendisk` points to a valid and
>>         // initialized instance of `struct gendisk`, and it was previously added
>>         // to the VFS.
>>         unsafe { bindings::del_gendisk(self.gendisk) };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
>> +        // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
>> +        // `ForeignOwnable::from_foreign` is only called here.
>> +        drop(unsafe { T::QueueData::from_foreign(queue_data) });
>>     }
>> }
>> diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
>> index c2b98f507bcb..6fb256f55acc 100644
>> --- a/rust/kernel/block/mq/operations.rs
>> +++ b/rust/kernel/block/mq/operations.rs
>> @@ -6,14 +6,15 @@
>>
>> use crate::{
>>     bindings,
>> -    block::mq::request::RequestDataWrapper,
>> -    block::mq::Request,
>> +    block::mq::{request::RequestDataWrapper, Request},
>>     error::{from_result, Result},
>>     prelude::*,
>> -    types::ARef,
>> +    types::{ARef, ForeignOwnable},
>> };
>> use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering};
>>
>> +type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;
>> +
>> /// Implement this trait to interface blk-mq as block devices.
>> ///
>> /// To implement a block device driver, implement this trait as described in the
>> @@ -26,12 +27,20 @@
>> /// [module level documentation]: kernel::block::mq
>> #[macros::vtable]
>> pub trait Operations: Sized {
>> +    /// 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 queue a request with the driver. If `is_last` is
>>     /// `false`, the driver is allowed to defer committing the request.
>> -    fn queue_rq(rq: ARef<Request<Self>>, is_last: bool) -> Result;
>> +    fn queue_rq(
>> +        queue_data: ForeignBorrowed<'_, Self::QueueData>,
>> +        rq: ARef<Request<Self>>,
>> +        is_last: bool,
>> +    ) -> Result;
>>
>>     /// Called by the kernel to indicate that queued requests should be submitted.
>> -    fn commit_rqs();
>> +    fn commit_rqs(queue_data: ForeignBorrowed<'_, Self::QueueData>);
>>
>>     /// Called by the kernel to poll the device for completed requests. Only
>>     /// used for poll queues.
>> @@ -70,7 +79,7 @@ impl<T: Operations> OperationsVTable<T> {
>>     ///   promise to not access the request until the driver calls
>>     ///   `bindings::blk_mq_end_request` for the request.
>>     unsafe extern "C" fn queue_rq_callback(
>> -        _hctx: *mut bindings::blk_mq_hw_ctx,
>> +        hctx: *mut bindings::blk_mq_hw_ctx,
>>         bd: *const bindings::blk_mq_queue_data,
>>     ) -> bindings::blk_status_t {
>>         // SAFETY: `bd.rq` is valid as required by the safety requirement for
>> @@ -88,10 +97,20 @@ impl<T: Operations> OperationsVTable<T> {
>>         //    reference counted by `ARef` until then.
>>         let rq = unsafe { Request::aref_from_raw((*bd).rq) };
>>
>> +        // SAFETY: `hctx` is valid as required by this function.
>> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
>
> isn’t this set on build() ?

Yes, you are right. Refactoring happened.

>
>> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
>
> into_pointer() ?

Should be `into_foreign`, will fix.

>
>> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
>> +        // dropped, which happens after we are dropped.
>> +        let queue_data = unsafe { T::QueueData::borrow(queue_data) };
>> +
>>         // SAFETY: We have exclusive access and we just set the refcount above.
>>         unsafe { Request::start_unchecked(&rq) };
>>
>>         let ret = T::queue_rq(
>> +            queue_data,
>>             rq,
>>             // SAFETY: `bd` is valid as required by the safety requirement for
>>             // this function.
>> @@ -110,9 +129,18 @@ impl<T: Operations> OperationsVTable<T> {
>>     ///
>>     /// # Safety
>>     ///
>> -    /// This function may only be called by blk-mq C infrastructure.
>> -    unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
>> -        T::commit_rqs()
>> +    /// This function may only be called by blk-mq C infrastructure. The caller
>> +    /// must ensure that `hctx` is valid.
>> +    unsafe extern "C" fn commit_rqs_callback(hctx: *mut bindings::blk_mq_hw_ctx) {
>> +        // SAFETY: `hctx` is valid as required by this function.
>> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
>> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
>
> into_foreign()?

Yep, thanks.

>
>> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
>> +        // dropped, which happens after we are dropped.
>> +        let queue_data = unsafe { T::QueueData::borrow(queue_data) };
>> +        T::commit_rqs(queue_data)
>>     }
>>
>>     /// This function is called by the C kernel. It is not currently
>>
>> --
>> 2.47.2
>>
>>
>>
>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>