Received-SPF: pass (google.com: domain of linux-kernel+bounces-197807-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Andreas Hindborg <nmi@metaspace.dk>
To: Benno Lossin <benno.lossin@proton.me>
Cc: Jens Axboe <axboe@kernel.dk>,  Christoph Hellwig <hch@lst.de>,  Keith
 Busch <kbusch@kernel.org>,  Damien Le Moal <dlemoal@kernel.org>,  Bart Van
 Assche <bvanassche@acm.org>,  Hannes Reinecke <hare@suse.de>,  Ming Lei
 <ming.lei@redhat.com>,  "linux-block@vger.kernel.org"
 <linux-block@vger.kernel.org>,  Andreas Hindborg <a.hindborg@samsung.com>,
  Greg KH <gregkh@linuxfoundation.org>,  Matthew Wilcox
 <willy@infradead.org>,  Miguel Ojeda <ojeda@kernel.org>,  Alex Gaynor
 <alex.gaynor@gmail.com>,  Wedson Almeida Filho <wedsonaf@gmail.com>,
  Boqun Feng <boqun.feng@gmail.com>,  Gary Guo <gary@garyguo.net>,
  =?utf-8?Q?Bj=C3=B6rn?=
 Roy Baron <bjorn3_gh@protonmail.com>,  Alice Ryhl <aliceryhl@google.com>,
  Chaitanya Kulkarni <chaitanyak@nvidia.com>,  Luis Chamberlain
 <mcgrof@kernel.org>,  Yexuan Yang <1182282462@bupt.edu.cn>,  Sergio
 =?utf-8?Q?Gonz=C3=A1lez?= Collado <sergio.collado@gmail.com>,  Joel
 Granados
 <j.granados@samsung.com>,  "Pankaj Raghav (Samsung)"
 <kernel@pankajraghav.com>,  Daniel Gomez <da.gomez@samsung.com>,  Niklas
 Cassel <Niklas.Cassel@wdc.com>,  Philipp Stanner <pstanner@redhat.com>,
  Conor Dooley <conor@kernel.org>,  Johannes Thumshirn
 <Johannes.Thumshirn@wdc.com>,  Matias =?utf-8?Q?Bj=C3=B8rling?=
 <m@bjorling.me>,  open list
 <linux-kernel@vger.kernel.org>,  "rust-for-linux@vger.kernel.org"
 <rust-for-linux@vger.kernel.org>,  "lsf-pc@lists.linux-foundation.org"
 <lsf-pc@lists.linux-foundation.org>,  "gost.dev@samsung.com"
 <gost.dev@samsung.com>
Subject: Re: [PATCH v3 2/3] rust: block: add rnull, Rust null_blk
 implementation
In-Reply-To: <2021ea8f-a3df-46c6-8fdf-61b1df231773@proton.me> (Benno Lossin's
	message of "Sat, 01 Jun 2024 09:15:59 +0000")
References: <20240601081806.531954-1-nmi@metaspace.dk>
	<20240601081806.531954-3-nmi@metaspace.dk>
	<2021ea8f-a3df-46c6-8fdf-61b1df231773@proton.me>
Date: Sat, 01 Jun 2024 13:15:54 +0200
Message-ID: <87ed9goqvp.fsf@metaspace.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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 =3D Arc::pin_init(TagSet::try_new(1, 256, 1), flags:=
:GFP_KERNEL)?;
>> +
>> +        let disk =3D {
>> +            let block_size: u16 =3D 4096;
>> +            if block_size % 512 !=3D 0 || !(512..=3D4096).contains(&blo=
ck_size) {
>> +                return Err(kernel::error::code::EINVAL);
>> +            }
>> +
>> +            let mut disk =3D 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 =3D Box::pin_init(new_mutex!(disk, "nullb:disk"), flag=
s::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 =F0=9F=91=8D

BR Andreas