Re: [PATCH V2 05/16] ublk: move auto buffer register handling into one dedicated helper

[Ming Lei <ming.lei@redhat.com>]

On Fri, Jul 11, 2025 at 09:47:56AM -0400, Caleb Sander Mateos wrote:
> On Mon, Jul 7, 2025 at 9:18 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Move check & clearing UBLK_IO_FLAG_AUTO_BUF_REG to
> > ublk_handle_auto_buf_reg(), also return buffer index from this helper.
> >
> > Also move ublk_set_auto_buf_reg() to this single helper too.
> >
> > Add ublk_config_io_buf() for setting up ublk io buffer, covers both
> > ublk buffer copy or auto buffer register.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 131 ++++++++++++++++++++++-----------------
> >  1 file changed, 75 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 41248b0d1182..dab02a8be41a 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -48,6 +48,8 @@
> >
> >  #define UBLK_MINORS            (1U << MINORBITS)
> >
> > +#define UBLK_INVALID_BUF_IDX   ((u16)-1)
> > +
> >  /* private ioctl command mirror */
> >  #define UBLK_CMD_DEL_DEV_ASYNC _IOC_NR(UBLK_U_CMD_DEL_DEV_ASYNC)
> >  #define UBLK_CMD_UPDATE_SIZE   _IOC_NR(UBLK_U_CMD_UPDATE_SIZE)
> > @@ -2002,16 +2004,52 @@ static inline int ublk_check_cmd_op(u32 cmd_op)
> >         return 0;
> >  }
> >
> > +static inline int ublk_set_auto_buf_reg(struct io_uring_cmd *cmd)
> > +{
> > +       struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > +
> > +       pdu->buf = ublk_sqe_addr_to_auto_buf_reg(READ_ONCE(cmd->sqe->addr));
> > +
> > +       if (pdu->buf.reserved0 || pdu->buf.reserved1)
> > +               return -EINVAL;
> > +
> > +       if (pdu->buf.flags & ~UBLK_AUTO_BUF_REG_F_MASK)
> > +               return -EINVAL;
> > +       return 0;
> > +}
> > +
> > +static int ublk_handle_auto_buf_reg(struct ublk_io *io,
> > +                                   struct io_uring_cmd *cmd,
> > +                                   u16 *buf_idx)
> > +{
> > +       if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG) {
> > +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
> > +
> > +               /*
> > +                * `UBLK_F_AUTO_BUF_REG` only works iff `UBLK_IO_FETCH_REQ`
> > +                * and `UBLK_IO_COMMIT_AND_FETCH_REQ` are issued from same
> > +                * `io_ring_ctx`.
> > +                *
> > +                * If this uring_cmd's io_ring_ctx isn't same with the
> > +                * one for registering the buffer, it is ublk server's
> > +                * responsibility for unregistering the buffer, otherwise
> > +                * this ublk request gets stuck.
> > +                */
> > +               if (io->buf_ctx_handle == io_uring_cmd_ctx_handle(cmd))
> > +                       *buf_idx = io->buf_index;
> > +       }
> > +
> > +       return ublk_set_auto_buf_reg(cmd);
> > +}
> > +
> >  /* Once we return, `io->req` can't be used any more */
> >  static inline struct request *
> > -ublk_fill_io_cmd(struct ublk_io *io, struct io_uring_cmd *cmd,
> > -                unsigned long buf_addr, int result)
> > +ublk_fill_io_cmd(struct ublk_io *io, struct io_uring_cmd *cmd, int result)
> >  {
> >         struct request *req = io->req;
> >
> >         io->cmd = cmd;
> >         io->flags |= UBLK_IO_FLAG_ACTIVE;
> > -       io->addr = buf_addr;
> >         io->res = result;
> >
> >         /* now this cmd slot is owned by ublk driver */
> > @@ -2020,6 +2058,22 @@ ublk_fill_io_cmd(struct ublk_io *io, struct io_uring_cmd *cmd,
> >         return req;
> >  }
> >
> > +static inline int
> > +ublk_config_io_buf(const struct ublk_queue *ubq, struct ublk_io *io,
> > +                  struct io_uring_cmd *cmd, unsigned long buf_addr,
> > +                  u16 *buf_idx)
> > +{
> > +       if (ublk_support_auto_buf_reg(ubq)) {
> > +               int ret = ublk_handle_auto_buf_reg(io, cmd, buf_idx);
> > +
> > +               if (ret)
> > +                       return ret;
> 
> I mentioned this before, but just return ublk_handle_auto_buf_reg(io,
> cmd, buf_idx) to avoid the intermediate variable?

Will do it in next version.


Thanks,
Ming