Re: [PATCH] io_uring: save repeated issue_flags

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

On 12/6/23 01:41, Jens Axboe wrote:
> On 12/5/23 6:26 PM, Pavel Begunkov wrote:
>> On 12/5/23 22:00, Jens Axboe wrote:
>>> On 12/5/23 2:55 PM, Keith Busch wrote:
>>>> From: Keith Busch <kbusch@kernel.org>
>>>>
>>>> No need to rebuild the issue_flags on every IO: they're always the same.
>>>>
>>>> Suggested-by: Jens Axboe <axboe@kernel.dk>
>>>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>>>> ---
>> [...]
>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>> index 8a38b9f75d841..dbc0bfbfd0f05 100644
>>>> --- a/io_uring/uring_cmd.c
>>>> +++ b/io_uring/uring_cmd.c
>>>> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>        if (ret)
>>>>            return ret;
>>>>    -    if (ctx->flags & IORING_SETUP_SQE128)
>>>> -        issue_flags |= IO_URING_F_SQE128;
>>>> -    if (ctx->flags & IORING_SETUP_CQE32)
>>>> -        issue_flags |= IO_URING_F_CQE32;
>>>> -    if (ctx->compat)
>>>> -        issue_flags |= IO_URING_F_COMPAT;
>>>>        if (ctx->flags & IORING_SETUP_IOPOLL) {
>>>>            if (!file->f_op->uring_cmd_iopoll)
>>>>                return -EOPNOTSUPP;
>>>> -        issue_flags |= IO_URING_F_IOPOLL;
>>>>            req->iopoll_completed = 0;
>>>>        }
>>>>    +    issue_flags |= ctx->issue_flags;
>>>>        ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>>>        if (ret == -EAGAIN) {
>>>>            if (!req_has_async_data(req)) {
>>>
>>> I obviously like this idea, but it should be accompanied by getting rid
>>> of ->compat and ->syscall_iopoll in the ctx as well?
>>
>> This all piggy backing cmd specific bits onto core io_uring issue_flags
>> business is pretty nasty. Apart from that, it mixes constant io_uring
>> flags and "execution context" issue_flags. And we're dancing around it
>> not really addressing the problem.
>>
>> IMHO, cmds should be testing for IORING_SETUP flags directly via
>> helpers, not renaming them and abusing core io_uring flags. E.g. I had
>> a patch like below but didn't care enough to send:
>>
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 909377068a87..1a82a0633f16 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
>>   
>>       ublk_ctrl_cmd_dump(cmd);
>>   
>> -    if (!(issue_flags & IO_URING_F_SQE128))
>> +    if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
>>           goto out;
>>   
>>       ret = ublk_check_cmd_op(cmd_op);
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index d69b4038aa3e..8a18a705ff31 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
>>       return cmd_to_io_kiocb(cmd)->task;
>>   }
>>   
>> +static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
>> +{
>> +    struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
>> +
>> +    return ctx->flags;
>> +}
>> +
>>   #endif /* _LINUX_IO_URING_CMD_H */
> 
> Yeah this is fine too, I just don't like our current scheme of having to
> mirror state in issue flags. Consolidating one way or another would be
> really nice.

Just hiding ->compat into the cache won't help it, most of the cmd
flags mirror IORING_SETUP_*, so unless it checks IORING_SETUP_* directly
there will be this duplication.

-- 
Pavel Begunkov