[PATCH v2 0/8] md: don't wait for q->limits_lock while md holds back I/O

[Jack Wang <jinpu.wang@ionos.com>]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

Writing to a queue limits attribute of an md array while a spare is
being re-added deadlocks the array.  I reported this earlier here:

  https://lore.kernel.org/linux-raid/CAMGffE=heGA3y8FjQ0Sm1jj-kd-=H9Y54WozKASSEZhc9UNKjA@mail.gmail.com/

Four tasks, one array:

  udev-worker    queue_attr_store() holds q->limits_lock, waits in
                 blk_mq_freeze_queue() for q_usage_counter to drain
  fio            holds a q_usage_counter reference, parked in
                 md_handle_request()'s is_suspended() loop
  mdadm          suspended the array, waits for reconfig_mutex
  md_start_sync  holds reconfig_mutex, waits for q->limits_lock

The last leg is mddev_stack_new_rdev() from ->hot_add_disk().  Since
commit c99f66e4084a ("block: fix queue freeze vs limits lock order in
sysfs store methods") the sysfs store holds q->limits_lock across the
freeze, so md must not block on that lock while it is holding back the
I/O the freeze waits for.  That is the same hazard mddev_suspend()
already documents for reconfig_mutex.

v1 kept q->limits_lock outermost where it could and used a trylock
everywhere else.  Christoph asked for the lock ordering to be fixed
instead, so v2 drops the trylock and the block patch that added it.

The rule v2 applies is that q->limits_lock nests outside reconfig_mutex
and the suspend everywhere, with no exceptions to paper over.  The two
callers that cannot own an update do not need one: check_sb_changes()
and md_check_recovery() only ever re-add a device that is already a
member, so its limits are stacked and the update is a refresh, and they
now take the no-stack path unconditionally.  mddev_update_io_opt() is
not an add and does need the update, so patch 4 defers it to a work
item that takes the lock with nothing held.

Making the hoists unconditional exposed a second cycle that the trylock
had been hiding, which Nilay hit with lockdep on v1:

  q->limits_lock -> reconfig_mutex        added by this series
  reconfig_mutex -> disk->open_mutex      pre-existing, md opens legs
                                          under reconfig_mutex
  disk->open_mutex -> q->limits_lock      pre-existing, sd_open() ->
                                          sd_revalidate_disk()

Only the middle edge can go, so legs are now opened before any md lock
is taken.  Patch 7 does the opens and patch 8 the holder links, which
take disk->open_mutex too and were easy to miss because the release
side only takes blk_holder_mutex.  Opening without reconfig_mutex means
the superblock format fields have to be snapshotted and rechecked once
the array is locked.

Patches 1, 3 and 6 are plumbing with no functional change.

Two callers still take q->limits_lock inside reconfig_mutex, both with
the array suspended, and patch 5 says why: ->start_reshape() from
action_store(), which suspends before flushing sync_work, and
raid*_run() -> queue_limits_set() from level_store(), which already
hangs on its own because it freezes the queue while suspended.  Both
need more restructuring than belongs here.

The patches are based on v7.3-rc2.

v1: https://lore.kernel.org/linux-raid/20260909063029.GC29874@lst.de/T/#t

Changes since v1:
 - Drop the trylock and the block patch adding
   queue_limits_start_update_trylock(), per Christoph.
 - check_sb_changes() and md_check_recovery() take the no-stack path
   unconditionally instead of trying the lock first.
 - Defer the io_opt update to a work item rather than skipping it on a
   contended pass (patch 4, was "md: don't wait for q->limits_lock in
   mddev_update_io_opt()").
 - New patches 6-8 to close the reconfig_mutex -> disk->open_mutex
   edge that lockdep reported on v1: pass a queue_limits through
   ->run(), open new legs before locking the array, and link their
   holders there too.
 - Rebased on v7.3-rc2.

Testing, on v7.3-rc2 with lockdep (PROVE_LOCKING, DEBUG_LOCK_ALLOC):

 - the reproducer below, 20 fail/remove/add cycles with fio in flight
   and a loop writing queue/max_sectors_kb, where the same test wedges
   the array before the series
 - array start at raid0, raid1, raid5, raid10 and linear, spare add,
   fail and remove, the rdev sysfs stores, ADD_NEW_DISK, array_state
   transitions, a level change and a raid5 3 -> 4 reshape to
   completion, which is what exercises patch 4's work item: io_opt
   goes from 1048576 to 1572864 once end_reshape() runs
 - HOT_ADD_DISK of an undersized leg, so bind_rdev_to_array() fails
   and patch 8's release path runs

No lockdep reports, and debug_locks stayed 1 throughout.  Every patch
builds on its own.

A blktests case covering this deadlock will be sent separately.

The reproducer, for anyone who wants it:

  mdadm -C /dev/md111 --force -e 1.2 --assume-clean -l 1 \
        --bitmap=internal -n 2 /dev/ram0 /dev/ram1

  fio --direct=1 --rw=randrw --ioengine=libaio --iodepth=32 --numjobs=4 \
      --time_based=1 --runtime=180 --filename=/dev/md111 --name=repro &

  while :; do
      echo 128 > /sys/block/md111/queue/max_sectors_kb 2>/dev/null
  done &

  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

Jack Wang (8):
  md: pass a queue_limits down to ->hot_add_disk()
  md: don't wait for q->limits_lock in check_sb_changes()
  md: pass a queue_limits through the rdev sysfs stores
  md: defer the io_opt update out of the sync thread
  md: take q->limits_lock before locking and suspending the array
  md: pass a queue_limits through ->run()
  md: open new legs before locking the array
  md: link a new leg's holder before locking the array

 drivers/md/dm-raid.c       |   4 +-
 drivers/md/md-autodetect.c |  38 ++-
 drivers/md/md-linear.c     |  30 +-
 drivers/md/md.c            | 661 +++++++++++++++++++++++++++++--------
 drivers/md/md.h            |  56 +++-
 drivers/md/raid0.c         |  16 +-
 drivers/md/raid1.c         |  26 +-
 drivers/md/raid10.c        |  37 ++-
 drivers/md/raid5.c         |  54 ++-
 9 files changed, 741 insertions(+), 181 deletions(-)

-- 
2.43.0