Re: [PATCH v2 1/3] rust: block: introduce `kernel::block::mq` module

[Andreas Hindborg]

Benno Lossin <benno.lossin@xxxxxxxxx> writes:

> On 21.05.24 16:03, Andreas Hindborg wrote:
>> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>

[...]

>> >> +
>> +//! 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>`].

`GenDisk` is not in scope, so short link is not working. But I can do
whatever this is called:


//! - Create a [`GenDisk<T>`], passing in the `TagSet` reference
..
//! [`GenDisk<T>`]: gen_disk::GenDisk

Would you prefer that?

>
>> +//! - 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)

`TagSet` was linked above. Would you link it on each mention?

[...]

>> +//!
>> +//!     fn commit_rqs(
>> +//!     ) {
>> +//!     }
>
> Formatting.

I would love if `rustfmt` would do this. But I think it is both unstable
and broken for examples like this [1]. I'll fix it up by hand.

How do you manage formatting in examples? By hand?

[...]

>> +/// 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`?

Yes, that would make sense.

>
>> +    _tagset: Arc<TagSet<T>>,
>
> Does this really need a leading underscore?

It does not.

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

Yes.

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

I was considering the layout not being deterministic for `repr(Rust)`. I
think that in practice it will be the case that the two types will have
the same layout, but I could not find the documentation that states
this. Nomicon does not talk about zero sized types [2].

[...]

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

Yes. I will update.

[...]

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

Thanks, this is very useful. I will add it.

>
>> +
>> +        // 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>`?

No particular reason. I should probably move it.

[...]

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

This had me scratch my head for a bit. I run `make rustfmt` and `make
rustfmtcheck`, so I had no idea why my code would be formatted
incorrect. It took me a while to figure out that we are not enabling
`comment_width = 100`, presumably because it is an unstable `rustfmt`
feature. I am not sure what the correct way to enable it but I hacked
the Makefile and enabled it. It gives a huge diff all across the kernel
crate.

So, it seems we _are_ in fact using 80 line fill column, since that is
what much of our existing code is using, and that is what the build
system is configured to use.

Where did you come across the 100 character fill column?

Anyways, we should configure our tools to the standard we want. I don't
care if it's 80 or 100, as long as I can have the tools do the job for
me.

Let's discuss this at next meeting.

>
>> +///
>> +/// [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 `_`?

Copy pasta probably. Will remove.

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

I would agree. But we had a bit of discussion at LSF about naming things
differently in Rust vs C. C people prefer if we keep the C names, even
if they do not make sense to the people who write the new Rust code.

In C, the vtable entry is called `complete_callback` and the called
symbol is usually `my_driver_complete_rq`.

We could go with `completed`, `completed_callback`, or `complete_rq`.
Although `completed` sounds like it should return a bool indicating
whether the request was already completed.

I think I'll leave it for now, and we can always change it if we come up
with a really good name.

>
>> +
>> +    /// 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.

Yes.

>
>> +
>> +        // 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?

I can understand why you are confused. `Request::complete` is in my
outgoing patch queue. There is no way to call this function in this
patch. I will move it to a later patch. Thanks.

[...]

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

I think guaranteeing at least a null character is always written by
`write_str` is a good thing. It is used for writing C strings to device
name fields. `write_str` with a zero size buffer would give undesirable
results, and is probably not what the caller wants.

>
>> +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 `.`.

👍

[...]

>> +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).

I considered a `UniqueARef` but it would just move the error handing to
`ARef::into_unique` and then make `end_ok` infallible.

There are four states for a request that we need to track:

A) Request is owned by block layer (refcount 0)
B) Request is owned by driver but with zero `ARef`s in existence
   (refcount 1)
C) Request is owned by driver with exactly one `ARef` in existence
   (refcount 2)
D) Request is owned by driver with more than one `ARef` in existence
   (refcount > 2)

It is in the doc comments for `RequestDataWrapper` as well.

We need A and B to ensure we fail tag to request conversions for
requests that are not owned by the driver.

We need C and D to ensure that it is safe to end the request and hand back
ownership to the block layer.

I will ping you when I hook up the NVMe driver with this.

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

The refcount is zero after `try_set_end`, so there is no owner of the
count. The request will be in state A and thus block layer owns the
request. Block layer does not honor this refcount, it is only for the
driver to know.

Perhaps I should move the explanation up into the docs for `Request`.

>
>> +
>> +        // 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.

It will eventually wrap private data associated with the request. Those
patches will be submitted later. Should I change the name in the
meanwhile?

>
>> +    /// 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.

It looks like it! Except this function is returning the updated value,
`fetch_update` is returning the old value.

Would you rather that I rewrite in terms of the library function?

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

Either way is fine by me.

[...]

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

I have a `try_ffi_init` patch queued. I removed it from here to cut
dependencies. I will submit it soon after this is in.

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

Both options sound good. Ofc the first one sounds more user friendly
while the latter one sounds easier to implement. Getting rid of the
unsafe blocks here would be really nice.

>
>> +
>> +                // 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)

Thanks 👍

>
>> +
>> +        // SAFETY: `inner` is valid and has been properly initialised during construction.
>
> Should be an invariant.

Ok 👍

Thanks for the review! I will send a new version.


Best regards,
Andreas


[1] https://github.com/rust-lang/rustfmt/issues/3348
[2] https://doc.rust-lang.org/nomicon/repr-rust.html#reprrust