Re: [PATCH v2 2/3] rust: block: add rnull, Rust null_blk implementation

From: Andreas Hindborg
Date: Wed May 29 2024 - 09:01:15 EST

Benno Lossin <benno.lossin@xxxxxxxxx> writes:

> On 21.05.24 16:03, Andreas Hindborg wrote:
>> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>>
>> This patch adds an initial version of the Rust null block driver.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>> ---
>> drivers/block/Kconfig | 9 +++++
>> drivers/block/Makefile | 3 ++
>> drivers/block/rnull.rs | 86 +++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/block/mq.rs | 4 +-
>> 4 files changed, 101 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/block/rnull.rs
>>
>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
>> index 5b9d4aaebb81..ed209f4f2798 100644
>> --- a/drivers/block/Kconfig
>> +++ b/drivers/block/Kconfig
>> @@ -354,6 +354,15 @@ config VIRTIO_BLK
>> This is the virtual block driver for virtio. It can be used with
>> QEMU based VMMs (like KVM or Xen). Say Y or M.
>>
>> +config BLK_DEV_RUST_NULL
>> + tristate "Rust null block driver (Experimental)"
>> + depends on RUST
>> + help
>> + This is the Rust implementation of the null block driver. For now it
>> + is only a minimal stub.
>> +
>> + If unsure, say N.
>> +
>> config BLK_DEV_RBD
>> tristate "Rados block device (RBD)"
>> depends on INET && BLOCK
>> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
>> index 101612cba303..1105a2d4fdcb 100644
>> --- a/drivers/block/Makefile
>> +++ b/drivers/block/Makefile
>> @@ -9,6 +9,9 @@
>> # needed for trace events
>> ccflags-y += -I$(src)
>>
>> +obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
>> +rnull_mod-y := rnull.o
>> +
>> obj-$(CONFIG_MAC_FLOPPY) += swim3.o
>> obj-$(CONFIG_BLK_DEV_SWIM) += swim_mod.o
>> obj-$(CONFIG_BLK_DEV_FD) += floppy.o
>> diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs
>> new file mode 100644
>> index 000000000000..1d6ab6f0f26f
>> --- /dev/null
>> +++ b/drivers/block/rnull.rs
>> @@ -0,0 +1,86 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! This is a Rust implementation of the C null block driver.
>> +//!
>> +//! Supported features:
>> +//!
>> +//! - blk-mq interface
>> +//! - direct completion
>> +//! - block size 4k
>> +//!
>> +//! The driver is not configurable.
>> +
>> +use kernel::{
>> + alloc::flags,
>> + block::mq::{
>> + self,
>> + gen_disk::{self, GenDisk},
>> + Operations, TagSet,
>> + },
>> + error::Result,
>> + new_mutex, pr_info,
>> + prelude::*,
>> + sync::{Arc, Mutex},
>> + types::ARef,
>> +};
>> +
>> +module! {
>> + type: NullBlkModule,
>> + name: "rnull_mod",
>> + author: "Andreas Hindborg",
>> + license: "GPL v2",
>> +}
>> +
>> +struct NullBlkModule {
>> + _disk: Pin<Box<Mutex<GenDisk<NullBlkDevice, gen_disk::Added>>>>,
>> +}
>> +
>> +fn add_disk(tagset: Arc<TagSet<NullBlkDevice>>) -> Result<GenDisk<NullBlkDevice, gen_disk::Added>> {
>
> Any reason that this is its own function and not in the
> `NullBlkModule::init` function?

Would you embed it inside the `init` function? To what end? I don't
think that would read well.

>
>> + let block_size: u16 = 4096;
>> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>> + return Err(kernel::error::code::EINVAL);
>> + }
>> +
>> + let mut disk = gen_disk::try_new(tagset)?;
>> + disk.set_name(format_args!("rnullb{}", 0))?;
>> + disk.set_capacity_sectors(4096 << 11);
>> + disk.set_queue_logical_block_size(block_size.into());
>> + disk.set_queue_physical_block_size(block_size.into());
>> + disk.set_rotational(false);
>> + disk.add()
>> +}
>> +
>> +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 = Box::pin_init(
>> + new_mutex!(add_disk(tagset)?, "nullb:disk"),
>> + flags::GFP_KERNEL,
>> + )?;
>> +
>> + Ok(Self { _disk: disk })
>> + }
>> +}
>> +
>> +struct NullBlkDevice;
>> +
>> +#[vtable]
>> +impl Operations for NullBlkDevice {
>> + #[inline(always)]
>> + fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>> + mq::Request::end_ok(rq)
>> + .map_err(|_e| kernel::error::code::EIO)
>> + .expect("Failed to complete request");
>
> This error would only happen if `rq` is not the only ARef to that
> request, right?

Yes, it should never happen. If it happens, something is seriously
broken and panic is adequate.

Other drivers might want to retry later or something, but in this case
it should just work.

>
>> +
>> + Ok(())
>> + }
>> +
>> + fn commit_rqs() {}
>> +
>> + fn complete(rq: ARef<mq::Request<Self>>) {
>
> Am I correct in thinking that this function is never actually called,
> since all requests that are queued are immediately ended?

Yes, re my other email. It is a callback that cannot be triggered for
now. I will move it to a later patch where it belongs.

>
>> + mq::Request::end_ok(rq)
>> + .map_err(|_e| kernel::error::code::EIO)
>> + .expect("Failed to complete request")
>> + }
>> +}
>> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
>> index efbd2588791b..54e032bbdffd 100644
>> --- a/rust/kernel/block/mq.rs
>> +++ b/rust/kernel/block/mq.rs
>> @@ -51,6 +51,7 @@
>> //!
>> //! ```rust
>> //! use kernel::{
>> +//! alloc::flags,
>> //! block::mq::*,
>> //! new_mutex,
>> //! prelude::*,
>> @@ -77,7 +78,8 @@
>> //! }
>> //! }
>> //!
>> -//! let tagset: Arc<TagSet<MyBlkDevice>> = Arc::pin_init(TagSet::try_new(1, 256, 1))?;
>> +//! let tagset: Arc<TagSet<MyBlkDevice>> =
>> +//! Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>
> This change should probably be in the patch before (seems like an
> artifact from rebasing).

Yes, thank you for spotting that. I thought I checked that this was
building, so this is strange to me. Maybe I failed to build the
doctests between the two patches. It is weird that kernel robot did not
pick this up. Maybe it is not building doctests?


Best regards,
Andreas