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

On Tue, Sep 8, 2026 at 8:25 PM Nilay Shroff [off-list ref] wrote:

On 9/8/26 5:39 PM, Jinpu Wang wrote:

quoted

On Tue, Sep 8, 2026 at 1:16 PM Nilay Shroff [off-list ref] wrote:

quoted

On 9/7/26 7:09 PM, Jack Wang wrote:

quoted

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/ (local)

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.

The rule this series applies is that q->limits_lock nests outside both
reconfig_mutex and the suspend.  Where md cannot arrange that, because
it is called with reconfig_mutex already held or from the sync thread,
it takes the update with a trylock and does without one on a contended
pass.

Patches 1, 2 and 4 are plumbing with no functional change.  Patches 3
and 5 convert the two callers that cannot own an update.  Patch 6 does
the hoists, all in one patch because a mix of the two lock orders is an
ABBA.

Two callers still take q->limits_lock inside reconfig_mutex, both with
the array suspended, and patch 6 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.

Tested there with a raid1 of two ram devices, fio in flight and a loop
writing queue/max_sectors_kb: 20 fail/remove/add cycles complete,
where the same test wedges the array before the series.  Every patch
builds on its own.  A reshape and a level change are not covered by that
test.

Overall, I think the direction looks good. With this series, we now have the
locking order where q->limits_lock is acquired before the suspend and
reconfig_mutex.

However, when I ran these changes through blktests, I hit the lockdep splat[1],
which exposes an ABBA dependency between disk->open_mutex and q->limits_lock.

Looking at the existing dependency chain, disk->open_mutex is expected to
be acquired before q->limits_lock. However, with this change, md_ioctl()
acquires q->limits_lock first and subsequently reaches md_import_device(),
which acquires disk->open_mutex. This reverses the existing lock ordering
and introduces the ABBA dependency, so I think this needs to be addressed.

Thanks for running this through blktests.

You are right, and it is worse than the one path you hit.  The import is
not the only offender: everything in the mddev->pers branch of
md_add_new_disk() that opens or closes a component device runs with
q->limits_lock held.  Besides md_import_device() there are four
export_rdev() calls and one md_kick_rdev_from_array(), and export_rdev()
ends in fput(rdev->bdev_file), so it takes disk->open_mutex too.
rdev_attr_store() looks like a second instance: it starts the update for
every state_store() write, and "remove" reaches
md_kick_rdev_from_array().

I think fput() doesn't immediately release the blkdev, as the final __fput()
is deferred through task work or delayed fput work. So that shouldn't be an
issue here. Also, md_kick_rdev_from_array() defers the cleanup until
mddev_unlock(). mddev_unlock() should be called after we release
q->limits_lock, so this should not be an issue here.

Right. Both cases are not a real issue.

quoted

So the rule needs to be stronger than what I wrote: q->limits_lock nests
outside reconfig_mutex and the suspend, and must not be held across any
component device open or close.

Two ways to get there, and I would rather hear which you prefer before
respinning:

1) Keep the lock outermost, move the open and close out from under it.
    md_ioctl() imports before taking q->limits_lock and releases after
    committing and unlocking; md_add_new_disk() hands the rdev back
    instead of exporting it.  The import then runs without
    reconfig_mutex, so the superblock format fields need a snapshot and a
    recheck under the lock.  Only the mddev->pers branch needs this, the
    other two never reach add_bound_rdev().

2) Drop the hoist for ADD_NEW_DISK and stack the leg after resume, with
    q->limits_lock on its own.  Much smaller, but the leg is live before
    its limits are stacked and the integrity rejection lands after the
    add rather than before it.

Is there a better option?  If ->hot_add_disk() is meant to be callable
with an update already in flight, that limits how far the open and close
can move.

I think moving q->limits_lock after md_import_device() in the mddev->pers
branch looks reasonable to me. This reduces code churn, and we can acquire
q->limits_lock after md_import_device() returns, before proceeding with
the queue-limit update.

We should also move the device suspend until after q->limits_lock is acquired
for ADD_NEW_DISK, so that we maintain the intended ordering of q->limits_lock
and suspend.

This also ensures that q->limits_lock is not held across component-device
open/import, avoiding the disk->open_mutex -> q->limits_lock dependency
reversal reported by lockdep.

Will give it a try.

Thanks,
--Nilay

Thanks for the suggestion.