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@xxxxxxxxxx>
>>>
>>> No need to rebuild the issue_flags on every IO: they're always the same.
>>>
>>> Suggested-by: Jens Axboe <axboe@xxxxxxxxx>
>>> Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
>>> ---
> [...]
>>> 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
