Hi,
在 2026/9/10 16:11, Jack Wang 写道:
> From: Jack Wang <[email protected]>
>
> 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-=h9y54wozkassezhc9un...@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
Sorry for the delay, usually blk_mq_freeze_queue() is useless for bio-based
disks, because it can't guarantee inflight BIOs to drain. So this looks like
a long term problem since queue_attr_store() can still update limits while
there can still be bios inflight.
> 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
And this deadlock looks related to blk_mq_freeze_queue() concurrent with
mddev_suspend(), hence there can be bio blocked between. So I think there
can be a simpler solution for this deadlock.
1) either remove the blk_mq_freeze_queue() from limits update for mdraid, or
2) also add blk_mq_freeze_queue() to mddev_suspend().
If this is a mdraid specific problem, I'll suggest 2). If there are other
bio-based drivers have the same problem, we might what to replace
blk_mq_freeze_queue() with driver's bio suspend to fix that limits can
update with inflight bios as well.
>
> 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/[email protected]/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(-)
>
--
Thanks,
Kuai
