Re: [PATCH] io_uring: save repeated issue_flags

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe