Re: [PATCH V3 10/27] ublk: handle UBLK_U_IO_PREP_IO_CMDS

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

On Tue, Nov 18, 2025 at 06:49:57PM -0800, Caleb Sander Mateos wrote:
> On Wed, Nov 12, 2025 at 1:39 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > This commit implements the handling of the UBLK_U_IO_PREP_IO_CMDS command,
> > which allows userspace to prepare a batch of I/O requests.
> >
> > The core of this change is the `ublk_walk_cmd_buf` function, which iterates
> > over the elements in the uring_cmd fixed buffer. For each element, it parses
> > the I/O details, finds the corresponding `ublk_io` structure, and prepares it
> > for future dispatch.
> >
> > Add per-io lock for protecting concurrent delivery and committing.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c      | 205 +++++++++++++++++++++++++++++++++-
> >  include/uapi/linux/ublk_cmd.h |   5 +
> >  2 files changed, 209 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 5f9d7ec9daa4..84d838df18cb 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -117,6 +117,7 @@ struct ublk_batch_io_data {
> >         struct ublk_device *ub;
> >         struct io_uring_cmd *cmd;
> >         struct ublk_batch_io header;
> > +       unsigned int issue_flags;
> >  };
> >
> >  /*
> > @@ -201,6 +202,7 @@ struct ublk_io {
> >         unsigned task_registered_buffers;
> >
> >         void *buf_ctx_handle;
> > +       spinlock_t lock;
> >  } ____cacheline_aligned_in_smp;
> >
> >  struct ublk_queue {
> > @@ -270,6 +272,16 @@ static inline bool ublk_dev_support_batch_io(const struct ublk_device *ub)
> >         return false;
> >  }
> >
> > +static inline void ublk_io_lock(struct ublk_io *io)
> > +{
> > +       spin_lock(&io->lock);
> > +}
> > +
> > +static inline void ublk_io_unlock(struct ublk_io *io)
> > +{
> > +       spin_unlock(&io->lock);
> > +}
> > +
> >  static inline struct ublksrv_io_desc *
> >  ublk_get_iod(const struct ublk_queue *ubq, unsigned tag)
> >  {
> > @@ -2531,6 +2543,183 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
> >         return ublk_ch_uring_cmd_local(cmd, issue_flags);
> >  }
> >
> > +static inline __u64 ublk_batch_buf_addr(const struct ublk_batch_io *uc,
> > +                                       const struct ublk_elem_header *elem)
> > +{
> > +       const void *buf = elem;
> > +
> > +       if (uc->flags & UBLK_BATCH_F_HAS_BUF_ADDR)
> > +               return *(__u64 *)(buf + sizeof(*elem));
> > +       return 0;
> > +}
> > +
> > +static struct ublk_auto_buf_reg
> > +ublk_batch_auto_buf_reg(const struct ublk_batch_io *uc,
> > +                       const struct ublk_elem_header *elem)
> > +{
> > +       struct ublk_auto_buf_reg reg = {
> > +               .index = elem->buf_index,
> > +               .flags = (uc->flags & UBLK_BATCH_F_AUTO_BUF_REG_FALLBACK) ?
> > +                       UBLK_AUTO_BUF_REG_FALLBACK : 0,
> > +       };
> > +
> > +       return reg;
> > +}
> > +
> > +/*
> > + * 48 can hold any type of buffer element(8, 16 and 24 bytes) because
> > + * it is the least common multiple(LCM) of 8, 16 and 24
> > + */
> > +#define UBLK_CMD_BATCH_TMP_BUF_SZ  (48 * 10)
> > +struct ublk_batch_io_iter {
> > +       /* copy to this buffer from iterator first */
> > +       unsigned char buf[UBLK_CMD_BATCH_TMP_BUF_SZ];
> > +       struct iov_iter iter;
> > +       unsigned done, total;
> > +       unsigned char elem_bytes;
> > +};
> > +
> > +static inline int
> > +__ublk_walk_cmd_buf(struct ublk_queue *ubq,
> > +                   struct ublk_batch_io_iter *iter,
> > +                   const struct ublk_batch_io_data *data,
> > +                   unsigned bytes,
> > +                   int (*cb)(struct ublk_queue *q,
> > +                           const struct ublk_batch_io_data *data,
> > +                           const struct ublk_elem_header *elem))
> > +{
> > +       int i, ret = 0;
> 
> unsigned i to avoid comparisons between signed and unsigned values?

OK.

> 
> > +
> > +       for (i = 0; i < bytes; i += iter->elem_bytes) {
> > +               const struct ublk_elem_header *elem =
> > +                       (const struct ublk_elem_header *)&iter->buf[i];
> > +
> > +               if (unlikely(elem->tag >= data->ub->dev_info.queue_depth)) {
> > +                       ret = -EINVAL;
> > +                       break;
> > +               }
> > +
> > +               ret = cb(ubq, data, elem);
> > +               if (unlikely(ret))
> > +                       break;
> > +       }
> > +
> > +       /* revert unhandled bytes in case of failure */
> > +       if (ret)
> > +               iov_iter_revert(&iter->iter, bytes - i);
> > +
> > +       iter->done += i;
> > +       return ret;
> > +}
> > +
> > +static int ublk_walk_cmd_buf(struct ublk_batch_io_iter *iter,
> > +                            const struct ublk_batch_io_data *data,
> > +                            int (*cb)(struct ublk_queue *q,
> > +                                    const struct ublk_batch_io_data *data,
> > +                                    const struct ublk_elem_header *elem))
> > +{
> > +       struct ublk_queue *ubq = ublk_get_queue(data->ub, data->header.q_id);
> > +       int ret = 0;
> > +
> > +       while (iter->done < iter->total) {
> > +               unsigned int len = min(sizeof(iter->buf), iter->total - iter->done);
> > +
> > +               ret = copy_from_iter(iter->buf, len, &iter->iter);
> 
> Would make more sense to store this as an unsigned value.

OK.

BTW, it has been changed to plain copy_from_user() in my local version by
dropping fixed buffer for commit/prep ios command.

There is also one big bug fix in patch 'ublk: add batch I/O dispatch infrastructure',

Do you prefer to continuing to review on V3 or the coming V4?

If you prefer to V4, I can prepare and send it soon.

> 
> > +               if (ret != len) {
> > +                       pr_warn("ublk%d: read batch cmd buffer failed %u/%u\n",
> > +                                       data->ub->dev_info.dev_id, ret, len);
> > +                       ret = -EINVAL;
> > +                       break;
> 
> Just return -EINVAL?
> 
> > +               }
> > +
> > +               ret = __ublk_walk_cmd_buf(ubq, iter, data, len, cb);
> > +               if (ret)
> > +                       break;
> 
> Just return ret?
> 
> > +       }
> > +       return ret;
> > +}
> > +
> > +static int ublk_batch_unprep_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];
> > +
> > +       data->ub->nr_io_ready--;
> > +       ublk_io_lock(io);
> > +       io->flags = 0;
> > +       ublk_io_unlock(io);
> > +       return 0;
> > +}
> > +
> > +static void ublk_batch_revert_prep_cmd(struct ublk_batch_io_iter *iter,
> > +                                      const struct ublk_batch_io_data *data)
> > +{
> > +       int ret;
> > +
> > +       if (!iter->done)
> > +               return;
> > +
> > +       iov_iter_revert(&iter->iter, iter->done);
> > +       iter->total = iter->done;
> > +       iter->done = 0;
> > +
> > +       ret = ublk_walk_cmd_buf(iter, data, ublk_batch_unprep_io);
> > +       WARN_ON_ONCE(ret);
> > +}
> > +
> > +static int ublk_batch_prep_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;
> > +       union ublk_io_buf buf = { 0 };
> > +       int ret;
> > +
> > +       if (ublk_dev_support_auto_buf_reg(data->ub))
> > +               buf.auto_reg = ublk_batch_auto_buf_reg(uc, elem);
> > +       else if (ublk_dev_need_map_io(data->ub)) {
> > +               buf.addr = ublk_batch_buf_addr(uc, elem);
> > +
> > +               ret = ublk_check_fetch_buf(data->ub, buf.addr);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       ublk_io_lock(io);
> > +       ret = __ublk_fetch(data->cmd, data->ub, io);
> > +       if (!ret)
> > +               io->buf = buf;
> > +       ublk_io_unlock(io);
> > +
> > +       return ret;
> > +}
> > +
> > +static int ublk_handle_batch_prep_cmd(const struct ublk_batch_io_data *data)
> > +{
> > +       const struct ublk_batch_io *uc = &data->header;
> > +       struct io_uring_cmd *cmd = data->cmd;
> > +       struct ublk_batch_io_iter iter = {
> > +               .total = uc->nr_elem * uc->elem_bytes,
> > +               .elem_bytes = uc->elem_bytes,
> > +       };
> > +       int ret;
> > +
> > +       ret = io_uring_cmd_import_fixed(cmd->sqe->addr, iter.total,
> 
> sqe-> addr should be accessed with READ_ONCE() since it may point to
> user-mapped memory.

OK.

> 
> > +                       WRITE, &iter.iter, cmd, data->issue_flags);
> > +       if (ret)
> > +               return ret;
> > +
> > +       mutex_lock(&data->ub->mutex);
> > +       ret = ublk_walk_cmd_buf(&iter, data, ublk_batch_prep_io);
> > +
> > +       if (ret && iter.done)
> > +               ublk_batch_revert_prep_cmd(&iter, data);
> 
> Mentioned this on V1 as well, but the iter.done check is duplicated in
> ublk_batch_revert_prep_cmd().

OK, will drop the check in ublk_batch_revert_prep_cmd().

Thanks,
Ming