On Mon, Dec 1, 2025 at 2:26 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Sun, Nov 30, 2025 at 08:39:49AM -0800, Caleb Sander Mateos wrote:
> > On Thu, Nov 20, 2025 at 5:59 PM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > Handle UBLK_U_IO_COMMIT_IO_CMDS by walking the uring_cmd fixed buffer:
> > >
> > > - read each element into one temp buffer in batch style
> > >
> > > - parse and apply each element for committing io result
> > >
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c      | 117 ++++++++++++++++++++++++++++++++--
> > >  include/uapi/linux/ublk_cmd.h |   8 +++
> > >  2 files changed, 121 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 66c77daae955..ea992366af5b 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -2098,9 +2098,9 @@ static inline int ublk_set_auto_buf_reg(struct ublk_io *io, struct io_uring_cmd
> > >         return 0;
> > >  }
> > >
> > > -static int ublk_handle_auto_buf_reg(struct ublk_io *io,
> > > -                                   struct io_uring_cmd *cmd,
> > > -                                   u16 *buf_idx)
> > > +static void __ublk_handle_auto_buf_reg(struct ublk_io *io,
> > > +                                      struct io_uring_cmd *cmd,
> > > +                                      u16 *buf_idx)
> >
> > The name could be a bit more descriptive. How about "ublk_clear_auto_buf_reg()"?
>
> Looks fine.
>
> >
> > >  {
> > >         if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG) {
> > >                 io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
> > > @@ -2118,7 +2118,13 @@ static int ublk_handle_auto_buf_reg(struct ublk_io *io,
> > >                 if (io->buf_ctx_handle == io_uring_cmd_ctx_handle(cmd))
> > >                         *buf_idx = io->buf.auto_reg.index;
> > >         }
> > > +}
> > >
> > > +static int ublk_handle_auto_buf_reg(struct ublk_io *io,
> > > +                                   struct io_uring_cmd *cmd,
> > > +                                   u16 *buf_idx)
> > > +{
> > > +       __ublk_handle_auto_buf_reg(io, cmd, buf_idx);
> > >         return ublk_set_auto_buf_reg(io, cmd);
> > >  }
> > >
> > > @@ -2553,6 +2559,17 @@ static inline __u64 ublk_batch_buf_addr(const struct ublk_batch_io *uc,
> > >         return 0;
> > >  }
> > >
> > > +static inline __u64 ublk_batch_zone_lba(const struct ublk_batch_io *uc,
> > > +                                       const struct ublk_elem_header *elem)
> > > +{
> > > +       const void *buf = (const void *)elem;
> >
> > Unnecessary cast
>
> OK
>
> >
> > > +
> > > +       if (uc->flags & UBLK_BATCH_F_HAS_ZONE_LBA)
> > > +               return *(__u64 *)(buf + sizeof(*elem) +
> > > +                               8 * !!(uc->flags & UBLK_BATCH_F_HAS_BUF_ADDR));
> >
> > Cast to a const pointer?
>
> OK, but I feel it isn't necessary.

I don't feel strongly, just seems like the purpose of the cast is
clearer when it doesn't change the const-ness of the pointer.

>
> >
> >
> > > +       return -1;
> > > +}
> > > +
> > >  static struct ublk_auto_buf_reg
> > >  ublk_batch_auto_buf_reg(const struct ublk_batch_io *uc,
> > >                         const struct ublk_elem_header *elem)
> > > @@ -2708,6 +2725,98 @@ static int ublk_handle_batch_prep_cmd(const struct ublk_batch_io_data *data)
> > >         return ret;
> > >  }
> > >
> > > +static int ublk_batch_commit_io_check(const struct ublk_queue *ubq,
> > > +                                     struct ublk_io *io,
> > > +                                     union ublk_io_buf *buf)
> > > +{
> > > +       struct request *req = io->req;
> > > +
> > > +       if (!req)
> > > +               return -EINVAL;
> >
> > This check seems redundant with the UBLK_IO_FLAG_OWNED_BY_SRV check?
>
> I'd keep the check, which has document benefit, or warn_on()?

WARN_ON() seems okay, though not sure it's necessary. Though there are
several existing places that assume io->req is set when
UBLK_IO_FLAG_OWNED_BY_SRV is set. (And that's the documented
precondition for using io->req: "valid if UBLK_IO_FLAG_OWNED_BY_SRV is
set".)

Best,
Caleb

>
> >
> > > +
> > > +       if (io->flags & UBLK_IO_FLAG_ACTIVE)
> > > +               return -EBUSY;
> >
> > Aren't UBLK_IO_FLAG_ACTIVE and UBLK_IO_FLAG_OWNED_BY_SRV mutually
> > exclusive? Then this check is also redundant with the
> > UBLK_IO_FLAG_OWNED_BY_SRV check.
>
> OK.
>
> >
> > > +
> > > +       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> > > +               return -EINVAL;
> > > +
> > > +       if (ublk_need_map_io(ubq)) {
> > > +               /*
> > > +                * COMMIT_AND_FETCH_REQ has to provide IO buffer if
> > > +                * NEED GET DATA is not enabled or it is Read IO.
> > > +                */
> > > +               if (!buf->addr && (!ublk_need_get_data(ubq) ||
> > > +                                       req_op(req) == REQ_OP_READ))
> > > +                       return -EINVAL;
> > > +       }
> > > +       return 0;
> > > +}
> > > +
> > > +static int ublk_batch_commit_io(struct ublk_queue *ubq,
> > > +                               const struct ublk_batch_io_data *data,
> > > +                               const struct ublk_elem_header *elem)
> > > +{
> > > +       struct ublk_io *io = &ubq->ios[elem->tag];
> > > +       const struct ublk_batch_io *uc = &data->header;
> > > +       u16 buf_idx = UBLK_INVALID_BUF_IDX;
> > > +       union ublk_io_buf buf = { 0 };
> > > +       struct request *req = NULL;
> > > +       bool auto_reg = false;
> > > +       bool compl = false;
> > > +       int ret;
> > > +
> > > +       if (ublk_dev_support_auto_buf_reg(data->ub)) {
> > > +               buf.auto_reg = ublk_batch_auto_buf_reg(uc, elem);
> > > +               auto_reg = true;
> > > +       } else if (ublk_dev_need_map_io(data->ub))
> > > +               buf.addr = ublk_batch_buf_addr(uc, elem);
> > > +
> > > +       ublk_io_lock(io);
> > > +       ret = ublk_batch_commit_io_check(ubq, io, &buf);
> > > +       if (!ret) {
> > > +               io->res = elem->result;
> > > +               io->buf = buf;
> > > +               req = ublk_fill_io_cmd(io, data->cmd);
> > > +
> > > +               if (auto_reg)
> > > +                       __ublk_handle_auto_buf_reg(io, data->cmd, &buf_idx);
> > > +               compl = ublk_need_complete_req(data->ub, io);
> > > +       }
> > > +       ublk_io_unlock(io);
> > > +
> > > +       if (unlikely(ret)) {
> > > +               pr_warn("%s: dev %u queue %u io %u: commit failure %d\n",
> > > +                       __func__, data->ub->dev_info.dev_id, ubq->q_id,
> > > +                       elem->tag, ret);
> >
> > This warning can be triggered by userspace. It should probably be
> > rate-limited or changed to pr_devel().
>
> Looks fine.
>
>
>
> Thanks,
> Ming
>