Message-Id: <D6CDE1A5-879F-49B1-9E10-2998D04B678F@collabora.com>
Date: Fri, 1 Aug 2025 10:48:40 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Sidong Yang <sidong.yang@...iosa.ai>
Cc: Caleb Sander Mateos <csander@...estorage.com>,
 Benno Lossin <lossin@...nel.org>,
 Miguel Ojeda <ojeda@...nel.org>,
 Arnd Bergmann <arnd@...db.de>,
 Jens Axboe <axboe@...nel.dk>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 io-uring@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for
 io-uring cmd

Hi Sidong,

> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@...iosa.ai> wrote:
> 
> This patch introduces rust abstraction for io-uring sqe, cmd. IoUringSqe
> abstracts io_uring_sqe and it has cmd_data(). and IoUringCmd is
> abstraction for io_uring_cmd. From this, user can get cmd_op, flags,
> pdu and also sqe.

IMHO you need to expand this substantially.

Instead of a very brief discussion of *what* you're doing, you need to explain
*why* you're doing this and how this patch fits with the overall plan that you
have in mind.

Also, for the sake of reviewers, try to at least describe the role of all the
types you've mentioned.

> 
> Signed-off-by: Sidong Yang <sidong.yang@...iosa.ai>
> ---
> rust/kernel/io_uring.rs | 183 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs      |   1 +
> 2 files changed, 184 insertions(+)
> create mode 100644 rust/kernel/io_uring.rs
> 
> diff --git a/rust/kernel/io_uring.rs b/rust/kernel/io_uring.rs
> new file mode 100644
> index 000000000000..0acdf3878247
> --- /dev/null
> +++ b/rust/kernel/io_uring.rs
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Furiosa AI.
> +

Perhaps this instead [0].


> +//! IoUring command and submission queue entry abstractions.

Maybe expand this just a little bit as well.

> +//!
> +//! C headers: [`include/linux/io_uring/cmd.h`](srctree/include/linux/io_uring/cmd.h) and
> +//! [`include/linux/io_uring/io_uring.h`](srctree/include/linux/io_uring/io_uring.h)
> +
> +use core::{mem::MaybeUninit, pin::Pin, ptr::addr_of_mut};
> +
> +use crate::{fs::File, types::Opaque};
> +
> +/// A Rust abstraction for the Linux kernel's `io_uring_cmd` structure.

Is there a link for io_uring_cmd that you can use here?

> +///
> +/// This structure is a safe, opaque wrapper around the raw C `io_uring_cmd`
> +/// binding from the Linux kernel. It represents a command structure used
> +/// in io_uring operations within the kernel.

Perhaps backticks on “io_uring” ?

> +///
> +/// # Type Safety
> +///
> +/// The `#[repr(transparent)]` attribute ensures that this wrapper has
> +/// the same memory layout as the underlying `io_uring_cmd` structure,
> +/// allowing it to be safely transmuted between the two representations.
> +///
> +/// # Fields
> +///
> +/// * `inner` - An opaque wrapper containing the actual `io_uring_cmd` data.
> +///             The `Opaque` type prevents direct access to the internal
> +///             structure fields, ensuring memory safety and encapsulation.

Place this on top of the field itself please. Also, I don’t think you need
this at all because you don't need to explain the Opaque type, as it's
extensively used in the kernel crate.

> +///
> +/// # Usage

I don’t think you need this.

> +///
> +/// This type is used internally by the io_uring subsystem to manage
> +/// asynchronous I/O commands. It is typically accessed through a pinned
> +/// mutable reference: `Pin<&mut IoUringCmd>`. The pinning ensures that
> +/// the structure remains at a fixed memory location, which is required
> +/// for safe interaction with the kernel's io_uring infrastructure.

I don’t think you need anything other than:

> +/// This type is used internally by the io_uring subsystem to manage
> +/// asynchronous I/O commands.

Specifically, you don’t need to say this:

>  The pinning ensures that
> +/// the structure remains at a fixed memory location,



