Rust block device driver API and null block driver

rust/kernel/block/mq/gen_disk.rs new file +31 -0

+    tagset: Arc<TagSet<T>>,
+    gendisk: *mut bindings::gendisk,
+    _phantom: core::marker::PhantomData<S>,
+}
+
+// SAFETY: `GenDisk` is an owned pointer to a `struct gendisk` and an `Arc` to a
+// `TagSet` It is safe to send this to other threads as long as T is Send.
+unsafe impl<T: Operations + Send, S: GenDiskState> Send for GenDisk<T, S> {}
+
+/// Disks in this state are allocated and initialized, but are not yet
+/// accessible from the kernel VFS.
+pub enum Initialized {}
+
+/// Disks in this state have been attached to the kernel VFS and may receive IO
+/// requests.
+pub enum Added {}
+
+mod seal {
+    pub trait Sealed {}
+}
+
+/// Typestate representing states of a `GenDisk`.
+///
+/// This trait cannot be implemented by downstream crates.
+pub trait GenDiskState: seal::Sealed {
+    /// Set to true if [`GenDisk`] should call `del_gendisk` on drop.
+    const DELETE_ON_DROP: bool;
+}
+
+impl seal::Sealed for Initialized {}
+impl GenDiskState for Initialized {

rust/kernel/block/mq/gen_disk.rs new file +31 -0

+        let lock_class_key = crate::sync::LockClassKey::new();
+
+        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
+        let gendisk = from_err_ptr(unsafe {
+            bindings::__blk_mq_alloc_disk(
+                tagset.raw_tag_set(),
+                core::ptr::null_mut(), // TODO: We can pass queue limits right here
+                core::ptr::null_mut(),
+                lock_class_key.as_ptr(),
+            )
+        })?;
+
+        const TABLE: bindings::block_device_operations = bindings::block_device_operations {
+            submit_bio: None,
+            open: None,
+            release: None,
+            ioctl: None,
+            compat_ioctl: None,
+            check_events: None,
+            unlock_native_capacity: None,
+            getgeo: None,
+            set_read_only: None,
+            swap_slot_free_notify: None,
+            report_zones: None,
+            devnode: None,
+            alternative_gpt_sector: None,
+            get_unique_id: None,
+            // TODO: Set to THIS_MODULE. Waiting for const_refs_to_static feature to
+            // be merged (unstable in rustc 1.78 which is staged for linux 6.10)
+            // https://github.com/rust-lang/rust/issues/119618
+            owner: core::ptr::null_mut(),

rust/kernel/block/mq/gen_disk.rs new file +8 -0

+        // `__blk_mq_alloc_disk` above.
+        Ok(GenDisk {
+            tagset,
+            gendisk,
+            _phantom: PhantomData,
+        })
+    }
+

rust/kernel/block/mq/operations.rs new file +5 -0

+    unsafe extern "C" fn queue_rq_callback(
+        _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

rust/kernel/block/mq/operations.rs new file +45 -0

+
+        // SAFETY: We have exclusive access and we just set the refcount above.
+        unsafe { Request::start_unchecked(&rq) };
+
+        let ret = T::queue_rq(
+            rq,
+            // SAFETY: `bd` is valid as required by the safety requirement for this function.
+            unsafe { (*bd).last },
+        );
+
+        if let Err(e) = ret {
+            e.to_blk_status()
+        } else {
+            bindings::BLK_STS_OK as _
+        }
+    }
+
+    /// 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 commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
+        T::commit_rqs()
+    }
+
+    /// This function is called by the C kernel. It is not currently
+    /// implemented, and there is no way to exercise this code path.
+    ///
+    /// # Safety
+    ///
+    /// This function may only be called by blk-mq C infrastructure.
+    unsafe extern "C" fn complete_callback(_rq: *mut bindings::request) {}
+
+    /// 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 poll_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _iob: *mut bindings::io_comp_batch,
+    ) -> core::ffi::c_int {

rust/kernel/block/mq/operations.rs new file +27 -0

+    /// for the same context.
+    unsafe extern "C" fn init_hctx_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _tagset_data: *mut core::ffi::c_void,
+        _hctx_idx: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| Ok(0))
+    }
+
+    /// 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 exit_hctx_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _hctx_idx: core::ffi::c_uint,
+    ) {
+    }
+
+    /// 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. `set` must

rust/kernel/block/mq/operations.rs new file +9 -0

