Hi Andreas,
just so you know, I received this email today, so it was very late,
since the send date is January 12.
On 12.01.24 10:18, Andreas Hindborg (Samsung) wrote:
>
> Hi Benno,
>
> Benno Lossin <benno.lossin@xxxxxxxxx> writes:
>
> <...>
>
>>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>>> new file mode 100644
>>> index 000000000000..50496af15bbf
>>> --- /dev/null
>>> +++ b/rust/kernel/block/mq/gen_disk.rs
>>> @@ -0,0 +1,133 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! GenDisk abstraction
>>> +//!
>>> +//! C header: [`include/linux/blkdev.h`](../../include/linux/blkdev.h)
>>> +//! C header: [`include/linux/blk_mq.h`](../../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, types::ForeignOwnable,
>>> + types::ScopeGuard,
>>> +};
>>> +use core::fmt::{self, Write};
>>> +
>>> +/// A generic block device
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>>> +pub struct GenDisk<T: Operations> {
>>> + _tagset: Arc<TagSet<T>>,
>>> + gendisk: *mut bindings::gendisk,
>>
>> Why are these two fields not embedded? Shouldn't the user decide where
>> to allocate?
>
> The `TagSet` can be shared between multiple `GenDisk`. Using an `Arc`
> seems resonable?
>
> For the `gendisk` field, the allocation is done by C and the address
> must be stable. We are owning the pointee and must drop it when it goes out
> of scope. I could do this:
>
> #[repr(transparent)]
> struct GenDisk(Opaque<bindings::gendisk>);
>
> struct UniqueGenDiskRef {
> _tagset: Arc<TagSet<T>>,
> gendisk: Pin<&'static mut GenDisk>,
>
> }
>
> but it seems pointless. `struct GenDisk` would not be pub in that case. What do you think?
Hmm, I am a bit confused as to how you usually use a `struct gendisk`.
You said that a `TagSet` might be shared between multiple `GenDisk`s,
but that is not facilitated by the C side?
Is it the case that on the C side you create a struct containing a
tagset and a gendisk for every block device you want to represent?
And you decided for the Rust abstractions that you want to have only a
single generic struct for any block device, distinguished by the generic
parameter?
I think these kinds of details would be nice to know. Not only for
reviewers, but also for veterans of the C APIs.
--
Cheers,
Benno
