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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

On Sun, Sep 06, 2026 at 12:13 +0200, Abd-Alrhman Masalkhi wrote:
> Hi Jack,
>
> On Sun, Sep 06, 2026 at 06:58 +0200, 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 am working on a bug in slot_store(). It calls remove_and_add_spares()
> without suspending the array. If I just suspend the array without taking
> care of q->limits_lock, it would  trigger the same issue you
> mentioned.

To clarify, I haven't submitted my patch for slot_store() yet. I was 
going to, but your report shows that won't be enough. I'll hold off on
my changes so I can use your queue_limits threading approach.

> 2- state_store() also calls remove_and_add_spares(), while the array is
> suspended by rdev_attr_store(), so it would probably trigger the same
> issue as well.
>
>> 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.
>>
>> Thanks,
>> Jack
>>
>
> -- 
> Best Regards,
> Abd-Alrhman

-- 
Best Regards,
Abd-Alrhman