+            let pdu = unsafe { bindings::blk_mq_rq_to_pdu(rq) }.cast::<RequestDataWrapper>();
+
+            // SAFETY: The refcount field is allocated but not initialized, this
+            // valid for write.
+            unsafe { RequestDataWrapper::refcount_ptr(pdu).write(AtomicU64::new(0)) };
+
+            Ok(0)
+        })
+    }

rust/kernel/block/mq/request.rs new file +17 -0

+        unsafe { bindings::blk_mq_start_request(this.0.get()) };
+    }
+
+    fn try_set_end(this: ARef<Self>) -> Result<ARef<Self>, ARef<Self>> {
+        // We can race with `TagSet::tag_to_rq`
+        match this.wrapper_ref().refcount().compare_exchange(
+            2,
+            0,
+            Ordering::Relaxed,
+            Ordering::Relaxed,
+        ) {
+            Err(_old) => Err(this),
+            Ok(_) => Ok(this),
+        }
+    }
+
+    /// Notify the block layer that the request has been completed without errors.

rust/kernel/block/mq/request.rs new file +6 -0

+        unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as _) };
+
+        Ok(())
+    }
+
+    /// Return a pointer to the `RequestDataWrapper` stored in the private area

rust/kernel/block/mq/request.rs new file +5 -0

+            unsafe { bindings::blk_mq_rq_to_pdu(request_ptr).cast::<RequestDataWrapper>() };
+        // SAFETY: By C api contract, wrapper_ptr points to a valid allocation
+        // and is not null.
+        unsafe { NonNull::new_unchecked(wrapper_ptr) }
+    }

rust/kernel/block/mq/gen_disk.rs line 18

Quoted 6 lines · Andreas Hindborg

rust/kernel/block/mq/gen_disk.rs new file +6 -0

