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

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

On Thu, Jul 03, 2025 at 04:19:50PM -0400, Caleb Sander Mateos wrote:
> On Wed, Jul 2, 2025 at 12:04 AM 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 | 132 ++++++++++++++++++++++-----------------
> >  1 file changed, 76 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 1780f9ce3a24..ba1b2672e7a8 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   ((unsigned short)-1)
> 
> u16?

Yeah.

> 
> > +
> >  /* 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)
> > @@ -1983,16 +1985,53 @@ 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)
> 
> Not sure the buf_idx check here is necessary. This io->flags &
> UBLK_IO_FLAG_AUTO_BUF_REG path should only be reachable from
> ublk_commit_and_fetch(), not ublk_fetch() or UBLK_IO_NEED_GET_DATA.

Good catch, UBLK_IO_FLAG_AUTO_BUF_REG can only be set from
ublk_commit_and_fetch(), will fix it in next version.


Thanks,
Ming