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

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Sep 28, 2026 at 10:53:38AM -0700, Caleb Sander Mateos wrote:
> On Mon, Sep 28, 2026 at 9:03 AM Josef Bacik <josef@toxicpanda.com> wrote:
> > +               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?

I measured it on a c6id.metal (Xeon 8375C) under KVM: an 8 vCPU guest,
kublk null target with 4 queues, fio 4k randread with 4 jobs at iodepth
32, ten interleaved rounds of three kernels: for-next, this series, and
this series with io->lock taken back out of the commit path. That last
one also drops the second lock/unlock pair patch 5 adds after
ublk_prep_cancel(), so it isolates both. The guests weren't pinned and
landed at two throughput levels about 15% apart, so I compared within a
level.

Series against the no-lock kernel, IOPS / CPU time per I/O:

  plain      -0.04% / +0.7%  (high level)   -0.7% / +0.7%  (low level)
  zero copy  -0.6%  / +1.1%                 +1.5% / -1.2%

Batch mode, which runs the same per-I/O code on both kernels, differs by
-0.03% / +1.2% and +0.4% / +0.5%, so the lock is inside the noise of
this setup, which is under 1% of about 3.5us per I/O. Against for-next
the series is +0.4% IOPS / +1.0% CPU per I/O in plain mode at the high
level. I'm rerunning with pinned guests to tighten that and will follow
up if it moves.

It's one lock per io that only the task committing that io takes, so
it's uncontended, which fits those numbers.

Waiting for the tags to go idle doesn't close the race this is for,
though. The control path claims io->cmd while the server can still
commit on the same io, and what matters is ordering the claim against
the commit switching the io from the request to the new command. Idle
doesn't give you that: a tag can be idle when you look and be re-armed
by a COMMIT_AND_FETCH right after. That's the QUIESCE_DEV hang on
for-next today, its cancel pass skips a tag whose request is with the
server, the server commits and re-arms it, and nothing ever completes
that command.

Also, ublk_wait_for_idle_io() can't actually wait as it is.
blk_mq_tagset_busy_iter() only visits started requests and
ublk_count_busy_req() only counts requests that aren't started, so the
count is always 0. I'll send a fix for that separately with the
QUIESCE_DEV work.

If the numbers show a real cost I'd rather find a way to keep the lock
off the fast path than lose the ordering, so I'm open to ideas.

Thanks,
Josef