On 10/10/24 04:28, Ming Lei wrote:
On Wed, Oct 09, 2024 at 04:14:33PM +0100, Pavel Begunkov wrote:
On 10/6/24 09:46, Ming Lei wrote:
On Fri, Oct 04, 2024 at 04:44:54PM +0100, Pavel Begunkov wrote:
On 9/12/24 11:49, Ming Lei wrote:
...
so driver can check if device buffer can be provided with this uring_cmd,
but I prefer to the new uring_cmd flag:
- IORING_PROVIDE_GROUP_KBUF can provide device buffer in generic way.
Ok, could be.
- ->prep() can fail fast in case that it isn't one group request
I don't believe that matters, a behaving user should never
see that kind of failure.
1. Extra overhead for files / cmds that don't even care about the
feature.
It is just checking ioucmd->flags in ->prep(), and basically zero cost.
It's not if we add extra code for each every feature, at
which point it becomes a maze of such "ifs".
Yeah, I guess it can't be avoided in current uring_cmd design, which
If can't only if we keep putting all custom / some specific
command features into the common path. And, for example, I
just named how this one could be avoided.
The real question is whether we deem that buffer providing
feature applicable widely enough so that it could be useful
to many potential command implementations and therefore is
worth of partially handling it generically in the common path.
serves for different subsystems now, and more in future.
And the situation is similar with ioctl.
Well, commands look too much as ioctl for my taste, but even
then I naively hope it can avoid regressing to it.
2. As it stands with this patch, the flag is ignored by all other
cmd implementations, which might be quite confusing as an api,
especially so since if we don't set that REQ_F_GROUP_KBUF memeber
requests will silently try to import a buffer the "normal way",
The usage is same with buffer select or fixed buffer, and consumer
has to check the flag.
We fails requests when it's asked to use the feature but
those are not supported, at least non-cmd requests.
And same with IORING_URING_CMD_FIXED which is ignored by other
implementations except for nvme, :-)
Oh, that's bad. If you'd try to implement the flag in the
future it might break the uapi. It might be worth to patch it
up on the ublk side, i.e. reject the flag, + backport, and hope
nobody tried to use them together, hmm?
I can understand the concern, but it exits since uring cmd is born.
i.e. interpret sqe->addr or such as the target buffer.
3. We can't even put some nice semantics on top since it's
still cmd specific and not generic to all other io_uring
requests.
I'd even think that it'd make sense to implement it as a
new cmd opcode, but that's the business of the file implementing
it, i.e. ublk.
*/
#define IORING_URING_CMD_FIXED (1U << 0)
-#define IORING_URING_CMD_MASK IORING_URING_CMD_FIXED
+#define IORING_PROVIDE_GROUP_KBUF (1U << 1)
+#define IORING_URING_CMD_MASK (IORING_URING_CMD_FIXED | IORING_PROVIDE_GROUP_KBUF)
It needs one new file operation, and we shouldn't work toward
this way.
Not a new io_uring request, I rather meant sqe->cmd_op,
like UBLK_U_IO_FETCH_REQ_PROVIDER_BUFFER.
`cmd_op` is supposed to be defined by subsystems, but maybe we can
reserve some for generic uring_cmd. Anyway this shouldn't be one big
deal, we can do that in future if there are more such uses.
That's if the generic handling is desired, which isn't much
different from a flag, otherwise it can be just a new random
file specific cmd opcode as any other.
--
Pavel Begunkov
