On Thu, Jul 03, 2025 at 11:41:07PM -0600, Uday Shankar wrote: > Recently, we've observed a few cases where a ublk server is able to > complete restart more quickly than the driver can process the exit of > the previous ublk server. The new ublk server comes up, attempts > recovery of the preexisting ublk devices, and observes them still in > state UBLK_S_DEV_LIVE. While this is possible due to the asynchronous > nature of io_uring cleanup and should therefore be handled properly in > the ublk server, it is still preferable to make ublk server exit > handling faster if possible, as we should strive for it to not be a > limiting factor in how fast a ublk server can restart and provide > service again. > > Analysis of the issue showed that the vast majority of the time spent in > handling the ublk server exit was in calls to blk_mq_quiesce_queue, > which is essentially just a (relatively expensive) call to > synchronize_rcu. The ublk server exit path currently issues an > unnecessarily large number of calls to blk_mq_quiesce_queue, for two > reasons: > > 1. It tries to call blk_mq_quiesce_queue once per ublk_queue. However, > blk_mq_quiesce_queue targets the request_queue of the underlying ublk > device, of which there is only one. So the number of calls is larger > than necessary by a factor of nr_hw_queues. > 2. In practice, it calls blk_mq_quiesce_queue _more_ than once per > ublk_queue. This is because of a data race where we read > ubq->canceling without any locking when deciding if we should call > ublk_start_cancel. It is thus possible for two calls to > ublk_uring_cmd_cancel_fn against the same ublk_queue to both call > ublk_start_cancel against the same ublk_queue. > > Fix this by making the "canceling" flag a per-device state. This > actually matches the existing code better, as there are several places > where the flag is set or cleared for all queues simultaneously, and > there is the general expectation that cancellation corresponds with ublk > server exit. This per-device canceling flag is then checked under a > (new) lock (addressing the data race (2) above), and the queue is only > quiesced if it is cleared (addressing (1) above). The result is just one > call to blk_mq_quiesce_queue per ublk device. > > To minimize the number of cache lines that are accessed in the hot path, > the per-queue canceling flag is kept. The values of the per-device > canceling flag and all per-queue canceling flags should always match. > > In our setup, where one ublk server handles I/O for 128 ublk devices, > each having 24 hardware queues of depth 4096, here are the results > before and after this patch, where teardown time is measured from the > first call to io_ring_ctx_wait_and_kill to the return from the last > ublk_ch_release: > > before after > number of calls to blk_mq_quiesce_queue: 6469 256 > teardown time: 11.14s 2.44s > > There are still some potential optimizations here, but this takes care > of a big chunk of the ublk server exit handling delay. > > Signed-off-by: Uday Shankar <ushankar@xxxxxxxxxxxxxxx> Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx> thanks, Ming