> +///
> +/// Users typically receive this type as an argument in the `file_operations::uring_cmd()`
> +/// callback function, where they can access and manipulate the io_uring command
> +/// data for custom file operations.
> +///
> +/// This type should not be constructed or manipulated directly by
> +/// kernel module developers.

Well, this is pub, so the reality is that it can be manipulated directly
through whatever public API it offers.

> +#[repr(transparent)]
> +pub struct IoUringCmd {
> +    inner: Opaque<bindings::io_uring_cmd>,
> +}
> +
> +impl IoUringCmd {
> +    /// Returns the cmd_op with associated with the io_uring_cmd.

Backticks

> +    #[inline]
> +    pub fn cmd_op(&self) -> u32 {
> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid

Not sure I understand what you’re trying to say here. Perhaps add an
invariant saying that `self.inner` always points to a valid
`bindings::io_uring_cmd` and mention that here instead.

> +        unsafe { (*self.inner.get()).cmd_op }
> +    }
> +
> +    /// Returns the flags with associated with the io_uring_cmd.
> +    #[inline]
> +    pub fn flags(&self) -> u32 {
> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> +        unsafe { (*self.inner.get()).flags }
> +    }
> +
> +    /// Returns the ref pdu for free use.

I have no idea what “ref pdu” is. You need to describe these acronyms.

> +    #[inline]
> +    pub fn pdu(&mut self) -> &mut MaybeUninit<[u8; 32]> {

Why MaybeUninit? Also, this is a question for others, but I don’t think
that `u8`s can ever be uninitialized as all byte values are valid for `u8`.

> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> +        let inner = unsafe { &mut *self.inner.get() };
> +        let ptr = addr_of_mut!(inner.pdu) as *mut MaybeUninit<[u8; 32]>;

&raw mut

> +
> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> +        unsafe { &mut *ptr }
> +    }
> +
> +    /// Constructs a new `IoUringCmd` from a raw `io_uring_cmd`

[`IoUringCmd`]

By the way, always build the docs and see if they look nice.

> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must guarantee that:
> +    /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_cmd`.
> +    /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.
> +    /// - The memory will not be moved or freed while the returned `Pin<&mut IoUringCmd>` is alive.

This returns a wrapper over a mutable reference. You must mention Rust’s aliasing rules here.

> +    #[inline]
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::io_uring_cmd) -> Pin<&'a mut IoUringCmd> {
> +        // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> +        // duration of 'a. The cast is okay because `IoUringCmd` is `repr(transparent)` and has the
> +        // same memory layout as `bindings::io_uring_cmd`. The returned `Pin` ensures that the object
> +        // cannot be moved, which is required because the kernel may hold pointers to this memory
> +        // location and moving it would invalidate those pointers.

Please break this into multiple paragraphs.

> +        unsafe { Pin::new_unchecked(&mut *ptr.cast()) }
> +    }
> +
> +    /// Returns the file that referenced by uring cmd self.
> +    #[inline]
> +    pub fn file(&self) -> &File {
> +        // SAFETY: The call guarantees that the `self.inner` is not dangling and stays valid
> +        let file = unsafe { (*self.inner.get()).file };
> +        // SAFETY: The call guarantees that `file` points valid file.
> +        unsafe { File::from_raw_file(file) }
> +    }
> +
> +    /// Returns a reference to the uring cmd's SQE.

Please define what `SQE` means. Add links if possible.

> +    #[inline]
> +    pub fn sqe(&self) -> &IoUringSqe {
> +        // SAFETY: The call guarantees that the `self.inner` is not dangling and stays valid
> +        let sqe = unsafe { (*self.inner.get()).sqe };
> +        // SAFETY: The call guarantees that the `sqe` points valid io_uring_sqe.

Backticks

> +        unsafe { IoUringSqe::from_raw(sqe) }
> +    }
> +
> +    /// Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command

Backticks

> +    #[inline]
> +    pub fn done(self: Pin<&mut IoUringCmd>, ret: isize, res2: u64, issue_flags: u32) {

The arguments are cryptic here. Please let us know what they do.

> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> +        unsafe {
> +            bindings::io_uring_cmd_done(self.inner.get(), ret, res2, issue_flags);
> +        }
> +    }
> +}
> +
> +/// A Rust abstraction for the Linux kernel's `io_uring_sqe` structure.
> +///
> +/// This structure is a safe, opaque wrapper around the raw C `io_uring_sqe`
> +/// binding from the Linux kernel. It represents a Submission Queue Entry

Ah, SQE == Submission Queue Entry. Is there a link for this?

> +/// used in io_uring operations within the kernel.
> +///
> +/// # Type Safety
> +///
> +/// The `#[repr(transparent)]` attribute ensures that this wrapper has
> +/// the same memory layout as the underlying `io_uring_sqe` structure,
> +/// allowing it to be safely transmuted between the two representations.
> +///
> +/// # Fields
> +///
> +/// * `inner` - An opaque wrapper containing the actual `io_uring_sqe` data.
> +///             The `Opaque` type prevents direct access to the internal
> +///             structure fields, ensuring memory safety and encapsulation.
> +///
> +/// # Usage
> +///
> +/// This type represents a submission queue entry that describes an I/O
> +/// operation to be executed by the io_uring subsystem. It contains
> +/// information such as the operation type, file descriptor, buffer
> +/// pointers, and other operation-specific data.

This description is very good :)