+/// A generic block device.
+///
+/// # Invariants
+///
+///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
+pub struct GenDisk<T: Operations, S: GenDiskState = Added> {

I am curious, do you need the type state for this struct? AFAIU you are only using it to configure the GenDisk, so could you also use a config struct that is given to GenDisk::new. That way we can avoid the extra traits and generic argument.

Since there are so many options, a builder config struct might be a good idea.

Quoted 37 lines

rust/kernel/block/mq/gen_disk.rs new file +36 -0

+    tagset: Arc<TagSet<T>>,
+    gendisk: *mut bindings::gendisk,
+    _phantom: core::marker::PhantomData<S>,
+}
+
+// SAFETY: `GenDisk` is an owned pointer to a `struct gendisk` and an `Arc` to a
+// `TagSet` It is safe to send this to other threads as long as T is Send.
+unsafe impl<T: Operations + Send, S: GenDiskState> Send for GenDisk<T, S> {}
+
+/// Disks in this state are allocated and initialized, but are not yet
+/// accessible from the kernel VFS.
+pub enum Initialized {}
+
+/// Disks in this state have been attached to the kernel VFS and may receive IO
+/// requests.
+pub enum Added {}
+
+mod seal {
+    pub trait Sealed {}
+}
+
+/// Typestate representing states of a `GenDisk`.
+///
+/// This trait cannot be implemented by downstream crates.
+pub trait GenDiskState: seal::Sealed {
+    /// Set to true if [`GenDisk`] should call `del_gendisk` on drop.
+    const DELETE_ON_DROP: bool;
+}
+
+impl seal::Sealed for Initialized {}
+impl GenDiskState for Initialized {
+    const DELETE_ON_DROP: bool = false;
+}
+impl seal::Sealed for Added {}
+impl GenDiskState for Added {
+    const DELETE_ON_DROP: bool = true;

rust/kernel/block/mq/gen_disk.rs line 59

Quoted 5 lines

rust/kernel/block/mq/gen_disk.rs new file +5 -0

+}
+
+impl<T: Operations> GenDisk<T, Initialized> {
+    /// Try to create a new `GenDisk`.
+    pub fn try_new(tagset: Arc<TagSet<T>>) -> Result<Self> {

Since there is no non-try new function, I think we should name this function just new.

Quoted 36 lines

rust/kernel/block/mq/gen_disk.rs new file +35 -0

+        let lock_class_key = crate::sync::LockClassKey::new();
+
+        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
+        let gendisk = from_err_ptr(unsafe {
+            bindings::__blk_mq_alloc_disk(
+                tagset.raw_tag_set(),
+                core::ptr::null_mut(), // TODO: We can pass queue limits right here
+                core::ptr::null_mut(),
+                lock_class_key.as_ptr(),
+            )
+        })?;
+
+        const TABLE: bindings::block_device_operations = bindings::block_device_operations {
+            submit_bio: None,
+            open: None,
+            release: None,
+            ioctl: None,
+            compat_ioctl: None,
+            check_events: None,
+            unlock_native_capacity: None,
+            getgeo: None,
+            set_read_only: None,
+            swap_slot_free_notify: None,
+            report_zones: None,
+            devnode: None,
+            alternative_gpt_sector: None,
+            get_unique_id: None,
+            // TODO: Set to THIS_MODULE. Waiting for const_refs_to_static feature to
+            // be merged (unstable in rustc 1.78 which is staged for linux 6.10)
+            // https://github.com/rust-lang/rust/issues/119618
+            owner: core::ptr::null_mut(),
+            pr_ops: core::ptr::null_mut(),
+            free_disk: None,
+            poll_bio: None,
+        };

rust/kernel/block/mq/gen_disk.rs line 100

Quoted 6 lines

rust/kernel/block/mq/gen_disk.rs new file +6 -0

+
+        // SAFETY: gendisk is a valid pointer as we initialized it above
+        unsafe { (*gendisk).fops = &TABLE };
+
+        // INVARIANT: `gendisk` was initialized above.
+        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to

There is no data in the mentioned call above.

Quoted 9 lines

rust/kernel/block/mq/gen_disk.rs new file +8 -0

+        // `__blk_mq_alloc_disk` above.
+        Ok(GenDisk {
+            tagset,
+            gendisk,
+            _phantom: PhantomData,
+        })
+    }
+

[...]

rust/kernel/block/mq/operations.rs line 61

Quoted 8 lines

rust/kernel/block/mq/operations.rs new file +8 -0

+impl<T: Operations> OperationsVTable<T> {
+    /// 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
+    ///
+    /// - The caller of this function must ensure `bd` is valid
+    ///   and initialized. The pointees must outlive this function.

Until when do the pointees have to be alive? "must outlive this function" could also be the case if the pointees die immediately after this function returns.

rust/kernel/block/mq/operations.rs line 65

Quoted 5 lines

rust/kernel/block/mq/operations.rs new file +4 -0

+    /// - This function must not be called with a `hctx` for which
+    ///   `Self::exit_hctx_callback()` has been called.
+    /// - (*bd).rq must point to a valid `bindings:request` for which
+    ///   `OperationsVTable<T>::init_request_callback` was called

Missing . at the end.

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +5 -0

+    unsafe extern "C" fn queue_rq_callback(
+        _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

rust/kernel/block/mq/operations.rs line 80

Quoted 10 lines

rust/kernel/block/mq/operations.rs new file +10 -0

+        // this function.
+        let request = unsafe { &*(*bd).rq.cast::<Request<T>>() };
+
+        // One refcount for the ARef, one for being in flight
+        request.wrapper_ref().refcount().store(2, Ordering::Relaxed);
+
+        // SAFETY: We own a refcount that we took above. We pass that to `ARef`.
+        // By the safety requirements of this function, `request` is a valid
+        // `struct request` and the private data is properly initialized.
+        let rq = unsafe { Request::aref_from_raw((*bd).rq) };

I think that you need to require that the request is alive at least until blk_mq_end_request is called for the request (since at that point all ARefs will be gone).
Also if this is not guaranteed, the safety requirements of AlwaysRefCounted are violated (since the object can just disappear even if it has refcount > 0 [the refcount refers to the Rust refcount in the RequestDataWrapper, not the one in C]).

Quoted 50 lines

rust/kernel/block/mq/operations.rs new file +49 -0

+
+        // SAFETY: We have exclusive access and we just set the refcount above.
+        unsafe { Request::start_unchecked(&rq) };
+
+        let ret = T::queue_rq(
+            rq,
+            // SAFETY: `bd` is valid as required by the safety requirement for this function.
+            unsafe { (*bd).last },
+        );
+
+        if let Err(e) = ret {
+            e.to_blk_status()
+        } else {
+            bindings::BLK_STS_OK as _
+        }
+    }
+
+    /// 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 commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
+        T::commit_rqs()
+    }
+
+    /// This function is called by the C kernel. It is not currently
+    /// implemented, and there is no way to exercise this code path.
+    ///
+    /// # Safety
+    ///
+    /// This function may only be called by blk-mq C infrastructure.
+    unsafe extern "C" fn complete_callback(_rq: *mut bindings::request) {}
+
+    /// 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 poll_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _iob: *mut bindings::io_comp_batch,
+    ) -> core::ffi::c_int {
+        T::poll().into()
+    }
+
+    /// This function is called by the C kernel. A pointer to this function is

