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

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 [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().

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