| Message ID | 20240601081806.531954-3-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust block device driver API and null block driver | expand |
On 01.06.24 10:18, Andreas Hindborg wrote: > From: Andreas Hindborg <a.hindborg@samsung.com> > > This patch adds an initial version of the Rust null block driver. > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > --- > drivers/block/Kconfig | 9 +++++ > drivers/block/Makefile | 3 ++ > drivers/block/rnull.rs | 78 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+) > create mode 100644 drivers/block/rnull.rs I left two comments below, but it already seems fine to me: Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > 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..c69ea7e7436e > --- /dev/null > +++ b/drivers/block/rnull.rs > @@ -0,0 +1,78 @@ > +// 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>>>>, > +} > + > +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); > + } > + > + let mut disk = gen_disk::GenDisk::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() > + }?; Personally, I would prefer to put the `?` into the line above. > + > + let disk = Box::pin_init(new_mutex!(disk, "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("Fatal error - expected to be able to end request"); I expected something more along the lines of: "expected to be able to end request, since `NullBlkDevice` never takes refcounts on Requests and as such the ARef must be unique, but `end_ok` only fails if that is not the case". But maybe that would fit better in a comment, what do you think? > + > + Ok(()) > + } > + > + fn commit_rqs() {} > +} > -- > 2.45.1 >
Benno Lossin <benno.lossin@proton.me> writes: >> +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); >> + } >> + >> + let mut disk = gen_disk::GenDisk::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() >> + }?; > > Personally, I would prefer to put the `?` into the line above. I have no strong opinion here. > >> + >> + let disk = Box::pin_init(new_mutex!(disk, "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("Fatal error - expected to be able to end request"); > > I expected something more along the lines of: "expected to be able to > end request, since `NullBlkDevice` never takes refcounts on Requests and > as such the ARef must be unique, but `end_ok` only fails if that is not > the case". But maybe that would fit better in a comment, what do you > think? I can add a comment
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..c69ea7e7436e --- /dev/null +++ b/drivers/block/rnull.rs @@ -0,0 +1,78 @@ +// 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>>>>, +} + +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); + } + + let mut disk = gen_disk::GenDisk::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() + }?; + + let disk = Box::pin_init(new_mutex!(disk, "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("Fatal error - expected to be able to end request"); + + Ok(()) + } + + fn commit_rqs() {} +}