Re: [BUG] md/raid1: deadlock between a queue limits sysfs store and a spare re-add

[Nilay Shroff <nilay@linux.ibm.com>]

On 9/6/26 10:28 AM, Jinpu Wang wrote:
> Hi,
> 
> writing to a queue limit attribute of an md array while a spare is being
> re-added deadlocks the array, with three tasks left unkillable in D state.
> The host has to be rebooted to recover.
> 
> We hit this in production on 6.12.100 with RAID1 arrays, triggered by a
> udev rule writing queue/max_sectors_kb.  Reading the code, v7.2 looks
> affected too; see "Which versions" below for exactly what was tested and
> what was not.
> 
> The cycle
> =========
> 
> Three tasks, one array:
> 
>    udev-worker     queue_attr_store() holds q->limits_lock and waits in
>                    blk_mq_freeze_queue() for q_usage_counter to drain
> 
>    fio             holds a q_usage_counter reference, waits in
>                    md_handle_request()'s is_suspended() loop
> 
>    md_start_sync   holds mddev->suspended, waits for q->limits_lock
> 
> Nobody can proceed: the freeze needs the in-flight I/O to finish, that
> I/O needs mddev->suspended cleared, and clearing it needs the spare add
> to finish, which is blocked on the lock the first task holds.
> 
> The three legs in v7.2 (8d3ae59288f1):
> 
> block/blk-sysfs.c, queue_attr_store():
> 
> struct queue_limits lim = queue_limits_start_update(q);
> 
> res = entry->store_limit(disk, page, length, &lim);
> if (res < 0) {
> queue_limits_cancel_update(q);
> return res;
> }
> 
> res = queue_limits_commit_update_frozen(q, &lim);
> 
> queue_limits_start_update() takes q->limits_lock, and
> queue_limits_commit_update_frozen() calls blk_mq_freeze_queue() with it
> still held.  max_sectors_kb is a QUEUE_LIM_RW_ENTRY, so a plain
> 
> echo 1024 > /sys/block/mdN/queue/max_sectors_kb
> 
> reaches this path.
> 
> drivers/md/md.c, md_start_sync():
> 
> if (mddev->reshape_position == MaxSector &&
>     md_spares_need_change(mddev)) {
> suspend = true;
> mddev_suspend(mddev, false);
> }
> 
> mddev_lock_nointr(mddev);
> 
> and from there md_choose_sync_action() -> remove_and_add_spares() ->
> ->hot_add_disk() -> raid1_add_disk() -> mddev_stack_new_rdev(), which
> does a blocking
> 
> lim = queue_limits_start_update(mddev->gendisk->queue);
> 
> while mddev->suspended is set.
> 
> drivers/md/md.c, md_handle_request(), where the in-flight I/O waits:
> 
> if (is_suspended(mddev, bio)) {
> ...
> wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
> 
> So md acquires q->limits_lock while holding a quiescing primitive that
> blocks exactly the I/O a concurrent freeze is waiting to drain.
> 
> Backtraces
> ==========
> 
>  From a 6.12.100 based kernel:
> 
>    INFO: task kworker/2:2 blocked for more than 184 seconds.
>    Workqueue: md_misc md_start_sync [md_mod]
>    Call Trace:
>     __mutex_lock.constprop.0+0x31c/0x6d0
>     mddev_stack_new_rdev+0x59/0x150 [md_mod]
>     raid1_add_disk+0x97/0x180 [raid1]
>     remove_and_add_spares+0xe8/0x230 [md_mod]
>     md_start_sync+0x14c/0x3e0 [md_mod]
>     process_one_work+0x162/0x370
> 
>    INFO: task (udev-worker) blocked for more than 184 seconds.
>    Call Trace:
>     blk_mq_freeze_queue_wait+0x9e/0xd0
>     queue_limits_commit_update_frozen+0x12/0x40
>     queue_attr_store+0xc9/0x1c0
>     kernfs_fop_write_iter+0x133/0x220
>     vfs_write+0x29c/0x450
> 
>    INFO: task fio blocked for more than 184 seconds.
>    Call Trace:
>     md_handle_request+0x10d/0x2b0 [md_mod]
>     __submit_bio+0x23e/0x2f0
>     submit_bio_noacct_nocheck+0x1a3/0x3c0
>     blkdev_direct_IO+0x265/0x5d0
> 
> /proc/mdstat at that point, with the spare add never completing:
> 
>    md0 : active raid1 rnbd0[0] rnbd1[1](S)
>          5238784 blocks super 1.2 [2/1] [U_]
> 
> Reproducer
> ==========
> 
> On a scratch machine, with two ram devices:
> 
>    mdadm -C /dev/md111 --force -e 1.2 --assume-clean -l 1 \
>          --bitmap=internal -n 2 /dev/ram0 /dev/ram1
> 
>    # keep I/O in flight
>    fio --direct=1 --rw=randrw --ioengine=libaio --iodepth=32 --numjobs=4 \
>        --time_based=1 --runtime=180 --filename=/dev/md111 --name=repro &
> 
>    # stand in for the udev worker
>    while :; do
>        echo 128 > /sys/block/md111/queue/max_sectors_kb 2>/dev/null
>    done &
> 
>    # drive spare re-adds
>    for i in $(seq 20); do
>        mdadm /dev/md111 --fail /dev/ram0
>        mdadm /dev/md111 --remove /dev/ram0
>        mdadm /dev/md111 --add /dev/ram0
>        mdadm --wait /dev/md111
>    done
> 
> It reproduced on the first iteration for us, though it is a race, so it
> may need a few attempts on other machines.
> 
> Which versions
> ==============
> 
> Reproduced: 6.12.100 (distro kernel carrying the stable backport of
> c99f66e4084a), RAID1, repeatedly, on several hosts.
> 
> Not reproduced, code inspection only: v7.2 (8d3ae59288f1).  All three
> legs quoted above are from the v7.2 tree and are unchanged there, so it
> looks affected, but we have not run the reproducer on a mainline build.
> Happy to do that if it helps.
> 
> raid10 calls mddev_stack_new_rdev() from raid10_add_disk() in the same
> way, so it looks exposed too; we have only tested raid1.
> 
> When it started
> ===============
> 
> Before commit c99f66e4084a ("block: fix queue freeze vs limits lock
> order in sysfs store methods"), queue_attr_store() froze the queue first
> and took limits_lock afterwards, so limits_lock was never held across the
> freeze wait and this cycle could not form.  That commit moved the freeze
> inside queue_limits_commit_update_frozen(), i.e. under limits_lock:
> 
>    Fixes: c99f66e4084a ("block: fix queue freeze vs limits lock order
> in sysfs store methods")
> 
> That is not an argument for reverting it: it exists so sd_revalidate_disk()
> can issue SCSI commands while holding the limits lock, which cannot work
> on a frozen queue.  The md side is on the wrong side of the ordering the
> block layer expects, as spelled out in commit 06a2ff603f1f ("loop: Fix
> recently introduced lock inversion"): all block driver code takes
> queue_limits_start_update() before freezing.  md instead takes a
> quiescing primitive of its own first.
> 
> What we are running
> ===================
> 
> We fixed it on the md side, by taking the limits update in
> md_start_sync() before mddev_suspend(), threading the queue_limits
> through ->hot_add_disk() so the personality stacks into it without
> taking the lock itself, and committing before resuming, so the update
> still lands while the array is quiesced.  That removes the inversion
> without touching the shared block layer code.
> 
I think you nailed it. We settled on the locking order in the block layer
with commit c99f66e4084a ("block: fix queue freeze vs limits lock order in
sysfs store methods"), where we now acquire q->limits_lock before
freezing the queue. This ordering is followed by the block layer code that
updates queue limits, while md appears to be an exception since it still
uses mddev_suspend() to stall I/O instead of blk_mq_freeze_queue().

So to me, your proposed approach looks reasonable: acquire
q->limits_lock first and then call mddev_suspend(). This should avoid
the lock inversion described above while still allowing the queue limits
update to be performed while the array is quiesced.

> Two approaches we tried first and discarded, in case they save someone
> the detour:
> 
> - mutex_trylock() in mddev_stack_new_rdev() with a retry on contention.
>    A writer that keeps retaking limits_lock wins nearly every time, so
>    the retry does not converge: we measured 1132 backoffs against 1
>    success, the array staying degraded with an idle spare throughout, and
>    md_check_recovery() suspending and resuming the array on every pass.
> 
> - Dropping limits_lock around the freeze wait inside
>    queue_limits_commit_update_frozen().  This works, but it inverts the
>    ordering the block layer has standardised on, and a concurrent update
>    committing in the window is then silently overwritten.
> 
> We can post the md-side patch if that direction looks right, or defer to
> whatever you prefer.  Reproducer script and the full logs are available.
> 
I think you should send out the md-side changes so that others can review the
locking changes and comment on the approach.

Thanks,
--Nilay