> +///
> +/// Users can obtain this type from `IoUringCmd::sqe()` method, which
> +/// extracts the submission queue entry associated with a command.

[`IoUringCmd::sqe`]

> +///
> +/// This type should not be constructed or manipulated directly by
> +/// kernel module developers.

Again, this is pub and can be freely manipulated through whatever
public API it offers.

> +#[repr(transparent)]
> +pub struct IoUringSqe {
> +    inner: Opaque<bindings::io_uring_sqe>,
> +}
> +
> +impl<'a> IoUringSqe {

Why is this ‘a here?

> +    /// Returns the cmd_data with associated with the io_uring_sqe.
> +    /// This function returns 16 byte array. We don't support IORING_SETUP_SQE128 for now.
> +    pub fn cmd_data(&'a self) -> &'a [Opaque<u8>] {

This is automatically placed by the compiler. See the lifetime elision rules
[1].

Also why does this return a reference to an array of Opaque<u8>?

You can return a &[u8] here if you can prove that this reference is legal given
Rust's aliasing rules. If you can't, then you have to look at what the DMA
allocator code is doing and use that as an example, i.e.: have a look at the
dma_read and dma_write macros and mark the function that returns the slice as
"unsafe".

> +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> +        let cmd = unsafe { (*self.inner.get()).__bindgen_anon_6.cmd.as_ref() };

What do you mean by “the call” ? Same in all other places where this sentence is used.
> +
> +        // SAFETY: The call guarantees that `cmd` is not dangling and stays valid
> +        unsafe { core::slice::from_raw_parts(cmd.as_ptr() as *const Opaque<u8>, 16) }
> +    }
> +
> +    /// Constructs a new `IoUringSqe` from a raw `io_uring_sqe`
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must guarantee that:
> +    /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_sqe`.
> +    /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.

Must mention Rust’s aliasing rules here.

> +    #[inline]
> +    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
> +        // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> +        // duration of 'a. The cast is okay because `IoUringSqe` is `repr(transparent)` and has the
> +        // same memory layout as `bindings::io_uring_sqe`.
> +        unsafe { &*ptr.cast() }
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c3..fb310e78d51d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -80,6 +80,7 @@
> pub mod fs;
> pub mod init;
> pub mod io;
> +pub mod io_uring;
> pub mod ioctl;
> pub mod jump_label;
> #[cfg(CONFIG_KUNIT)]
> -- 
> 2.43.0
> 
> 

[0] https://spdx.github.io/spdx-spec/v3.0.1/model/Software/Properties/copyrightText/
[1] https://doc.rust-lang.org/reference/lifetime-elision.html

Powered by blists - more mailing lists