Re: [RFC PATCH 4/5] rust: block: add rnull, Rust null_blk implementation

From: Benno Lossin <benno.lossin@proton.me>
To: Andreas Hindborg <nmi@metaspace.dk>
Cc: "Jens Axboe" <axboe@kernel.dk>, "Christoph Hellwig" <hch@lst.de>,
	"Keith Busch" <kbusch@kernel.org>,
	"Damien Le Moal" <Damien.LeMoal@wdc.com>,
	"Bart Van Assche" <bvanassche@acm.org>,
	"Hannes Reinecke" <hare@suse.de>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"Andreas Hindborg" <a.hindborg@samsung.com>,
	"Niklas Cassel" <Niklas.Cassel@wdc.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>,
	"Björn 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 González Collado" <sergio.collado@gmail.com>,
	"Joel Granados" <j.granados@samsung.com>,
	"Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>,
	"Daniel Gomez" <da.gomez@samsung.com>,
	"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: [RFC PATCH 4/5] rust: block: add rnull, Rust null_blk implementation
Date: Tue, 02 Apr 2024 22:35:32 +0000	[thread overview]
Message-ID: <1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me> (raw)
In-Reply-To: <87msqc3p0e.fsf@metaspace.dk>

On 02.04.24 14:52, Andreas Hindborg wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
> 
>> On 3/13/24 12:05, Andreas Hindborg wrote:
>>> +module! {
>>> +    type: NullBlkModule,
>>> +    name: "rnull_mod",
>>> +    author: "Andreas Hindborg",
>>> +    license: "GPL v2",
>>> +    params: {
>>> +        param_memory_backed: bool {
>>> +            default: true,
>>> +            permissions: 0,
>>> +            description: "Use memory backing",
>>> +        },
>>> +        // Problems with pin_init when `irq_mode`
>>
>> Can you elaborate?
> 
> I think we discussed this before, but I do not recall what you decided
> was the issue.

Ah I vaguely remember.

> It is probably easier if you can apply the patches and try to build with
> this on top:
> 
> diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs
> index 04bdb6668558..bd089c5e6e89 100644
> --- a/drivers/block/rnull.rs
> +++ b/drivers/block/rnull.rs
> @@ -48,7 +48,7 @@
>              description: "Use memory backing",
>          },
>          // Problems with pin_init when `irq_mode`
> -        param_irq_mode: u8 {
> +        irq_mode: u8 {
>              default: 0,
>              permissions: 0,
>              description: "IRQ Mode (0: None, 1: Soft, 2: Timer)",
> @@ -101,7 +101,7 @@ fn add_disk(tagset: Arc<TagSet<NullBlkDevice>>) -> Result<GenDisk<NullBlkDevice>
>          return Err(kernel::error::code::EINVAL);
>      }
> 
> -    let irq_mode = (*param_irq_mode.read()).try_into()?;
> +    let irq_mode = (*irq_mode.read()).try_into()?;
> 
>      let queue_data = Box::pin_init(pin_init!(
>          QueueData {
> 
> ---
> 
> There is some kind of name clash issue when using `pin_init!` in the expression on
> line 106:
> 
>     let queue_data = Box::pin_init(pin_init!(
>         QueueData {
>             tree <- TreeContainer::new(),
>             completion_time_nsec: *param_completion_time_nsec.read(),
>             irq_mode,
>             memory_backed: *param_memory_backed.read(),
>             block_size,
>         }
>     ))?;
> 
> I cannot immediately decipher the error message:
> 
>   RUSTC [M] drivers/block/rnull.o
> error[E0277]: the trait bound `__rnull_mod_irq_mode: From<u8>` is not satisfied
>    --> /home/aeh/src/linux-rust/linux/drivers/block/rnull.rs:104:39
>     |
> 104 |     let irq_mode = (*irq_mode.read()).try_into()?;
>     |                                       ^^^^^^^^ the trait `From<u8>` is not implemented for `__rnull_mod_irq_mode`
>     |
>     = note: required for `u8` to implement `Into<__rnull_mod_irq_mode>`
>     = note: required for `__rnull_mod_irq_mode` to implement `TryFrom<u8>`
>     = note: required for `u8` to implement `TryInto<__rnull_mod_irq_mode>`
> 
> error[E0308]: mismatched types
>     --> /home/aeh/src/linux-rust/linux/drivers/block/rnull.rs:106:36
>      |
> 106  |       let queue_data = Box::pin_init(pin_init!(
>      |  ____________________________________^
> 107  | |         QueueData {
> 108  | |             tree <- TreeContainer::new(),
> 109  | |             completion_time_nsec: *param_completion_time_nsec.read(),
> ...    |
> 113  | |         }
> 114  | |     ))?;
>      | |     ^
>      | |     |
>      | |_____expected `IRQMode`, found `__rnull_mod_irq_mode`
>      |       arguments to this function are incorrect
>      |
> note: function defined here
>     --> /home/aeh/.rustup/toolchains/1.74.1-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1365:21
>      |
> 1365 | pub const unsafe fn write<T>(dst: *mut T, src: T) {
>      |                     ^^^^^
>      = note: this error originates in the macro `$crate::__init_internal` which comes from the expansion of the macro `pin_init` (in Nightly builds, run with -Z macro-backtrace for more info)
> 
> error[E0308]: mismatched types
>    --> /home/aeh/src/linux-rust/linux/drivers/block/rnull.rs:106:36
>     |
> 39  | / module! {
> 40  | |     type: NullBlkModule,
> 41  | |     name: "rnull_mod",
> 42  | |     author: "Andreas Hindborg",
> ...   |
> 71  | |     },
> 72  | | }
>     | |_- constant defined here
> ...
> 106 |       let queue_data = Box::pin_init(pin_init!(
>     |  ____________________________________^
>     | |____________________________________|
>     | |____________________________________|
>     | |____________________________________|
>     | |
> 107 | |         QueueData {
> 108 | |             tree <- TreeContainer::new(),
> 109 | |             completion_time_nsec: *param_completion_time_nsec.read(),
> ...   |
> 113 | |         }
> 114 | |     ))?;
>     | |     ^
>     | |_____|
>     | |_____expected `DropGuard<IRQMode>`, found `__rnull_mod_irq_mode`
>     | |_____this expression has type `DropGuard<IRQMode>`
>     | |_____`irq_mode` is interpreted as a constant, not a new binding
>     |       help: introduce a new binding instead: `other_irq_mode`
>     |
>     = note: expected struct `DropGuard<IRQMode>`
>                found struct `__rnull_mod_irq_mode`
>     = note: this error originates in the macro `$crate::__init_internal` which comes from the expansion of the macro `pin_init` (in Nightly builds, run with -Z macro-backtrace for more info)
> 
> error: aborting due to 3 previous errors
> 
> Some errors have detailed explanations: E0277, E0308.
> For more information about an error, try `rustc --explain E0277`.
> make[5]: *** [/home/aeh/src/linux-rust/linux/scripts/Makefile.build:293: drivers/block/rnull.o] Error 1

So I did some digging and there are multiple things at play. I am going
to explain the second error first, since that one might be a problem
with `pin_init`:
- the `params` extension of the `module!` macro creates constants with
   snake case names.
- your `QueueData` struct has the same name as a field.
- `pin_init!` generates `let $field_name = ...` statements for each
   field you initialize

Now when you define a constant in Rust, you are able to pattern-match
with that constant, eg:

     const FOO: u8 = 0;

     fn main() {
         match 10 {
             FOO => println!("foo"),
             _ => {}
         }
     }

So when you do `let FOO = x;`, then it interprets `FOO` as the constant.
This is still true if the constant has a snake case name.
Since the expression in the `pin_init!` macro has type
`DropGuard<$field_type>`, we get the error "expected
`DropGuard<IRQMode>`, found `__rnull_mod_irq_mode`".

Now to the first error, this is a problem with the parameter handling of
`module`. By the same argument above, your let binding in line 104:

     let irq_mode = (*irq_mode.read()).try_into()?;

Tries to pattern-match the `irq_mode` constant with the right
expression. Since you use the `try_into` function, it tries to search
for a `TryInto` implementation for the type of `irq_mode` which is
generated by the `module!` macro. The type is named
__rnull_mod_irq_mode.

Now what to do about this. For the second error (the one related to
`pin_init`), I could create a patch that fixes it by adding the suffix
`_guard` to those let bindings, preventing the issue. Not sure if we
need that though, since it will not get rid of the first issue.

For the first issue, I think there is no other way than to use a
different name for either the field or the constant. Since constants are
usually named using screaming snake case, I think it should be renamed.
I believe your reason for using a snake case name is that these names
are used directly as the names for the parameters when loading the
module and there the convention is to use snake case, right?
In that case I think we could expect people to write the screaming snake
case name in rust and have it automatically be lower-cased by the
`module!` macro when it creates the names that the parameters are shown
with.

Hope this helps!

-- 
Cheers,
Benno