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

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