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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Jack and Nilay,

On Tue, Sep 08, 2026 at 14:09 +0200, Jinpu Wang wrote:
> On Tue, Sep 8, 2026 at 1:16 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>>
>> On 9/7/26 7:09 PM, Jack Wang wrote:
>> > 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.
>> >
>> > 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().
>
> 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.
>
I am thinking about changeing the order of reconfig_mutex and the
suspention. we would suspend the array inside raid1_add_disk()
and raid1_remove_disk() when we add/remove the rdev from raid1 conf.

> 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.
>
> Thanks,
> Jack

-- 
Best Regards,
Abd-Alrhman