Re: [PATCH 3/9] ublk: publish io->cmd under io->lock in the commit paths

[Caleb Sander Mateos]

On Mon, Sep 28, 2026 at 9:03 AM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> io->cmd shares its storage with io->req. FETCH, COMMIT_AND_FETCH and
> NEED_GET_DATA switch the io from one to the other in
> ublk_fill_io_cmd() without a lock of their own. That was fine as long
> as everything else looking at io->cmd ran under the uring_lock of the
> same ring, which is the case for the io_uring cancel callback.
>
> ublk_cancel_cmd() is also called from the control path, from
> ublk_cancel_dev() with IO_URING_F_UNLOCKED, and the next patches add
> more such callers which run while the server still commits requests.
> Nothing orders their reads of io->flags and io->cmd against
> ublk_fill_io_cmd() then. A reader can find UBLK_IO_FLAG_ACTIVE set and
> still pick up the request pointer from the union.
>
> The batch commit path calls ublk_fill_io_cmd() under io->lock already.
> Do the same in the other three callers, so that io->lock covers every
> switch of the union to a command. io->lock is per io and only taken by
> the task which commits that io, so the fast path gains an uncontended
> lock in a cacheline it writes anyway.
>
> ublk_batch_prep_io() calls __ublk_fetch() with io->lock held, which
> makes the order io->lock, then ubq->cancel_lock.
>
> The readers move under io->lock in the next patch.
>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
>  drivers/block/ublk_drv.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 50c99b28ef21..17a33539f271 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -3169,6 +3169,9 @@ ublk_fill_io_cmd(struct ublk_io *io, struct io_uring_cmd *cmd)
>  {
>         struct request *req = io->req;
>
> +       /* io->cmd shares its storage with io->req, switch them under io->lock */
> +       lockdep_assert_held(&io->lock);
> +
>         io->cmd = cmd;
>         io->flags |= UBLK_IO_FLAG_ACTIVE;
>         /* now this cmd slot is owned by ublk driver */
> @@ -3338,8 +3341,11 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub,
>          */
>         mutex_lock(&ub->mutex);
>         ret = ublk_validate_io_buf(ub, cmd, &auto_buf);
> -       if (!ret)
> +       if (!ret) {
> +               ublk_io_lock(io);
>                 ret = __ublk_fetch(cmd, ub, io, q_id);
> +               ublk_io_unlock(io);
> +       }
>         if (!ret) {
>                 ublk_apply_io_buf(ub, io, cmd, buf_addr, &auto_buf, NULL);
>                 ublk_mark_io_ready(ub, q_id);
> @@ -3496,7 +3502,9 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
>                 if (ret)
>                         goto out;
>                 io->res = result;
> +               ublk_io_lock(io);
>                 req = ublk_fill_io_cmd(io, cmd);
> +               ublk_io_unlock(io);

Taking a spinlock for every ublk I/O completion will be very
expensive. Is it not possible all paths calling ublk_cancel_dev() to
wait for all tags to go idle?

Best,
Caleb

>                 ublk_apply_io_buf(ub, io, cmd, addr, &auto_buf, &buf_idx);
>                 if (buf_idx != UBLK_INVALID_BUF_IDX)
>                         io_buffer_unregister(cmd, buf_idx, issue_flags);
> @@ -3514,7 +3522,9 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
>                  * uring_cmd active first and prepare for handling new requeued
>                  * request
>                  */
> +               ublk_io_lock(io);
>                 req = ublk_fill_io_cmd(io, cmd);
> +               ublk_io_unlock(io);
>                 io->buf.addr = addr;
>                 if (likely(ublk_get_data(ubq, io, req))) {
>                         __ublk_prep_compl_io_cmd(io, req);
>
> --
> 2.55.0
>