Re: [RFC PATCH 1/7] io_uring: add 'struct io_buf_data' for register/unregister bvec buffer

[Caleb Sander Mateos <csander@purestorage.com>]

On Mon, Apr 28, 2025 at 2:44 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Add 'struct io_buf_data' for register/unregister bvec buffer, and
> prepare for supporting to register buffer into one specified io_uring
> context by its FD.
>
> No functional change.

I'm not a big fan of this change. Passing the arguments by reference
instead of in registers makes io_buffer_register_bvec() more expensive
to call. And it's a strange API for io_buffer_unregister_bvec() to
take a full struct io_buf_data but only use the index field.

Best,
Caleb


>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c     | 13 ++++++++++---
>  include/linux/io_uring/cmd.h | 11 ++++++++---
>  io_uring/rsrc.c              | 12 ++++++++----
>  3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 0d82014679f8..ac56482b55f5 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1925,6 +1925,10 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
>  {
>         struct ublk_device *ub = cmd->file->private_data;
>         const struct ublk_io *io = &ubq->ios[tag];
> +       struct io_buf_data data = {
> +               .index = index,
> +               .release = ublk_io_release,
> +       };
>         struct request *req;
>         int ret;
>
> @@ -1938,8 +1942,8 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
>         if (!req)
>                 return -EINVAL;
>
> -       ret = io_buffer_register_bvec(cmd, req, ublk_io_release, index,
> -                                     issue_flags);
> +       data.rq = req;
> +       ret = io_buffer_register_bvec(cmd, &data, issue_flags);
>         if (ret) {
>                 ublk_put_req_ref(ubq, req);
>                 return ret;
> @@ -1953,6 +1957,9 @@ static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
>                                   unsigned int index, unsigned int issue_flags)
>  {
>         const struct ublk_io *io = &ubq->ios[tag];
> +       struct io_buf_data data = {
> +               .index = index,
> +       };
>
>         if (!ublk_support_zero_copy(ubq))
>                 return -EINVAL;
> @@ -1960,7 +1967,7 @@ static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
>         if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
>                 return -EINVAL;
>
> -       return io_buffer_unregister_bvec(cmd, index, issue_flags);
> +       return io_buffer_unregister_bvec(cmd, &data, issue_flags);
>  }
>
>  static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> index 0634a3de1782..78fa336a284b 100644
> --- a/include/linux/io_uring/cmd.h
> +++ b/include/linux/io_uring/cmd.h
> @@ -23,6 +23,12 @@ struct io_uring_cmd_data {
>         void                    *op_data;
>  };
>
> +struct io_buf_data {
> +       unsigned short index;
> +       struct request *rq;
> +       void (*release)(void *);
> +};
> +
>  static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)
>  {
>         return sqe->cmd;
> @@ -140,10 +146,9 @@ static inline struct io_uring_cmd_data *io_uring_cmd_get_async_data(struct io_ur
>         return cmd_to_io_kiocb(cmd)->async_data;
>  }
>
> -int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
> -                           void (*release)(void *), unsigned int index,
> +int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct io_buf_data *data,
>                             unsigned int issue_flags);
> -int io_buffer_unregister_bvec(struct io_uring_cmd *cmd, unsigned int index,
> +int io_buffer_unregister_bvec(struct io_uring_cmd *cmd, struct io_buf_data *data,
>                               unsigned int issue_flags);
>
>  #endif /* _LINUX_IO_URING_CMD_H */
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index b4c5f3ee8855..66d2c11e2f46 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -918,12 +918,14 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg,
>         return ret;
>  }
>
> -int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
> -                           void (*release)(void *), unsigned int index,
> +int io_buffer_register_bvec(struct io_uring_cmd *cmd,
> +                           struct io_buf_data *buf,
>                             unsigned int issue_flags)
>  {
>         struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
>         struct io_rsrc_data *data = &ctx->buf_table;
> +       unsigned int index = buf->index;
> +       struct request *rq = buf->rq;
>         struct req_iterator rq_iter;
>         struct io_mapped_ubuf *imu;
>         struct io_rsrc_node *node;
> @@ -963,7 +965,7 @@ int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
>         imu->folio_shift = PAGE_SHIFT;
>         imu->nr_bvecs = nr_bvecs;
>         refcount_set(&imu->refs, 1);
> -       imu->release = release;
> +       imu->release = buf->release;
>         imu->priv = rq;
>         imu->is_kbuf = true;
>         imu->dir = 1 << rq_data_dir(rq);
> @@ -980,11 +982,13 @@ int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
>  }
>  EXPORT_SYMBOL_GPL(io_buffer_register_bvec);
>
> -int io_buffer_unregister_bvec(struct io_uring_cmd *cmd, unsigned int index,
> +int io_buffer_unregister_bvec(struct io_uring_cmd *cmd,
> +                             struct io_buf_data *buf,
>                               unsigned int issue_flags)
>  {
>         struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
>         struct io_rsrc_data *data = &ctx->buf_table;
> +       unsigned index = buf->index;
>         struct io_rsrc_node *node;
>         int ret = 0;
>
> --
> 2.47.0
>