Re: [PATCH 3/3] io_uring/uring_cmd: import fixed buffer before going async

[Pavel Begunkov]

On 3/21/25 21:38, Caleb Sander Mateos wrote:
> On Fri, Mar 21, 2025 at 1:34 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
> > On 3/21/25 18:48, Caleb Sander Mateos wrote:
> > > For uring_cmd operations with fixed buffers, the fixed buffer lookup
> > > happens in io_uring_cmd_import_fixed(), called from the ->uring_cmd()
> > > implementation. A ->uring_cmd() implementation could return -EAGAIN on
> > > the initial issue for any reason before io_uring_cmd_import_fixed().
> > > For example, nvme_uring_cmd_io() calls nvme_alloc_user_request() first,
> > > which can return -EAGAIN if all tags in the tag set are in use.
> >
> > That's up to command when it resolves the buffer, you can just
> > move the call to io_import_reg_buf() earlier in nvme cmd code
> > and not working it around at the io_uring side.
> >
> > In general, it's a step back, it just got cleaned up from the
> > mess where node resolution and buffer imports were separate
> > steps and duplicate by every single request type that used it.
>
> Yes, I considered just reordering the steps in nvme_uring_cmd_io().
> But it seems easy for a future change to accidentally introduce
> another point where the issue can go async before it has looked up the
> fixed buffer. And I am imagining there will be more uring_cmd fixed
> buffer users added (e.g. btrfs). This seems like a generic problem
> rather than something specific to NVMe passthru.

That's working around the api for ordering requests, that's the
reason you have an ordering problem.

> My other feeling is that the fixed buffer lookup is an io_uring-layer
> detail, whereas the use of the buffer is more a concern of the
> ->uring_cmd() implementation. If only the opcodes were consistent
> about how a fixed buffer is requested, we could do the lookup in the
> generic io_uring code like fixed files already do.

That's one of things I'd hope was done better, and not only
specifically for registered buffers, but it's late for that.

> But I'm open to implementing a different fix here if Jens would prefer.

It's not a fix, the behaviour is well within the constrained
of io_uring.

--
Pavel Begunkov