rust/kernel/block/mq/operations.rs line 135

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +6 -0

+    /// installed in the `blk_mq_ops` vtable for the driver.
+    ///
+    /// # Safety
+    ///
+    /// This function may only be called by blk-mq C infrastructure. This
+    /// function may only be called onece before `exit_hctx_callback` is called

Typo: "onece"

Quoted 33 lines

rust/kernel/block/mq/operations.rs new file +32 -0

+    /// for the same context.
+    unsafe extern "C" fn init_hctx_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _tagset_data: *mut core::ffi::c_void,
+        _hctx_idx: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| Ok(0))
+    }
+
+    /// 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 exit_hctx_callback(
+        _hctx: *mut bindings::blk_mq_hw_ctx,
+        _hctx_idx: core::ffi::c_uint,
+    ) {
+    }
+
+    /// 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. `set` must
+    /// point to an initialized `TagSet<T>`.
+    unsafe extern "C" fn init_request_callback(
+        _set: *mut bindings::blk_mq_tag_set,
+        rq: *mut bindings::request,
+        _hctx_idx: core::ffi::c_uint,

rust/kernel/block/mq/operations.rs line 172

Quoted 5 lines

rust/kernel/block/mq/operations.rs new file +5 -0

+        _numa_node: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The `blk_mq_tag_set` invariants guarantee that all
+            // requests are allocated with extra memory for the request data.

What guarantees that the right amount of memory has been allocated? AFAIU that is guaranteed by the TagSet (but there is no invariant).

Quoted 10 lines

rust/kernel/block/mq/operations.rs new file +9 -0

+            let pdu = unsafe { bindings::blk_mq_rq_to_pdu(rq) }.cast::<RequestDataWrapper>();
+
+            // SAFETY: The refcount field is allocated but not initialized, this
+            // valid for write.
+            unsafe { RequestDataWrapper::refcount_ptr(pdu).write(AtomicU64::new(0)) };
+
+            Ok(0)
+        })
+    }

[...]

rust/kernel/block/mq/request.rs line 82

Quoted 13 lines

rust/kernel/block/mq/request.rs new file +13 -0

+    /// Notify the block layer that a request is going to be processed now.
+    ///
+    /// The block layer uses this hook to do proper initializations such as
+    /// starting the timeout timer. It is a requirement that block device
+    /// drivers call this function when starting to process a request.
+    ///
+    /// # Safety
+    ///
+    /// The caller must have exclusive ownership of `self`, that is
+    /// `self.wrapper_ref().refcount() == 2`.
+    pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
+        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
+        // existence of `&mut self` we have exclusive access.

We don't have a &mut self. But the safety requirements ask for a unique ARef.

Quoted 24 lines

rust/kernel/block/mq/request.rs new file +23 -0

+        unsafe { bindings::blk_mq_start_request(this.0.get()) };
+    }
+
+    fn try_set_end(this: ARef<Self>) -> Result<ARef<Self>, ARef<Self>> {
+        // We can race with `TagSet::tag_to_rq`
+        match this.wrapper_ref().refcount().compare_exchange(
+            2,
+            0,
+            Ordering::Relaxed,
+            Ordering::Relaxed,
+        ) {
+            Err(_old) => Err(this),
+            Ok(_) => Ok(this),
+        }
+    }
+
+    /// Notify the block layer that the request has been completed without errors.
+    ///
+    /// This function will return `Err` if `this` is not the only `ARef`
+    /// referencing the request.
+    pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> {
+        let this = Self::try_set_end(this)?;
+        let request_ptr = this.0.get();

rust/kernel/block/mq/request.rs line 109

Quoted 4 lines

rust/kernel/block/mq/request.rs new file +4 -0

+        core::mem::forget(this);
+
+        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
+        // existence of `&mut self` we have exclusive access.

Same here, but in this case, the ARef is unique, since you called try_set_end. You could make it a # Guarantee of try_set_end: "If Ok(aref) is returned, then the aref is unique."

Quoted 11 lines

rust/kernel/block/mq/request.rs new file +10 -0

+        unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as _) };
+
+        Ok(())
+    }
+
+    /// Return a pointer to the `RequestDataWrapper` stored in the private area
+    /// of the request structure.
+    ///
+    /// # Safety
+    ///

rust/kernel/block/mq/request.rs line 125

Quoted 6 lines

rust/kernel/block/mq/request.rs new file +6 -0

