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

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.

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