Re: [PATCH 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

["Benno Lossin" <lossin@kernel.org>]

On Mon Jul 21, 2025 at 5:04 PM CEST, Caleb Sander Mateos wrote:
> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
>> > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> > > +    }
>> > > +
>> > > +    // Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command
>> > > +    #[inline]
>> > > +    pub fn done(self, ret: isize, res2: u64, issue_flags: u32) {
>> >
>> > I don't think it's safe to move io_uring_cmd. io_uring_cmd_done(), for
>> > example, calls cmd_to_io_kiocb() to turn struct io_uring_cmd *ioucmd
>> > into struct io_kiocb *req via a pointer cast. And struct io_kiocb's
>> > definitely need to be pinned in memory. For example,
>> > io_req_normal_work_add() inserts the struct io_kiocb into a linked
>> > list. Probably some sort of pinning is necessary for IoUringCmd.
>>
>> Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
>> in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
>> &mut IoUringCmd for arg.
>
> I'm still a little worried about exposing &mut IoUringCmd without
> pinning. It would allow swapping the fields of two IoUringCmd's (and
> therefore struct io_uring_cmd's), for example. If a struct
> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> swapping it with another struct io_uring_cmd would result in
> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> Maybe it would be okay if IoUringCmd had an invariant that the struct
> io_uring_cmd is not on the task work list. But I would feel safer with
> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> the kernel, though, so I would welcome other opinions.

Pinning in the kernel isn't much different from userspace. From your
description of what normally happens with `struct io_uring_cmd`, it
definitely must be pinned.

From a quick glance at the patch series, I don't see a way to create a
`IoUringCmd` by-value, which also means that the `done` function won't
be callable (also the `fn pdu(&mut self)` function won't be callable,
since you only ever create a `&IoUringCmd`). I'm not sure if I'm missing
something, do you plan on further patches in the future?

How (aside from `from_raw`) are `IoUringCmd` values going to be created
or exposed to the user?

---
Cheers,
Benno