+    /// - `this` must point to a valid allocation.
+    pub(crate) unsafe fn wrapper_ptr(this: *mut Self) -> NonNull<RequestDataWrapper> {
+        let request_ptr = this.cast::<bindings::request>();
+        let wrapper_ptr =
+            // SAFETY: By safety requirements for this function, `this` is a
+            // valid allocation.

Formatting: move the safety comment above the let.

Quoted 16 lines · Benno Lossin

rust/kernel/block/mq/gen_disk.rs line 59

Quoted 12 lines · Andreas Hindborg

rust/kernel/block/mq/gen_disk.rs new file +12 -0

+impl seal::Sealed for Initialized {}
+impl GenDiskState for Initialized {
+    const DELETE_ON_DROP: bool = false;
+}
+impl seal::Sealed for Added {}
+impl GenDiskState for Added {
+    const DELETE_ON_DROP: bool = true;
+}
+
+impl<T: Operations> GenDisk<T, Initialized> {
+    /// Try to create a new `GenDisk`.
+    pub fn try_new(tagset: Arc<TagSet<T>>) -> Result<Self> {

Since there is no non-try new function, I think we should name this function just new.

Right, I am still getting used to the new naming scheme. Do you know if it is documented anywhere?

Quoted 9 lines

rust/kernel/block/mq/operations.rs line 61

Quoted 8 lines

rust/kernel/block/mq/operations.rs new file +8 -0

+impl<T: Operations> OperationsVTable<T> {
+    /// 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
+    ///
+    /// - The caller of this function must ensure `bd` is valid
+    ///   and initialized. The pointees must outlive this function.

Quoted 3 lines

Until when do the pointees have to be alive? "must outlive this function" could also be the case if the pointees die immediately after this function returns.

It should not be plural. What I intended to communicate is that what bd points to must be valid for read for the duration of the function call. I think that is what "The pointee must outlive this function" states? Although when we talk about lifetime of an object pointed to by a pointer, I am not sure about the correct way to word this. Do we talk about the lifetime of the pointer or the lifetime of the pointed to object (the pointee). We should not use the same wording for the pointer and the pointee.

How about:

    /// - The caller of this function must ensure that the pointee of `bd` is
    ///   valid for read for the duration of this function.

Quoted 6 lines

rust/kernel/block/mq/operations.rs line 65

Quoted 4 lines

rust/kernel/block/mq/operations.rs new file +4 -0

+    /// - This function must not be called with a `hctx` for which
+    ///   `Self::exit_hctx_callback()` has been called.
+    /// - (*bd).rq must point to a valid `bindings:request` for which
+    ///   `OperationsVTable<T>::init_request_callback` was called

Missing . at the end.

Thanks.

Quoted 17 lines

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +5 -0

+    unsafe extern "C" fn queue_rq_callback(
+        _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

rust/kernel/block/mq/operations.rs line 80

Quoted 10 lines

rust/kernel/block/mq/operations.rs new file +10 -0

+        // this function.
+        let request = unsafe { &*(*bd).rq.cast::<Request<T>>() };
+
+        // One refcount for the ARef, one for being in flight
+        request.wrapper_ref().refcount().store(2, Ordering::Relaxed);
+
+        // SAFETY: We own a refcount that we took above. We pass that to `ARef`.
+        // By the safety requirements of this function, `request` is a valid
+        // `struct request` and the private data is properly initialized.
+        let rq = unsafe { Request::aref_from_raw((*bd).rq) };

Quoted 7 lines

I think that you need to require that the request is alive at least until blk_mq_end_request is called for the request (since at that point all ARefs will be gone).
Also if this is not guaranteed, the safety requirements of AlwaysRefCounted are violated (since the object can just disappear even if it has refcount > 0 [the refcount refers to the Rust refcount in the RequestDataWrapper, not the one in C]).

Yea, for the last invariant of Request:

/// * self is reference counted by atomic modification of
/// self.wrapper_ref().refcount().

I will add this to the safety comment at the call site:

// - rq will be alive until blk_mq_end_request is called and is
// reference counted by ARef until then.

Quoted 6 lines

rust/kernel/block/mq/operations.rs line 162

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +6 -0

+    /// 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. `set` must

Quoted 11 lines

rust/kernel/block/mq/operations.rs line 172

Quoted 10 lines

rust/kernel/block/mq/operations.rs new file +10 -0

+    /// point to an initialized `TagSet<T>`.
+    unsafe extern "C" fn init_request_callback(
+        _set: *mut bindings::blk_mq_tag_set,
+        rq: *mut bindings::request,
+        _hctx_idx: core::ffi::c_uint,
+        _numa_node: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The `blk_mq_tag_set` invariants guarantee that all
+            // requests are allocated with extra memory for the request data.

Quoted 2 lines

What guarantees that the right amount of memory has been allocated? AFAIU that is guaranteed by the TagSet (but there is no invariant).

It is by C API contract. TagSet::try_new (now new) writes cmd_size into the struct blk_mq_tag_set. That is picked up by blk_mq_alloc_tag_set` to allocate the right amount of space for each request.

The invariant here is on the C type. Perhaps the wording is wrong. I am not exactly sure how to express this. How about this:

            // SAFETY: We instructed `blk_mq_alloc_tag_set` to allocate requests
            // with extra memory for the request data when we called it in
            // `TagSet::new`.

Quoted 26 lines

Quoted 9 lines

rust/kernel/block/mq/operations.rs new file +9 -0

+            let pdu = unsafe { bindings::blk_mq_rq_to_pdu(rq) }.cast::<RequestDataWrapper>();
+
+            // SAFETY: The refcount field is allocated but not initialized, this
+            // valid for write.
+            unsafe { RequestDataWrapper::refcount_ptr(pdu).write(AtomicU64::new(0)) };
+
+            Ok(0)
+        })
+    }

[...]

rust/kernel/block/mq/request.rs line 82

Quoted 13 lines

rust/kernel/block/mq/request.rs new file +13 -0

+    /// Notify the block layer that a request is going to be processed now.
+    ///
+    /// The block layer uses this hook to do proper initializations such as
+    /// starting the timeout timer. It is a requirement that block device
+    /// drivers call this function when starting to process a request.
+    ///
+    /// # Safety
+    ///
+    /// The caller must have exclusive ownership of `self`, that is
+    /// `self.wrapper_ref().refcount() == 2`.
+    pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
+        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
+        // existence of `&mut self` we have exclusive access.

Quoted 2 lines

We don't have a &mut self. But the safety requirements ask for a unique ARef.

Thanks, I'll rephrase to:

        // SAFETY: By type invariant, `self.0` is a valid `struct request` and
        // we have exclusive access.

Quoted 24 lines

Quoted 24 lines

rust/kernel/block/mq/request.rs new file +23 -0

+        unsafe { bindings::blk_mq_start_request(this.0.get()) };
+    }
+
+    fn try_set_end(this: ARef<Self>) -> Result<ARef<Self>, ARef<Self>> {
+        // We can race with `TagSet::tag_to_rq`
+        match this.wrapper_ref().refcount().compare_exchange(
+            2,
+            0,
+            Ordering::Relaxed,
+            Ordering::Relaxed,
+        ) {
+            Err(_old) => Err(this),
+            Ok(_) => Ok(this),
+        }
+    }
+
+    /// Notify the block layer that the request has been completed without errors.
+    ///
+    /// This function will return `Err` if `this` is not the only `ARef`
+    /// referencing the request.
+    pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> {
+        let this = Self::try_set_end(this)?;
+        let request_ptr = this.0.get();

Quoted 8 lines

rust/kernel/block/mq/request.rs line 109

Quoted 4 lines

rust/kernel/block/mq/request.rs new file +4 -0

+        core::mem::forget(this);
+
+        // SAFETY: By type invariant, `self.0` is a valid `struct request`. By
+        // existence of `&mut self` we have exclusive access.

Same here, but in this case, the ARef is unique, since you called try_set_end. You could make it a # Guarantee of try_set_end: "If Ok(aref) is returned, then the aref is unique."

Makes sense. I have not seen # Guarantee used anywhere. Do you have a link for that use?

Quoted 24 lines

Quoted 9 lines

rust/kernel/block/mq/operations.rs line 61

Quoted 8 lines

rust/kernel/block/mq/operations.rs new file +8 -0

+impl<T: Operations> OperationsVTable<T> {
+    /// 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
+    ///
+    /// - The caller of this function must ensure `bd` is valid
+    ///   and initialized. The pointees must outlive this function.

Quoted 3 lines

Until when do the pointees have to be alive? "must outlive this function" could also be the case if the pointees die immediately after this function returns.

It should not be plural. What I intended to communicate is that what bd points to must be valid for read for the duration of the function call. I think that is what "The pointee must outlive this function" states? Although when we talk about lifetime of an object pointed to by a pointer, I am not sure about the correct way to word this. Do we talk about the lifetime of the pointer or the lifetime of the pointed to object (the pointee). We should not use the same wording for the pointer and the pointee.

How about:

Quoted 2 lines

    /// - The caller of this function must ensure that the pointee of `bd` is
    ///   valid for read for the duration of this function.

But this is not enough for it to be sound, right? You create an ARef from bd.rq, which potentially lives forever. You somehow need to require that the pointer bd stays valid for reads and (synchronized) writes until the request is ended (probably via blk_mq_end_request).

Quoted 6 lines

Quoted 6 lines

rust/kernel/block/mq/operations.rs line 162

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +6 -0

+    /// 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. `set` must

set doesn't exist (_set does), you are also not using this requirement.

Quoted 22 lines

Quoted 12 lines

rust/kernel/block/mq/operations.rs line 172

Quoted 11 lines

rust/kernel/block/mq/operations.rs new file +10 -0

+    /// point to an initialized `TagSet<T>`.
+    unsafe extern "C" fn init_request_callback(
+        _set: *mut bindings::blk_mq_tag_set,
+        rq: *mut bindings::request,
+        _hctx_idx: core::ffi::c_uint,
+        _numa_node: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The `blk_mq_tag_set` invariants guarantee that all
+            // requests are allocated with extra memory for the request data.

Quoted 2 lines

What guarantees that the right amount of memory has been allocated? AFAIU that is guaranteed by the TagSet (but there is no invariant).

It is by C API contract. TagSet::try_new (now new) writes cmd_size into the struct blk_mq_tag_set. That is picked up by blk_mq_alloc_tag_set` to allocate the right amount of space for each request.

The invariant here is on the C type. Perhaps the wording is wrong. I am not exactly sure how to express this. How about this:

Quoted 3 lines

            // SAFETY: We instructed `blk_mq_alloc_tag_set` to allocate requests
            // with extra memory for the request data when we called it in
            // `TagSet::new`.

I think you need a safety requirement on the function: rq points to a valid Request. Then you could just use Request::wrapper_ptr instead of the line below.

Benno Lossin <[email protected]> writes:

[...]

Quoted 24 lines

Quoted 24 lines

Quoted 9 lines

rust/kernel/block/mq/operations.rs line 61

Quoted 8 lines

rust/kernel/block/mq/operations.rs new file +8 -0

+impl<T: Operations> OperationsVTable<T> {
+    /// 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
+    ///
+    /// - The caller of this function must ensure `bd` is valid
+    ///   and initialized. The pointees must outlive this function.

Quoted 3 lines

Until when do the pointees have to be alive? "must outlive this function" could also be the case if the pointees die immediately after this function returns.

It should not be plural. What I intended to communicate is that what bd points to must be valid for read for the duration of the function call. I think that is what "The pointee must outlive this function" states? Although when we talk about lifetime of an object pointed to by a pointer, I am not sure about the correct way to word this. Do we talk about the lifetime of the pointer or the lifetime of the pointed to object (the pointee). We should not use the same wording for the pointer and the pointee.

How about:

Quoted 7 lines

Quoted 2 lines

    /// - The caller of this function must ensure that the pointee of `bd` is
    ///   valid for read for the duration of this function.

But this is not enough for it to be sound, right? You create an ARef from bd.rq, which potentially lives forever. You somehow need to require that the pointer bd stays valid for reads and (synchronized) writes until the request is ended (probably via blk_mq_end_request).

The statement does not say anything about *((*bd).rq). *bd needs to be valid only for the duration of the function. It carries a pointer to a struct request in the rq field. The pointee of that pointer must be exclusively owned by the driver until the request is done.

Maybe like this:

# Safety

- The caller of this function must ensure that the pointee of bd is valid for read for the duration of this function.

- This function must be called for an initialized and live hctx. That is, Self::init_hctx_callback was called and Self::exit_hctx_callback() was not yet called.
- (*bd).rq must point to an initialized and live bindings:request. That is, Self::init_request_callback was called but Self::exit_request_callback was not yet called for the request.
- (*bd).rq must be owned by the driver. That is, the block layer must promise to not access the request until the driver calls bindings::blk_mq_end_request for the request.

[...]

Quoted 9 lines

Quoted 6 lines

Quoted 6 lines

rust/kernel/block/mq/operations.rs line 162

Quoted 6 lines

rust/kernel/block/mq/operations.rs new file +6 -0

+    /// 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. `set` must

set doesn't exist (_set does), you are also not using this requirement.

Would be nice if there was a way in rustdoc no name arguments explicitly.

Quoted 22 lines

Quoted 22 lines

Quoted 12 lines

rust/kernel/block/mq/operations.rs line 172

Quoted 11 lines

rust/kernel/block/mq/operations.rs new file +10 -0

+    /// point to an initialized `TagSet<T>`.
+    unsafe extern "C" fn init_request_callback(
+        _set: *mut bindings::blk_mq_tag_set,
+        rq: *mut bindings::request,
+        _hctx_idx: core::ffi::c_uint,
+        _numa_node: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The `blk_mq_tag_set` invariants guarantee that all
+            // requests are allocated with extra memory for the request data.

Quoted 2 lines

What guarantees that the right amount of memory has been allocated? AFAIU that is guaranteed by the TagSet (but there is no invariant).

It is by C API contract. TagSet::try_new (now new) writes cmd_size into the struct blk_mq_tag_set. That is picked up by blk_mq_alloc_tag_set` to allocate the right amount of space for each request.

The invariant here is on the C type. Perhaps the wording is wrong. I am not exactly sure how to express this. How about this:

Quoted 7 lines

Quoted 3 lines

            // SAFETY: We instructed `blk_mq_alloc_tag_set` to allocate requests
            // with extra memory for the request data when we called it in
            // `TagSet::new`.

I think you need a safety requirement on the function: rq points to a valid Request. Then you could just use Request::wrapper_ptr instead of the line below.

I cannot require rq to point to a valid Request, because that would require the private data area to already be initialized as a valid RequestDataWrapper. Using the wrapper_ptr is good 👍. How is this:

    /// # Safety
    ///
    /// - This function may only be called by blk-mq C infrastructure.
    /// - `_set` must point to an initialized `TagSet<T>`.
    /// - `rq` must point to an initialized `bindings::request`.
    /// - The allocation pointed to by `rq` must be at the size of `Request`
    ///   plus the size of `RequestDataWrapper`.

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Do you think it is OK to have this redundancy? It would only be for a few cycles.

Quoted 22 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Quoted 2 lines

Do you think it is OK to have this redundancy? It would only be for a few cycles.

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Quoted 23 lines · Keith Busch

Quoted 22 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Quoted 8 lines

Quoted 2 lines

Do you think it is OK to have this redundancy? It would only be for a few cycles.

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Right, that makes sense. I modeled it after the C null_blk validation code in null_validate_conf. It contains this:

	dev->blocksize = round_down(dev->blocksize, 512);
	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);

That would have the same semantics, right? I guess I'll try to make a device with a 1536 block size and see what happens.

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Quoted 4 lines · Keith Busch

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

Quoted 1 line

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

I'd drop the range check. We're pretty close to landing the bs>PS patches, so just

	if block_size & block_size - 1 != 0

should be enough of a validation.

Is it considered "good style" in
Rust to omit the brackets here?

Quoted 22 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Quoted 2 lines

Do you think it is OK to have this redundancy? It would only be for a few cycles.

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Quoted 26 lines · Keith Busch

Quoted 24 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Do you think it is OK to have this redundancy? It would only be for a few cycles.

Quoted 6 lines

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

      if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Can't we overload .contains() to check only power-of-2 values?

Quoted 32 lines · Hannes Reinecke

Quoted 30 lines · Keith Busch

Quoted 17 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Quoted 7 lines

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Do you think it is OK to have this redundancy? It would only be for a few cycles.

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

Quoted 3 lines

Quoted 2 lines

      if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)

Can't we overload .contains() to check only power-of-2 values?

Rust integers have a method called is_power_of_two. If you need to assert that it's a power of two, you can use that.

Quoted 25 lines · Keith Busch

Quoted 24 lines · Andreas Hindborg

Quoted 14 lines · Keith Busch

drivers/block/rnull.rs line 47

Quoted 10 lines · Andreas Hindborg

drivers/block/rnull.rs new file +10 -0

+impl kernel::Module for NullBlkModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust null_blk loaded\n");
+        let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        let disk = {
+            let block_size: u16 = 4096;
+            if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
+                return Err(kernel::error::code::EINVAL);
+            }

You've set block_size to the literal 4096, then validate its value immediately after? Am I missing some way this could ever be invalid?

Good catch. It is because I have a patch in the outbound queue that allows setting the block size via a module parameter. The module parameter patch is not upstream yet. Once I have that up, I will send the patch with the block size config.

Do you think it is OK to have this redundancy? It would only be for a few cycles.

Quoted 6 lines

It's fine, just wondering why it's there. But it also allows values like 1536 and 3584, which are not valid block sizes, so I think you want the check to be:

	if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1))

!= 0)

Can't we overload .contains() to check only power-of-2 values?