On 21.05.24 16:03, Andreas Hindborg wrote: > From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx> > > Add initial abstractions for working with blk-mq. > > This patch is a maintained, refactored subset of code originally published > by Wedson Almeida Filho <wedsonaf@xxxxxxxxx> [1]. > > [1] https://github.com/wedsonaf/linux/tree/f2cfd2fe0e2ca4e90994f96afe268bbd4382a891/rust/kernel/blk/mq.rs > > Cc: Wedson Almeida Filho <wedsonaf@xxxxxxxxx> > Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxxx> > --- > rust/bindings/bindings_helper.h | 2 + > rust/helpers.c | 16 ++ > rust/kernel/block.rs | 5 + > rust/kernel/block/mq.rs | 97 ++++++++++++ > rust/kernel/block/mq/gen_disk.rs | 206 ++++++++++++++++++++++++ > rust/kernel/block/mq/operations.rs | 245 +++++++++++++++++++++++++++++ > rust/kernel/block/mq/raw_writer.rs | 55 +++++++ > rust/kernel/block/mq/request.rs | 227 ++++++++++++++++++++++++++ > rust/kernel/block/mq/tag_set.rs | 93 +++++++++++ > rust/kernel/error.rs | 6 + > rust/kernel/lib.rs | 2 + > 11 files changed, 954 insertions(+) > create mode 100644 rust/kernel/block.rs > create mode 100644 rust/kernel/block/mq.rs > create mode 100644 rust/kernel/block/mq/gen_disk.rs > create mode 100644 rust/kernel/block/mq/operations.rs > create mode 100644 rust/kernel/block/mq/raw_writer.rs > create mode 100644 rust/kernel/block/mq/request.rs > create mode 100644 rust/kernel/block/mq/tag_set.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index ddb5644d4fd9..68f937f8374f 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -7,6 +7,8 @@ > */ > > #include <kunit/test.h> > +#include <linux/blk_types.h> > +#include <linux/blk-mq.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > #include <linux/jiffies.h> > diff --git a/rust/helpers.c b/rust/helpers.c > index 4c8b7b92a4f4..3ba108095346 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -178,3 +178,19 @@ static_assert( > __alignof__(size_t) == __alignof__(uintptr_t), > "Rust code expects C `size_t` to match Rust `usize`" > ); > + > +// This will soon be moved to a separate file, so no need to merge with above. > +#include <linux/blk-mq.h> > +#include <linux/blkdev.h> > + > +void *rust_helper_blk_mq_rq_to_pdu(struct request *rq) > +{ > + return blk_mq_rq_to_pdu(rq); > +} > +EXPORT_SYMBOL_GPL(rust_helper_blk_mq_rq_to_pdu); > + > +struct request *rust_helper_blk_mq_rq_from_pdu(void *pdu) > +{ > + return blk_mq_rq_from_pdu(pdu); > +} > +EXPORT_SYMBOL_GPL(rust_helper_blk_mq_rq_from_pdu); > diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs > new file mode 100644 > index 000000000000..150f710efe5b > --- /dev/null > +++ b/rust/kernel/block.rs > @@ -0,0 +1,5 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Types for working with the block layer. > + > +pub mod mq; > diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs > new file mode 100644 > index 000000000000..efbd2588791b > --- /dev/null > +++ b/rust/kernel/block/mq.rs > @@ -0,0 +1,97 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! This module provides types for implementing block drivers that interface the > +//! blk-mq subsystem. > +//! > +//! To implement a block device driver, a Rust module must do the following: > +//! > +//! - Implement [`Operations`] for a type `T` > +//! - Create a [`TagSet<T>`] > +//! - Create a [`gen_disk::GenDisk<T>`], passing in the `TagSet` reference I would use short links [`GenDisk<T>`]. > +//! - Add the disk to the system by calling [`gen_disk::GenDisk::add`] > +//! > +//! The types available in this module that have direct C counterparts are: > +//! > +//! - The `TagSet` type that abstracts the C type `struct tag_set`. Missing link? (also below) > +//! - The `GenDisk` type that abstracts the C type `struct gendisk`. > +//! - The `Request` type that abstracts the C type `struct request`. > +//! > +//! The kernel will interface with the block device driver by calling the method > +//! implementations of the `Operations` trait. > +//! > +//! IO requests are passed to the driver as [`kernel::types::ARef<Request>`] Ditto. > +//! instances. The `Request` type is a wrapper around the C `struct request`. > +//! The driver must mark end of processing by calling one of the > +//! `Request::end`, methods. Failure to do so can lead to deadlock or timeout > +//! errors. Please note that the C function `blk_mq_start_request` is implicitly > +//! called when the request is queued with the driver. > +//! > +//! The `TagSet` is responsible for creating and maintaining a mapping between > +//! `Request`s and integer ids as well as carrying a pointer to the vtable > +//! generated by `Operations`. This mapping is useful for associating > +//! completions from hardware with the correct `Request` instance. The `TagSet` > +//! determines the maximum queue depth by setting the number of `Request` > +//! instances available to the driver, and it determines the number of queues to > +//! instantiate for the driver. If possible, a driver should allocate one queue > +//! per core, to keep queue data local to a core. > +//! > +//! One `TagSet` instance can be shared between multiple `GenDisk` instances. > +//! This can be useful when implementing drivers where one piece of hardware > +//! with one set of IO resources are represented to the user as multiple disks. > +//! > +//! One significant difference between block device drivers implemented with > +//! these Rust abstractions and drivers implemented in C, is that the Rust > +//! drivers have to own a reference count on the `Request` type when the IO is > +//! in flight. This is to ensure that the C `struct request` instances backing > +//! the Rust `Request` instances are live while the Rust driver holds a > +//! reference to the `Request`. In addition, the conversion of an integer tag to > +//! a `Request` via the `TagSet` would not be sound without this bookkeeping. > +//! > +//! # Example > +//! > +//! ```rust > +//! use kernel::{ > +//! block::mq::*, > +//! new_mutex, > +//! prelude::*, > +//! sync::{Arc, Mutex}, > +//! types::{ARef, ForeignOwnable}, > +//! }; > +//! > +//! struct MyBlkDevice; > +//! > +//! #[vtable] > +//! impl Operations for MyBlkDevice { > +//! > +//! fn queue_rq(rq: ARef<Request<Self>>, _is_last: bool) -> Result { > +//! Request::end_ok(rq); > +//! Ok(()) > +//! } > +//! > +//! fn commit_rqs( > +//! ) { > +//! } Formatting. > +//! > +//! fn complete(rq: ARef<Request<Self>>) { > +//! Request::end_ok(rq); > +//! } > +//! } > +//! > +//! let tagset: Arc<TagSet<MyBlkDevice>> = Arc::pin_init(TagSet::try_new(1, 256, 1))?; > +//! let mut disk = gen_disk::try_new(tagset)?; > +//! disk.set_name(format_args!("myblk"))?; > +//! disk.set_capacity_sectors(4096); > +//! disk.add()?; > +//! > +//! # Ok::<(), kernel::error::Error>(()) > +//! ``` > + > +pub mod gen_disk; > +mod operations; > +mod raw_writer; > +mod request; > +mod tag_set; > + > +pub use operations::Operations; > +pub use request::Request; > +pub use tag_set::TagSet; > diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs > new file mode 100644 > index 000000000000..a88802c1f918 > --- /dev/null > +++ b/rust/kernel/block/mq/gen_disk.rs > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generic disk abstraction. > +//! > +//! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h) > +//! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h) > + > +use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet}; > +use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc}; > +use core::fmt::{self, Write}; > +use core::marker::PhantomData; > + > +/// A generic block device. > +/// > +/// # Invariants > +/// > +/// - `gendisk` must always point to an initialized and valid `struct gendisk`. > +pub struct GenDisk<T: Operations, S: GenDiskState> { Does it make sense to do `S: GenDiskState = Added`? > + _tagset: Arc<TagSet<T>>, Does this really need a leading underscore? > + 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 {} > + > +/// Typestate representing states of a `GenDisk`. > +pub trait GenDiskState {} Should this trait be sealed? > + > +impl GenDiskState for Initialized {} > +impl GenDiskState for Added {} > + > +impl<T: Operations> GenDisk<T, Initialized> { > + /// Register the device with the kernel. When this function returns, the > + /// device is accessible from VFS. The kernel may issue reads to the device > + /// during registration to discover partition information. > + pub fn add(self) -> Result<GenDisk<T, Added>> { > + crate::error::to_result( > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk` > + unsafe { > + bindings::device_add_disk( > + core::ptr::null_mut(), > + self.gendisk, > + core::ptr::null_mut(), > + ) > + }, > + )?; > + > + // We don't want to run the destuctor and remove the device from the VFS > + // when `disk` is dropped. > + let mut old = core::mem::ManuallyDrop::new(self); > + > + let new = GenDisk { > + _tagset: old._tagset.clone(), > + gendisk: old.gendisk, > + _phantom: PhantomData, > + }; > + > + // But we have to drop the `Arc` or it will leak. > + // SAFETY: `old._tagset` is valid for write, aligned, non-null, and we > + // have exclusive access. We are not accessing the value again after it > + // is dropped. > + unsafe { core::ptr::drop_in_place(&mut old._tagset) }; > + > + Ok(new) Instead of using `ManuallyDrop` and `drop_in_place` why not use `transmute`? I feel like that would be a lot simpler. > + } > + > + /// Set the name of the device. > + pub fn set_name(&mut self, args: fmt::Arguments<'_>) -> Result { > + let mut raw_writer = RawWriter::from_array( > + // SAFETY: By type invariant `self.gendisk` points to a valid and > + // initialized instance. We have exclusive access, since the disk is > + // not added to the VFS yet. > + unsafe { &mut (*self.gendisk).disk_name }, > + )?; > + raw_writer.write_fmt(args)?; > + raw_writer.write_char('\0')?; > + Ok(()) > + } > + > + /// Set the logical block size of the device. > + /// > + /// This is the smallest unit the storage device can address. It is > + /// typically 4096 bytes. > + pub fn set_queue_logical_block_size(&mut self, size: u32) { > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk`. > + unsafe { bindings::blk_queue_logical_block_size((*self.gendisk).queue, size) }; > + } > + > + /// Set the physical block size of the device. > + /// > + /// This is the smallest unit a physical storage device can write > + /// atomically. It is usually the same as the logical block size but may be > + /// bigger. One example is SATA drives with 4096 byte physical block size > + /// that expose a 512 byte logical block size to the operating system. > + pub fn set_queue_physical_block_size(&mut self, size: u32) { > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk`. > + unsafe { bindings::blk_queue_physical_block_size((*self.gendisk).queue, size) }; > + } > +} > + > +impl<T: Operations, S: GenDiskState> GenDisk<T, S> { > + /// Call to tell the block layer the capacity of the device in sectors (512 > + /// bytes). > + pub fn set_capacity_sectors(&self, sectors: u64) { > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk`. Callee takes a lock to By `Callee`, do you mean `set_capacity`? If so, I would prefer that. > + // synchronize this operation, so we will not race. > + unsafe { bindings::set_capacity(self.gendisk, sectors) }; > + } > + > + /// Set the rotational media attribute for the device. > + pub fn set_rotational(&self, rotational: bool) { > + if !rotational { > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk`. This operation uses a > + // relaxed atomic bit flip operation, so there is no race on this > + // field. > + unsafe { > + bindings::blk_queue_flag_set(bindings::QUEUE_FLAG_NONROT, (*self.gendisk).queue) > + }; > + } else { > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk`. This operation uses a > + // relaxed atomic bit flip operation, so there is no race on this > + // field. > + unsafe { > + bindings::blk_queue_flag_clear(bindings::QUEUE_FLAG_NONROT, (*self.gendisk).queue) > + }; > + } > + } > +} > + > +impl<T: Operations, S: GenDiskState> Drop for GenDisk<T, S> { > + fn drop(&mut self) { > + // TODO: This will `WARN` if the disk was not added. Since we cannot > + // specialize drop, we have to call it, or track state with a flag. You could add an associated constant to GenDiskState and then write the following: if <S as GenDiskState>::DELETE_ON_DROP { /* del_gendisk ... */ } Then the check is essentially done at compile-time. > + > + // SAFETY: By type invariant, `self.gendisk` points to a valid and > + // initialized instance of `struct gendisk` > + unsafe { bindings::del_gendisk(self.gendisk) }; > + } > +} > + > +/// Try to create a new `GenDisk`. > +pub fn try_new<T: Operations>(tagset: Arc<TagSet<T>>) -> Result<GenDisk<T, Initialized>> { Why is this not inside of an `impl` block of `GenDisk<T, Initialized>`? > + 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, > + }; > + > + // 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 > + // `__blk_mq_alloc_disk` above. > + Ok(GenDisk { > + _tagset: tagset, > + gendisk, > + _phantom: PhantomData, > + }) > +} > diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs > new file mode 100644 > index 000000000000..3bd1af2c2260 > --- /dev/null > +++ b/rust/kernel/block/mq/operations.rs > @@ -0,0 +1,245 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! This module provides an interface for blk-mq drivers to implement. > +//! > +//! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) > + > +use crate::{ > + bindings, > + block::mq::request::RequestDataWrapper, > + block::mq::Request, > + error::{from_result, Result}, > + types::ARef, > +}; > +use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering}; > + > +/// Implement this trait to interface blk-mq as block devices. > +/// > +/// To implement a block device driver, implement this trait as described in the > +/// [module level documentation]. The kernel will use the implementation of the > +/// functions defined in this trait to interface a block device driver. Note: > +/// There is no need for an exit_request() implementation, because the `drop` > +/// implementation of the [`Request`] type will be invoked by automatically by > +/// the C/Rust glue logic. This text is wrapped to 80 columns, but our usual wrapping is 100. > +/// > +/// [module level documentation]: kernel::block::mq > +#[macros::vtable] > +pub trait Operations: Sized { > + /// 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; > + > + /// Called by the kernel to indicate that queued requests should be submitted. > + fn commit_rqs(); > + > + /// Called by the kernel when the request is completed. > + fn complete(_rq: ARef<Request<Self>>); Is there a reason for the `_`? To me it seems this is called when the given request is already done, so would it make more sense to name it `completed` or `on_completion`? > + > + /// Called by the kernel to poll the device for completed requests. Only > + /// used for poll queues. > + fn poll() -> bool { > + crate::build_error(crate::error::VTABLE_DEFAULT_ERROR) > + } > +} > + > +/// A vtable for blk-mq to interact with a block device driver. > +/// > +/// A `bindings::blk_mq_opa` vtable is constructed from pointers to the `extern > +/// "C"` functions of this struct, exposed through the `OperationsVTable::VTABLE`. > +/// > +/// For general documentation of these methods, see the kernel source > +/// documentation related to `struct blk_mq_operations` in > +/// [`include/linux/blk-mq.h`]. > +/// > +/// [`include/linux/blk-mq.h`]: srctree/include/linux/blk-mq.h > +pub(crate) struct OperationsVTable<T: Operations>(PhantomData<T>); > + > +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. > + /// - 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 > + 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 > + // 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); > + > + let rq = > + // 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. > + unsafe {Request::aref_from_raw((*bd).rq)}; Can you put the SAFETY comment above the line, then the formatting is more natural. > + > + // 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. 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. `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. What do you mean by the first sentence? Where was this refcount leaked? > + 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. > + /// > + /// # 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 > + /// 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 > + /// 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, > + _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. > + 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) > + }) > + } > + > + /// 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. `rq` must > + /// point to a request that was initialized by a call to > + /// `Self::init_request_callback`. > + unsafe extern "C" fn exit_request_callback( > + _set: *mut bindings::blk_mq_tag_set, > + rq: *mut bindings::request, > + _hctx_idx: core::ffi::c_uint, > + ) { > + // SAFETY: The tagset invariants guarantee that all requests are allocated with extra memory > + // for the request data. > + let pdu = unsafe { bindings::blk_mq_rq_to_pdu(rq) }.cast::<RequestDataWrapper>(); > + > + // SAFETY: `pdu` is valid for read and write and is properly initialised. > + unsafe { core::ptr::drop_in_place(pdu) }; > + } > + > + const VTABLE: bindings::blk_mq_ops = bindings::blk_mq_ops { > + queue_rq: Some(Self::queue_rq_callback), > + queue_rqs: None, > + commit_rqs: Some(Self::commit_rqs_callback), > + get_budget: None, > + put_budget: None, > + set_rq_budget_token: None, > + get_rq_budget_token: None, > + timeout: None, > + poll: if T::HAS_POLL { > + Some(Self::poll_callback) > + } else { > + None > + }, > + complete: Some(Self::complete_callback), > + init_hctx: Some(Self::init_hctx_callback), > + exit_hctx: Some(Self::exit_hctx_callback), > + init_request: Some(Self::init_request_callback), > + exit_request: Some(Self::exit_request_callback), > + cleanup_rq: None, > + busy: None, > + map_queues: None, > + #[cfg(CONFIG_BLK_DEBUG_FS)] > + show_rq: None, > + }; > + > + pub(crate) const fn build() -> &'static bindings::blk_mq_ops { > + &Self::VTABLE > + } > +} > diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs > new file mode 100644 > index 000000000000..4f7e4692b592 > --- /dev/null > +++ b/rust/kernel/block/mq/raw_writer.rs > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +use core::fmt::{self, Write}; > + > +use crate::error::Result; > +use crate::prelude::EINVAL; > + > +/// A mutable reference to a byte buffer where a string can be written into. > +/// > +/// # Invariants > +/// > +/// `buffer` is always null terminated. I don't know how valuable this would be, but you could only ask for this invariant, if `buffer` is non-empty. Then you could have the `new` and `from_array` functions return a `RawWriter` without result. > +pub(crate) struct RawWriter<'a> { > + buffer: &'a mut [u8], > + pos: usize, > +} > + > +impl<'a> RawWriter<'a> { > + /// Create a new `RawWriter` instance. > + fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> { > + *(buffer.last_mut().ok_or(EINVAL)?) = 0; > + > + // INVARIANT: We null terminated the buffer above Missing `.`. > + Ok(Self { buffer, pos: 0 }) > + } > + > + pub(crate) fn from_array<const N: usize>( I think this could be a `From` impl (if you decide to change the invariant). > + a: &'a mut [core::ffi::c_char; N], > + ) -> Result<RawWriter<'a>> { > + Self::new( > + // SAFETY: the buffer of `a` is valid for read and write as `u8` for > + // at least `N` bytes. > + unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) }, > + ) > + } > +} > + > +impl Write for RawWriter<'_> { > + fn write_str(&mut self, s: &str) -> fmt::Result { > + let bytes = s.as_bytes(); > + let len = bytes.len(); > + > + // We do not want to overwrite our null terminator > + if self.pos + len > self.buffer.len() - 1 { > + return Err(fmt::Error); > + } > + > + // INVARIANT: We are not overwriting the last byte > + self.buffer[self.pos..self.pos + len].copy_from_slice(bytes); > + > + self.pos += len; > + > + Ok(()) > + } > +} > diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs > new file mode 100644 > index 000000000000..db5d760615d7 > --- /dev/null > +++ b/rust/kernel/block/mq/request.rs > @@ -0,0 +1,227 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! This module provides a wrapper for the C `struct request` type. > +//! > +//! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) > + > +use crate::{ > + bindings, > + block::mq::Operations, > + error::Result, > + types::{ARef, AlwaysRefCounted, Opaque}, > +}; > +use core::{ > + marker::PhantomData, > + ptr::{addr_of_mut, NonNull}, > + sync::atomic::{AtomicU64, Ordering}, > +}; > + > +/// A wrapper around a blk-mq `struct request`. This represents an IO request. > +/// > +/// # Invariants > +/// > +/// * `self.0` is a valid `struct request` created by the C portion of the kernel. > +/// * The private data area associated with this request must be an initialized > +/// and valid `RequestDataWrapper<T>`. > +/// * `self` is reference counted by atomic modification of > +/// self.wrapper_ref().refcount(). > +/// > +#[repr(transparent)] > +pub struct Request<T: Operations>(Opaque<bindings::request>, PhantomData<T>); > + > +impl<T: Operations> Request<T> { > + /// Create an `ARef<Request>` from a `struct request` pointer. > + /// > + /// # Safety > + /// > + /// * The caller must own a refcount on `ptr` that is transferred to the > + /// returned `ARef`. > + /// * The type invariants for `Request` must hold for the pointee of `ptr`. > + pub(crate) unsafe fn aref_from_raw(ptr: *mut bindings::request) -> ARef<Self> { > + // INVARIANTS: By the safety requirements of this function, invariants are upheld. Typo: INVARIANTS -> INVARIANT > + // SAFETY: By the safety requirement of this function, we own a > + // reference count that we can pass to `ARef`. > + unsafe { ARef::from_raw(NonNull::new_unchecked(ptr as *const Self as *mut Self)) } > + } > + > + /// 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. > + 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>> { I am not yet fully convinced that this is the way we should go. I think I would have to see a more complex usage of `Request` with that id <-> request mapping that you mentioned. Because with how rnull uses this API, it could also have a `URef<Self>` parameter (URef := unique ARef). > + let this = Self::try_set_end(this)?; > + let request_ptr = this.0.get(); > + core::mem::forget(this); Would be a good idea to mention who is going to own this refcount. > + > + // SAFETY: By type invariant, `self.0` is a valid `struct request`. By > + // existence of `&mut self` we have exclusive access. > + 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 > + /// > + /// - `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. > + 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) } > + } > + > + /// Return a reference to the `RequestDataWrapper` stored in the private > + /// area of the request structure. > + pub(crate) fn wrapper_ref(&self) -> &RequestDataWrapper { > + // SAFETY: By type invariant, `self.0` is a valid alocation. Further, > + // the private data associated with this request is initialized and > + // valid. The existence of `&self` guarantees that the private data is > + // valid as a shared reference. > + unsafe { Self::wrapper_ptr(self as *const Self as *mut Self).as_ref() } > + } > +} > + > +/// A wrapper around data stored in the private area of the C `struct request`. > +pub(crate) struct RequestDataWrapper { Why is this called `Wrapper`? It doesn't really wrap anything, `RequestData` seems fine. > + /// The Rust request refcount has the following states: > + /// > + /// - 0: The request is owned by C block layer. > + /// - 1: The request is owned by Rust abstractions but there are no ARef references to it. > + /// - 2+: There are `ARef` references to the request. > + refcount: AtomicU64, > +} > + > +impl RequestDataWrapper { > + /// Return a reference to the refcount of the request that is embedding > + /// `self`. > + pub(crate) fn refcount(&self) -> &AtomicU64 { > + &self.refcount > + } > + > + /// Return a pointer to the refcount of the request that is embedding the > + /// pointee of `this`. > + /// > + /// # Safety > + /// > + /// - `this` must point to a live allocation of at least the size of `Self`. > + pub(crate) unsafe fn refcount_ptr(this: *mut Self) -> *mut AtomicU64 { > + // SAFETY: Because of the safety requirements of this function, the > + // field projection is safe. > + unsafe { addr_of_mut!((*this).refcount) } > + } > +} > + > +// SAFETY: Exclusive access is thread-safe for `Request`. `Request` has no `&mut > +// self` methods and `&self` methods that mutate `self` are internally > +// synchronzied. > +unsafe impl<T: Operations> Send for Request<T> {} > + > +// SAFETY: Shared access is thread-safe for `Request`. `&self` methods that > +// mutate `self` are internally synchronized` > +unsafe impl<T: Operations> Sync for Request<T> {} > + > +/// Store the result of `op(target.load())` in target, returning new value of > +/// taret. > +fn atomic_relaxed_op_return(target: &AtomicU64, op: impl Fn(u64) -> u64) -> u64 { > + let mut old = target.load(Ordering::Relaxed); > + loop { > + match target.compare_exchange_weak(old, op(old), Ordering::Relaxed, Ordering::Relaxed) { > + Ok(_) => break, > + Err(x) => { > + old = x; > + } > + } > + } This seems like a reimplementation of `AtomicU64::fetch_update` to me. > + > + op(old) > +} > + > +/// Store the result of `op(target.load)` in `target` if `target.load() != > +/// pred`, returning previous value of target > +fn atomic_relaxed_op_unless(target: &AtomicU64, op: impl Fn(u64) -> u64, pred: u64) -> bool { > + let x = target.load(Ordering::Relaxed); > + loop { > + if x == pred { > + break; > + } > + if target > + .compare_exchange_weak(x, op(x), Ordering::Relaxed, Ordering::Relaxed) > + .is_ok() > + { > + break; > + } > + } > + > + x == pred > +} > + > +// SAFETY: All instances of `Request<T>` are reference counted. This > +// implementation of `AlwaysRefCounted` ensure that increments to the ref count > +// keeps the object alive in memory at least until a matching reference count > +// decrement is executed. > +unsafe impl<T: Operations> AlwaysRefCounted for Request<T> { > + fn inc_ref(&self) { > + let refcount = &self.wrapper_ref().refcount(); > + > + #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))] Another option would be to use `_updated` as the name of the variable. I personally would prefer that. What do you guys think? > + let updated = atomic_relaxed_op_unless(refcount, |x| x + 1, 0); > + > + #[cfg(CONFIG_DEBUG_MISC)] > + if !updated { > + panic!("Request refcount zero on clone") > + } > + } > + > + unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { > + // SAFETY: The type invariants of `ARef` guarantee that `obj` is valid > + // for read. > + let wrapper_ptr = unsafe { Self::wrapper_ptr(obj.as_ptr()).as_ptr() }; > + // SAFETY: The type invariant of `Request` guarantees that the private > + // data area is initialized and valid. > + let refcount = unsafe { &*RequestDataWrapper::refcount_ptr(wrapper_ptr) }; > + > + #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))] > + let new_refcount = atomic_relaxed_op_return(refcount, |x| x - 1); > + > + #[cfg(CONFIG_DEBUG_MISC)] > + if new_refcount == 0 { > + panic!("Request reached refcount zero in Rust abstractions"); > + } > + } > +} > diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs > new file mode 100644 > index 000000000000..4217c2b03ff3 > --- /dev/null > +++ b/rust/kernel/block/mq/tag_set.rs > @@ -0,0 +1,93 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! This module provides the `TagSet` struct to wrap the C `struct blk_mq_tag_set`. > +//! > +//! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) > + > +use core::pin::Pin; > + > +use crate::{ > + bindings, > + block::mq::request::RequestDataWrapper, > + block::mq::{operations::OperationsVTable, Operations}, > + error::{self, Error, Result}, > + prelude::PinInit, > + try_pin_init, > + types::Opaque, > +}; > +use core::{convert::TryInto, marker::PhantomData}; > +use macros::{pin_data, pinned_drop}; > + > +/// A wrapper for the C `struct blk_mq_tag_set`. > +/// > +/// `struct blk_mq_tag_set` contains a `struct list_head` and so must be pinned. > +#[pin_data(PinnedDrop)] > +#[repr(transparent)] > +pub struct TagSet<T: Operations> { > + #[pin] > + inner: Opaque<bindings::blk_mq_tag_set>, > + _p: PhantomData<T>, > +} > + > +impl<T: Operations> TagSet<T> { > + /// Try to create a new tag set > + pub fn try_new( > + nr_hw_queues: u32, > + num_tags: u32, > + num_maps: u32, > + ) -> impl PinInit<Self, error::Error> { > + try_pin_init!( TagSet { > + inner <- unsafe {kernel::init::pin_init_from_closure(move |place: *mut Opaque<bindings::blk_mq_tag_set>| -> Result<()> { We currently do not have `Opaque::try_ffi_init`, I vaguely remember that we talked about it. Do you mind adding it? Otherwise I can also send a patch. > + let place = place.cast::<bindings::blk_mq_tag_set>(); > + > + // SAFETY: try_ffi_init promises that `place` is writable, and > + // zeroes is a valid bit pattern for this structure. > + core::ptr::write_bytes(place, 0, 1); > + > + /// For a raw pointer to a struct, write a struct field without > + /// creating a reference to the field > + macro_rules! write_ptr_field { > + ($target:ident, $field:ident, $value:expr) => { > + ::core::ptr::write(::core::ptr::addr_of_mut!((*$target).$field), $value) > + }; > + } > + > + // SAFETY: try_ffi_init promises that `place` is writable > + write_ptr_field!(place, ops, OperationsVTable::<T>::build()); > + write_ptr_field!(place, nr_hw_queues , nr_hw_queues); > + write_ptr_field!(place, timeout , 0); // 0 means default which is 30 * HZ in C > + write_ptr_field!(place, numa_node , bindings::NUMA_NO_NODE); > + write_ptr_field!(place, queue_depth , num_tags); > + write_ptr_field!(place, cmd_size , core::mem::size_of::<RequestDataWrapper>().try_into()?); > + write_ptr_field!(place, flags , bindings::BLK_MQ_F_SHOULD_MERGE); > + write_ptr_field!(place, driver_data , core::ptr::null_mut::<core::ffi::c_void>()); > + write_ptr_field!(place, nr_maps , num_maps); I think that there is some way for pinned-init to do a better job here. I feel like we ought to be able to just write: Opaque::init( try_init!(bindings::blk_mq_tag_set { ops: OperationsVTable::<T>::build(), nr_hw_queues, timeout: 0, // 0 means default, which is 30Hz numa_node: bindings::NUMA_NO_NODE, queue_depth: num_tags, cmd_size: size_of::<RequestDataWrapper>().try_into()?, flags: bindings::BLK_MQ_F_SHOULD_MERGE, driver_data: null_mut(), nr_maps: num_maps, ..Zeroable::zeroed() }? Error) .chain(|tag_set| to_result(bindings::blk_mq_alloc_tag_set(tag_set))) ) But we don't have `Opaque::init` (shouldn't be difficult) and `bindings::blk_mq_tag_set` doesn't implement `Zeroable`. We would need bindgen to put `derive(Zeroable)` on certain structs... Another option would be to just list the fields explicitly, since there aren't that many. What do you think? > + > + // SAFETY: Relevant fields of `place` are initialised above > + let ret = bindings::blk_mq_alloc_tag_set(place); > + if ret < 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(()) > + })}, > + _p: PhantomData, > + }) > + } > + > + /// Return the pointer to the wrapped `struct blk_mq_tag_set` > + pub(crate) fn raw_tag_set(&self) -> *mut bindings::blk_mq_tag_set { > + self.inner.get() > + } > +} > + > +#[pinned_drop] > +impl<T: Operations> PinnedDrop for TagSet<T> { > + fn drop(self: Pin<&mut Self>) { > + // SAFETY: We are not moving self below > + let this = unsafe { Pin::into_inner_unchecked(self) }; You don't need to unwrap the `Pin`, since you only need access to `&Self` and `Pin` always allows you to do that. (so just use `self` instead of `this` below) > + > + // SAFETY: `inner` is valid and has been properly initialised during construction. Should be an invariant. --- Cheers, Benno > + unsafe { bindings::blk_mq_free_tag_set(this.inner.get()) }; > + } > +} > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs > index 55280ae9fe40..145f5c397009 100644 > --- a/rust/kernel/error.rs > +++ b/rust/kernel/error.rs > @@ -126,6 +126,12 @@ pub fn to_errno(self) -> core::ffi::c_int { > self.0 > } > > + #[cfg(CONFIG_BLOCK)] > + pub(crate) fn to_blk_status(self) -> bindings::blk_status_t { > + // SAFETY: `self.0` is a valid error due to its invariant. > + unsafe { bindings::errno_to_blk_status(self.0) } > + } > + > /// Returns the error encoded as a pointer. > #[allow(dead_code)] > pub(crate) fn to_ptr<T>(self) -> *mut T { > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 9a943d99c71a..79be44281fb4 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -27,6 +27,8 @@ > extern crate self as kernel; > > pub mod alloc; > +#[cfg(CONFIG_BLOCK)] > +pub mod block; > mod build_assert; > pub mod error; > pub mod init; > -- > 2.44.0 > >
