Hi Kuai,
On Tue, Oct 20, 2026 at 8:57 AM yu kuai <[email protected]> wrote:
>
> 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.
Thanks for the suggestion, will look into it back from vacation. Yes,
it's a MDraid specific problem.
>
> >
> > 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
