Re: [PATCH V6 7/8] io_uring/uring_cmd: support provide group kernel buffer

[Pavel Begunkov <asml.silence@gmail.com>]

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