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

["yu kuai" <yukuai@fygo.io>]

Hi,

在 2026/9/10 16:11, Jack Wang 写道:
> 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

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/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(-)
>
-- 
Thanks,
Kuai