* [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
@ 2026-09-07 13:39 Jack Wang
2026-09-07 13:39 ` [PATCH 1/6] block: add queue_limits_start_update_trylock() Jack Wang
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, 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
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.
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 (6):
block: add queue_limits_start_update_trylock()
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: don't wait for q->limits_lock in mddev_update_io_opt()
md: take q->limits_lock before locking and suspending the array
drivers/md/dm-raid.c | 2 +-
drivers/md/md-autodetect.c | 2 +-
drivers/md/md-linear.c | 3 +-
drivers/md/md.c | 276 +++++++++++++++++++++++++++++++------
drivers/md/md.h | 11 +-
drivers/md/raid1.c | 8 +-
drivers/md/raid10.c | 17 ++-
drivers/md/raid5.c | 38 +++--
include/linux/blkdev.h | 26 ++++
9 files changed, 315 insertions(+), 68 deletions(-)
base-commit: df2908090cda368b01ff43709f51890076c56157
--
2.43.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/6] block: add queue_limits_start_update_trylock()
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-09 6:29 ` Christoph Hellwig
2026-09-07 13:39 ` [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk() Jack Wang
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
Some callers must not wait for q->limits_lock, because they hold
something the current holder waits for. md is one: its
check_sb_changes() runs with reconfig_mutex held, while a
queue_attr_store() holding limits_lock waits in blk_mq_freeze_queue()
for I/O that can be waiting for a superblock update needing that mutex.
Add a trylock variant of queue_limits_start_update() for them.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
include/linux/blkdev.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b..b75e85291e29 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1101,6 +1101,32 @@ queue_limits_start_update(struct request_queue *q)
mutex_lock(&q->limits_lock);
return q->limits;
}
+
+/**
+ * queue_limits_start_update_trylock - try to start an atomic update of queue
+ * limits
+ * @q: queue to update
+ * @lim: returns a snapshot of the current limits on success
+ *
+ * Like queue_limits_start_update(), but fails instead of waiting when another
+ * update is in flight. For callers that must not block on q->limits_lock
+ * because they hold something its current owner is waiting for.
+ *
+ * Context: process context.
+ */
+static inline bool
+queue_limits_start_update_trylock(struct request_queue *q,
+ struct queue_limits *lim)
+ __cond_acquires(true, &q->limits_lock)
+{
+ if (!mutex_trylock(&q->limits_lock))
+ return false;
+
+ *lim = q->limits;
+
+ return true;
+}
+
int queue_limits_commit_update_frozen(struct request_queue *q,
struct queue_limits *lim) __releases(&q->limits_lock);
int queue_limits_commit_update(struct request_queue *q,
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk()
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
2026-09-07 13:39 ` [PATCH 1/6] block: add queue_limits_start_update_trylock() Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-07 13:39 ` [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes() Jack Wang
` (5 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
Adding a leg stacks its queue limits, which mddev_stack_new_rdev() does
by taking q->limits_lock itself. Callers that hold reconfig_mutex, or
have the array suspended, must not do that: the lock's holder waits for
I/O they are blocking. They need to own the update instead.
Give ->hot_add_disk(), remove_and_add_spares() and
md_choose_sync_action() a queue_limits argument, and add
mddev_stack_rdev_into() to stack into a caller-owned update. Every
caller passes NULL, so no functional change; the users follow.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
drivers/md/dm-raid.c | 2 +-
drivers/md/md-linear.c | 3 +-
drivers/md/md.c | 66 ++++++++++++++++++++++++++++++++----------
drivers/md/md.h | 5 +++-
drivers/md/raid1.c | 8 +++--
drivers/md/raid10.c | 15 +++++++---
drivers/md/raid5.c | 5 ++--
7 files changed, 78 insertions(+), 26 deletions(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8f5a5e1342a9..21a1922bee4f 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3923,7 +3923,7 @@ static void attempt_restore_of_faulty_devices(struct raid_set *rs)
clear_bit(Faulty, &r->flags);
clear_bit(WriteErrorSeen, &r->flags);
- if (mddev->pers->hot_add_disk(mddev, r)) {
+ if (mddev->pers->hot_add_disk(mddev, r, NULL)) {
/* Failed to revive this device, try next */
r->raid_disk = r->saved_raid_disk = -1;
r->flags = flags;
diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
index 73b367b61b87..1f44c2d7db93 100644
--- a/drivers/md/md-linear.c
+++ b/drivers/md/md-linear.c
@@ -186,7 +186,8 @@ static int linear_run(struct mddev *mddev)
return ret;
}
-static int linear_add(struct mddev *mddev, struct md_rdev *rdev)
+static int linear_add(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim)
{
/* Adding a drive to a linear array allows the array to grow.
* It is permitted if the new drive has a matching superblock
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..28fc903ffeea 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -94,8 +94,8 @@ static DECLARE_WAIT_QUEUE_HEAD(resync_wait);
*/
static struct workqueue_struct *md_misc_wq;
-static int remove_and_add_spares(struct mddev *mddev,
- struct md_rdev *this);
+static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this,
+ struct queue_limits *lim);
static void mddev_detach(struct mddev *mddev);
static void export_rdev(struct md_rdev *rdev);
static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
@@ -2994,7 +2994,7 @@ static int add_bound_rdev(struct md_rdev *rdev)
*/
super_types[mddev->major_version].
validate_super(mddev, NULL/*freshest*/, rdev);
- err = mddev->pers->hot_add_disk(mddev, rdev);
+ err = mddev->pers->hot_add_disk(mddev, rdev, NULL);
if (err) {
md_kick_rdev_from_array(rdev);
return err;
@@ -3110,7 +3110,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
} else if (cmd_match(buf, "remove")) {
if (rdev->mddev->pers) {
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(rdev->mddev, rdev);
+ remove_and_add_spares(rdev->mddev, rdev, NULL);
}
if (rdev->raid_disk >= 0)
err = -EBUSY;
@@ -3314,7 +3314,7 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
if (rdev->mddev->pers->hot_remove_disk == NULL)
return -EINVAL;
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(rdev->mddev, rdev);
+ remove_and_add_spares(rdev->mddev, rdev, NULL);
if (rdev->raid_disk >= 0)
return -EBUSY;
set_bit(MD_RECOVERY_NEEDED, &rdev->mddev->recovery);
@@ -3344,7 +3344,8 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
rdev->saved_raid_disk = -1;
clear_bit(In_sync, &rdev->flags);
clear_bit(Bitmap_sync, &rdev->flags);
- err = rdev->mddev->pers->hot_add_disk(rdev->mddev, rdev);
+ err = rdev->mddev->pers->hot_add_disk(rdev->mddev, rdev,
+ NULL);
if (err) {
rdev->raid_disk = -1;
return err;
@@ -6275,6 +6276,40 @@ int mddev_stack_new_rdev(struct mddev *mddev, struct md_rdev *rdev)
}
EXPORT_SYMBOL_GPL(mddev_stack_new_rdev);
+/*
+ * Stack a new rdev into limits the caller already holds limits_lock for and
+ * will commit itself. Used from paths that must take limits_lock before
+ * quiescing the array, see md_start_sync().
+ */
+int mddev_stack_rdev_into(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim)
+{
+ struct queue_limits tmp = *lim;
+
+ if (mddev_is_dm(mddev))
+ return 0;
+
+ if (queue_logical_block_size(rdev->bdev->bd_disk->queue) >
+ queue_logical_block_size(mddev->gendisk->queue)) {
+ pr_err("%s: incompatible logical_block_size, can not add\n",
+ mdname(mddev));
+ return -EINVAL;
+ }
+
+ queue_limits_stack_bdev(&tmp, rdev->bdev, rdev->data_offset,
+ mddev->gendisk->disk_name);
+
+ if (!queue_limits_stack_integrity_bdev(&tmp, rdev->bdev)) {
+ pr_err("%s: incompatible integrity profile for %pg\n",
+ mdname(mddev), rdev->bdev);
+ return -ENXIO;
+ }
+
+ *lim = tmp;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mddev_stack_rdev_into);
+
/* update the optimal I/O size after a reshape */
void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes)
{
@@ -7706,7 +7741,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
goto kick_rdev;
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(mddev, rdev);
+ remove_and_add_spares(mddev, rdev, NULL);
if (rdev->raid_disk >= 0)
goto busy;
@@ -10167,8 +10202,8 @@ static int remove_spares(struct mddev *mddev, struct md_rdev *this)
return removed;
}
-static int remove_and_add_spares(struct mddev *mddev,
- struct md_rdev *this)
+static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this,
+ struct queue_limits *lim)
{
struct md_rdev *rdev;
int spares = 0;
@@ -10191,7 +10226,7 @@ static int remove_and_add_spares(struct mddev *mddev,
continue;
if (!test_bit(Journal, &rdev->flags))
rdev->recovery_offset = 0;
- if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
+ if (mddev->pers->hot_add_disk(mddev, rdev, lim) == 0) {
/* failure here is OK */
sysfs_link_rdev(mddev, rdev);
if (!test_bit(Journal, &rdev->flags))
@@ -10206,7 +10241,8 @@ static int remove_and_add_spares(struct mddev *mddev,
return spares;
}
-static bool md_choose_sync_action(struct mddev *mddev, int *spares)
+static bool md_choose_sync_action(struct mddev *mddev, int *spares,
+ struct queue_limits *lim)
{
/* Check if reshape is in progress first. */
if (mddev->reshape_position != MaxSector) {
@@ -10234,7 +10270,7 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
* also removed and re-added, to allow the personality to fail the
* re-add.
*/
- *spares = remove_and_add_spares(mddev, NULL);
+ *spares = remove_and_add_spares(mddev, NULL, lim);
if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
@@ -10294,11 +10330,11 @@ static void md_start_sync(struct work_struct *ws)
* As we only add devices that are already in-sync, we can
* activate the spares immediately.
*/
- remove_and_add_spares(mddev, NULL);
+ remove_and_add_spares(mddev, NULL, NULL);
goto not_running;
}
- if (!md_choose_sync_action(mddev, &spares))
+ if (!md_choose_sync_action(mddev, &spares, NULL))
goto not_running;
if (!mddev->pers->sync_request)
@@ -10849,7 +10885,7 @@ static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
rdev2->saved_raid_disk = -1;
else
rdev2->saved_raid_disk = role;
- ret = remove_and_add_spares(mddev, rdev2);
+ ret = remove_and_add_spares(mddev, rdev2, NULL);
pr_info("Activated spare: %pg\n",
rdev2->bdev);
/* wakeup mddev->thread here, so array could
diff --git a/drivers/md/md.h b/drivers/md/md.h
index b6d2e8929a0f..39b95951cc17 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -765,7 +765,8 @@ struct md_personality
* if appropriate, and should abort recovery if needed
*/
void (*error_handler)(struct mddev *mddev, struct md_rdev *rdev);
- int (*hot_add_disk) (struct mddev *mddev, struct md_rdev *rdev);
+ int (*hot_add_disk)(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim);
int (*hot_remove_disk) (struct mddev *mddev, struct md_rdev *rdev);
int (*spare_active) (struct mddev *mddev);
sector_t (*sync_request)(struct mddev *mddev, sector_t sector_nr,
@@ -1047,6 +1048,8 @@ int do_md_run(struct mddev *mddev);
int mddev_stack_rdev_limits(struct mddev *mddev, struct queue_limits *lim,
unsigned int flags);
int mddev_stack_new_rdev(struct mddev *mddev, struct md_rdev *rdev);
+int mddev_stack_rdev_into(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim);
void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes);
extern const struct block_device_operations md_fops;
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f0646fb24371..dd348b5695d0 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1898,7 +1898,8 @@ static bool raid1_remove_conf(struct r1conf *conf, int disk)
return true;
}
-static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
+static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim)
{
struct r1conf *conf = mddev->private;
int err = -EEXIST;
@@ -1923,7 +1924,10 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
for (mirror = first; mirror <= last; mirror++) {
p = conf->mirrors + mirror;
if (!p->rdev) {
- err = mddev_stack_new_rdev(mddev, rdev);
+ if (lim)
+ err = mddev_stack_rdev_into(mddev, rdev, lim);
+ else
+ err = mddev_stack_new_rdev(mddev, rdev);
if (err)
return err;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 1093c798d9dd..a5b65f377d04 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2095,7 +2095,8 @@ static int raid10_spare_active(struct mddev *mddev)
return count;
}
-static int raid10_add_disk(struct mddev *mddev, struct md_rdev *rdev)
+static int raid10_add_disk(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim)
{
struct r10conf *conf = mddev->private;
int err = -EEXIST;
@@ -2130,7 +2131,10 @@ static int raid10_add_disk(struct mddev *mddev, struct md_rdev *rdev)
continue;
}
- err = mddev_stack_new_rdev(mddev, rdev);
+ if (lim)
+ err = mddev_stack_rdev_into(mddev, rdev, lim);
+ else
+ err = mddev_stack_new_rdev(mddev, rdev);
if (err)
return err;
p->head_position = 0;
@@ -2147,7 +2151,10 @@ static int raid10_add_disk(struct mddev *mddev, struct md_rdev *rdev)
clear_bit(In_sync, &rdev->flags);
set_bit(Replacement, &rdev->flags);
rdev->raid_disk = repl_slot;
- err = mddev_stack_new_rdev(mddev, rdev);
+ if (lim)
+ err = mddev_stack_rdev_into(mddev, rdev, lim);
+ else
+ err = mddev_stack_new_rdev(mddev, rdev);
if (err)
return err;
conf->fullsync = 1;
@@ -4484,7 +4491,7 @@ static int raid10_start_reshape(struct mddev *mddev)
rdev_for_each(rdev, mddev)
if (rdev->raid_disk < 0 &&
!test_bit(Faulty, &rdev->flags)) {
- if (raid10_add_disk(mddev, rdev) == 0) {
+ if (raid10_add_disk(mddev, rdev, NULL) == 0) {
if (rdev->raid_disk >=
conf->prev.raid_disks)
set_bit(In_sync, &rdev->flags);
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index b91545ce090d..0ec555ada64a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8441,7 +8441,8 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
return err;
}
-static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
+static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev,
+ struct queue_limits *lim)
{
struct r5conf *conf = mddev->private;
int ret, err = -EEXIST;
@@ -8728,7 +8729,7 @@ static int raid5_start_reshape(struct mddev *mddev)
rdev_for_each(rdev, mddev)
if (rdev->raid_disk < 0 &&
!test_bit(Faulty, &rdev->flags)) {
- if (raid5_add_disk(mddev, rdev) == 0) {
+ if (raid5_add_disk(mddev, rdev, NULL) == 0) {
if (rdev->raid_disk
>= conf->previous_raid_disks)
set_bit(In_sync, &rdev->flags);
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes()
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
2026-09-07 13:39 ` [PATCH 1/6] block: add queue_limits_start_update_trylock() Jack Wang
2026-09-07 13:39 ` [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk() Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-07 13:39 ` [PATCH 4/6] md: pass a queue_limits through the rdev sysfs stores Jack Wang
` (4 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
check_sb_changes() activates a spare added by another node, with
reconfig_mutex held, from md_reload_sb(). Stacking that leg's limits
waits for q->limits_lock while holding the mutex, and the lock's holder
waits for I/O that can be waiting for a superblock update needing the
same mutex.
Take the update without blocking and keep the activation inline. On a
contended pass only remove; MD_RECOVERY_NEEDED is set below, so
md_start_sync() does the add.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
drivers/md/md.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 28fc903ffeea..c7d1f9813f5d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6310,6 +6310,20 @@ int mddev_stack_rdev_into(struct mddev *mddev, struct md_rdev *rdev,
}
EXPORT_SYMBOL_GPL(mddev_stack_rdev_into);
+/* for callers that must not wait for q->limits_lock, see md_start_sync() */
+static bool mddev_stack_limits_trylock(struct mddev *mddev,
+ struct queue_limits *lim)
+{
+ struct request_queue *q;
+
+ if (mddev_is_dm(mddev))
+ return false;
+
+ q = mddev->gendisk->queue;
+
+ return queue_limits_start_update_trylock(q, lim);
+}
+
/* update the optimal I/O size after a reshape */
void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes)
{
@@ -10829,6 +10843,7 @@ static int __init md_init(void)
static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
{
+ struct queue_limits lim;
struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
struct md_rdev *rdev2, *tmp;
int role, ret;
@@ -10885,7 +10900,24 @@ static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
rdev2->saved_raid_disk = -1;
else
rdev2->saved_raid_disk = role;
- ret = remove_and_add_spares(mddev, rdev2, NULL);
+ /*
+ * reconfig_mutex is held, so don't wait for
+ * q->limits_lock; MD_RECOVERY_NEEDED below
+ * leaves a skipped add to md_start_sync().
+ */
+ if (mddev_stack_limits_trylock(mddev, &lim)) {
+ struct request_queue *q =
+ mddev->gendisk->queue;
+
+ ret = remove_and_add_spares(mddev,
+ rdev2, &lim);
+ queue_limits_commit_update(q, &lim);
+ } else if (mddev_is_dm(mddev)) {
+ ret = remove_and_add_spares(mddev,
+ rdev2, NULL);
+ } else {
+ ret = remove_spares(mddev, rdev2);
+ }
pr_info("Activated spare: %pg\n",
rdev2->bdev);
/* wakeup mddev->thread here, so array could
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/6] md: pass a queue_limits through the rdev sysfs stores
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
` (2 preceding siblings ...)
2026-09-07 13:39 ` [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes() Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-07 13:39 ` [PATCH 5/6] md: don't wait for q->limits_lock in mddev_update_io_opt() Jack Wang
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
state_store() and slot_store() can add a leg, which stacks its queue
limits, and the caller has to own that update because q->limits_lock
must be taken before the array is locked and suspended.
Give the rdev sysfs store callback a queue_limits argument.
rdev_attr_store() passes NULL, so no functional change; the user
follows.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
drivers/md/md.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index c7d1f9813f5d..8ad6fe178e96 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3033,7 +3033,13 @@ static int cmd_match(const char *cmd, const char *str)
struct rdev_sysfs_entry {
struct attribute attr;
ssize_t (*show)(struct md_rdev *, char *);
- ssize_t (*store)(struct md_rdev *, const char *, size_t);
+ /*
+ * @lim: a queue limits update the caller owns, or NULL. Stores that
+ * can add a leg to the array must stack into it rather than take
+ * q->limits_lock themselves, see md_start_sync().
+ */
+ ssize_t (*store)(struct md_rdev *rdev, const char *page, size_t len,
+ struct queue_limits *lim);
};
static ssize_t
@@ -3079,7 +3085,8 @@ state_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-state_store(struct md_rdev *rdev, const char *buf, size_t len)
+state_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
/* can write
* faulty - simulates an error
@@ -3257,7 +3264,8 @@ errors_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-errors_store(struct md_rdev *rdev, const char *buf, size_t len)
+errors_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned int n;
int rv;
@@ -3283,7 +3291,8 @@ slot_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-slot_store(struct md_rdev *rdev, const char *buf, size_t len)
+slot_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
int slot;
int err;
@@ -3378,7 +3387,8 @@ offset_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-offset_store(struct md_rdev *rdev, const char *buf, size_t len)
+offset_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned long long offset;
if (kstrtoull(buf, 10, &offset) < 0)
@@ -3404,7 +3414,8 @@ static ssize_t new_offset_show(struct md_rdev *rdev, char *page)
}
static ssize_t new_offset_store(struct md_rdev *rdev,
- const char *buf, size_t len)
+ const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned long long new_offset;
struct mddev *mddev = rdev->mddev;
@@ -3511,7 +3522,8 @@ static int strict_blocks_to_sectors(const char *buf, sector_t *sectors)
}
static ssize_t
-rdev_size_store(struct md_rdev *rdev, const char *buf, size_t len)
+rdev_size_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
struct mddev *my_mddev = rdev->mddev;
sector_t oldsectors = rdev->sectors;
@@ -3573,7 +3585,8 @@ static ssize_t recovery_start_show(struct md_rdev *rdev, char *page)
return sprintf(page, "%llu\n", recovery_start);
}
-static ssize_t recovery_start_store(struct md_rdev *rdev, const char *buf, size_t len)
+static ssize_t recovery_start_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned long long recovery_start;
@@ -3612,7 +3625,9 @@ static ssize_t bb_show(struct md_rdev *rdev, char *page)
{
return badblocks_show(&rdev->badblocks, page, 0);
}
-static ssize_t bb_store(struct md_rdev *rdev, const char *page, size_t len)
+
+static ssize_t bb_store(struct md_rdev *rdev, const char *page, size_t len,
+ struct queue_limits *lim)
{
int rv = badblocks_store(&rdev->badblocks, page, len, 0);
/* Maybe that ack was all we needed */
@@ -3627,7 +3642,9 @@ static ssize_t ubb_show(struct md_rdev *rdev, char *page)
{
return badblocks_show(&rdev->badblocks, page, 1);
}
-static ssize_t ubb_store(struct md_rdev *rdev, const char *page, size_t len)
+
+static ssize_t ubb_store(struct md_rdev *rdev, const char *page, size_t len,
+ struct queue_limits *lim)
{
return badblocks_store(&rdev->badblocks, page, len, 1);
}
@@ -3641,7 +3658,8 @@ ppl_sector_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-ppl_sector_store(struct md_rdev *rdev, const char *buf, size_t len)
+ppl_sector_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned long long sector;
@@ -3680,7 +3698,8 @@ ppl_size_show(struct md_rdev *rdev, char *page)
}
static ssize_t
-ppl_size_store(struct md_rdev *rdev, const char *buf, size_t len)
+ppl_size_store(struct md_rdev *rdev, const char *buf, size_t len,
+ struct queue_limits *lim)
{
unsigned int size;
@@ -3766,7 +3785,7 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
if (rdev->mddev == NULL)
rv = -ENODEV;
else
- rv = entry->store(rdev, page, length);
+ rv = entry->store(rdev, page, length, NULL);
suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/6] md: don't wait for q->limits_lock in mddev_update_io_opt()
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
` (3 preceding siblings ...)
2026-09-07 13:39 ` [PATCH 4/6] md: pass a queue_limits through the rdev sysfs stores Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-07 13:39 ` [PATCH 6/6] md: take q->limits_lock before locking and suspending the array Jack Wang
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
end_reshape() calls this from the sync thread, and
md_reap_sync_thread() waits for that thread with reconfig_mutex held.
Waiting for q->limits_lock here hangs a finishing reshape when the
lock's holder is waiting for I/O that only md_check_recovery() can
complete.
Use the trylock and skip the change on a contended pass; io_opt is a
hint. Callers that own an update pass it in and have it changed in
place.
Only the lock leg is addressed; the same cycle also runs through the
mddev_suspend() below, which this function has always done from the
sync thread.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
drivers/md/md.c | 33 +++++++++++++++++++++++++++------
drivers/md/md.h | 3 ++-
drivers/md/raid10.c | 2 +-
drivers/md/raid5.c | 2 +-
4 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 8ad6fe178e96..6af11a74db57 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6344,19 +6344,40 @@ static bool mddev_stack_limits_trylock(struct mddev *mddev,
}
/* update the optimal I/O size after a reshape */
-void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes)
+void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes,
+ struct queue_limits *lim)
{
- struct queue_limits lim;
+ struct queue_limits own;
if (mddev_is_dm(mddev))
return;
+ /*
+ * With an update owned by the caller just change it in place; it is
+ * committed, and the array resumed, by whoever started it. Taking
+ * q->limits_lock here would nest it inside reconfig_mutex and the
+ * suspend, which deadlocks, see md_start_sync().
+ */
+ if (lim) {
+ lim->io_opt = lim->io_min * nr_stripes;
+ return;
+ }
+
+ /*
+ * Called from the sync thread, which md_reap_sync_thread() waits for
+ * with reconfig_mutex held, so don't wait for q->limits_lock here.
+ * io_opt is a hint, skipping it on a contended pass is fine.
+ */
+ if (!mddev_stack_limits_trylock(mddev, &own))
+ return;
+
/* don't bother updating io_opt if we can't suspend the array */
- if (mddev_suspend(mddev, false) < 0)
+ if (mddev_suspend(mddev, false) < 0) {
+ queue_limits_cancel_update(mddev->gendisk->queue);
return;
- lim = queue_limits_start_update(mddev->gendisk->queue);
- lim.io_opt = lim.io_min * nr_stripes;
- queue_limits_commit_update(mddev->gendisk->queue, &lim);
+ }
+ own.io_opt = own.io_min * nr_stripes;
+ queue_limits_commit_update(mddev->gendisk->queue, &own);
mddev_resume(mddev);
}
EXPORT_SYMBOL_GPL(mddev_update_io_opt);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 39b95951cc17..9e3bd5ab5519 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -1050,7 +1050,8 @@ int mddev_stack_rdev_limits(struct mddev *mddev, struct queue_limits *lim,
int mddev_stack_new_rdev(struct mddev *mddev, struct md_rdev *rdev);
int mddev_stack_rdev_into(struct mddev *mddev, struct md_rdev *rdev,
struct queue_limits *lim);
-void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes);
+void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes,
+ struct queue_limits *lim);
extern const struct block_device_operations md_fops;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index a5b65f377d04..641619328a6c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4928,7 +4928,7 @@ static void end_reshape(struct r10conf *conf)
conf->reshape_safe = MaxSector;
spin_unlock_irq(&conf->device_lock);
- mddev_update_io_opt(conf->mddev, raid10_nr_stripes(conf));
+ mddev_update_io_opt(conf->mddev, raid10_nr_stripes(conf), NULL);
conf->fullsync = 0;
}
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0ec555ada64a..3faa2a94c03b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8800,7 +8800,7 @@ static void end_reshape(struct r5conf *conf)
wake_up(&conf->wait_for_reshape);
mddev_update_io_opt(conf->mddev,
- conf->raid_disks - conf->max_degraded);
+ conf->raid_disks - conf->max_degraded, NULL);
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 6/6] md: take q->limits_lock before locking and suspending the array
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
` (4 preceding siblings ...)
2026-09-07 13:39 ` [PATCH 5/6] md: don't wait for q->limits_lock in mddev_update_io_opt() Jack Wang
@ 2026-09-07 13:39 ` Jack Wang
2026-09-08 11:15 ` [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Nilay Shroff
2026-09-08 17:00 ` Johannes Thumshirn
7 siblings, 0 replies; 18+ messages in thread
From: Jack Wang @ 2026-09-07 13:39 UTC (permalink / raw)
To: Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
Writing a queue limits attribute while a spare is re-added deadlocks
the 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
Blocking on q->limits_lock while holding reconfig_mutex, or with the
array suspended, is waiting for normal I/O, which mddev_suspend()
already warns about:
* hold reconfig_mutex to wait for normal io will deadlock, because
* other context can't update super_block, and normal io can rely on
* updating super_block.
lockdep_assert_not_held(&mddev->reconfig_mutex);
The lock's holder waits for the queue to drain, and that I/O can be
parked on mddev->suspended, which another task holds while waiting for
reconfig_mutex. So q->limits_lock has to nest outside both.
Take the update before the array is locked and suspended, and pass it
down so the personality stacks into it:
- md_start_sync(), at both suspend points
- md_ioctl() for ADD_NEW_DISK and HOT_REMOVE_DISK
- rdev_attr_store(), for slot and for state "remove"/"re-add"
- raid5 skip_copy_store(), which took the lock while suspended
They are converted together because a mix of the two orders is an ABBA.
All of them commit while the array is still quiesced.
Two callers still take the lock inside reconfig_mutex, both with the
array suspended: ->start_reshape() from action_store(), which suspends
before flushing sync_work so the update cannot be held across it, and
raid*_run() -> queue_limits_set() from level_store(), which already
hangs on its own because it freezes the queue while suspended.
Verified 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 change. A reshape and a level
change are not covered by it.
Fixes: c99f66e4084a ("block: fix queue freeze vs limits lock order in sysfs store methods")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
drivers/md/md-autodetect.c | 2 +-
drivers/md/md.c | 116 +++++++++++++++++++++++++++++++------
drivers/md/md.h | 3 +-
drivers/md/raid5.c | 31 +++++++---
4 files changed, 124 insertions(+), 28 deletions(-)
diff --git a/drivers/md/md-autodetect.c b/drivers/md/md-autodetect.c
index 4b80165afd23..929513109657 100644
--- a/drivers/md/md-autodetect.c
+++ b/drivers/md/md-autodetect.c
@@ -213,7 +213,7 @@ static void __init md_setup_drive(struct md_setup_args *args)
(1 << MD_DISK_ACTIVE) | (1 << MD_DISK_SYNC);
}
- md_add_new_disk(mddev, &dinfo);
+ md_add_new_disk(mddev, &dinfo, NULL);
}
if (!err)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 6af11a74db57..c4054d06d107 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2981,7 +2981,7 @@ void md_update_sb(struct mddev *mddev, int force_change)
}
EXPORT_SYMBOL(md_update_sb);
-static int add_bound_rdev(struct md_rdev *rdev)
+static int add_bound_rdev(struct md_rdev *rdev, struct queue_limits *lim)
{
struct mddev *mddev = rdev->mddev;
int err = 0;
@@ -2994,7 +2994,7 @@ static int add_bound_rdev(struct md_rdev *rdev)
*/
super_types[mddev->major_version].
validate_super(mddev, NULL/*freshest*/, rdev);
- err = mddev->pers->hot_add_disk(mddev, rdev, NULL);
+ err = mddev->pers->hot_add_disk(mddev, rdev, lim);
if (err) {
md_kick_rdev_from_array(rdev);
return err;
@@ -3117,7 +3117,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len,
} else if (cmd_match(buf, "remove")) {
if (rdev->mddev->pers) {
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(rdev->mddev, rdev, NULL);
+ remove_and_add_spares(rdev->mddev, rdev, lim);
}
if (rdev->raid_disk >= 0)
err = -EBUSY;
@@ -3236,7 +3236,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len,
if (!mddev_is_clustered(rdev->mddev) ||
(err = mddev->cluster_ops->gather_bitmaps(rdev)) == 0) {
clear_bit(Faulty, &rdev->flags);
- err = add_bound_rdev(rdev);
+ err = add_bound_rdev(rdev, lim);
}
} else
err = -EBUSY;
@@ -3323,7 +3323,7 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len,
if (rdev->mddev->pers->hot_remove_disk == NULL)
return -EINVAL;
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(rdev->mddev, rdev, NULL);
+ remove_and_add_spares(rdev->mddev, rdev, lim);
if (rdev->raid_disk >= 0)
return -EBUSY;
set_bit(MD_RECOVERY_NEEDED, &rdev->mddev->recovery);
@@ -3354,7 +3354,7 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len,
clear_bit(In_sync, &rdev->flags);
clear_bit(Bitmap_sync, &rdev->flags);
err = rdev->mddev->pers->hot_add_disk(rdev->mddev, rdev,
- NULL);
+ lim);
if (err) {
rdev->raid_disk = -1;
return err;
@@ -3760,6 +3760,9 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
struct rdev_sysfs_entry *entry = container_of(attr, struct rdev_sysfs_entry, attr);
struct md_rdev *rdev = container_of(kobj, struct md_rdev, kobj);
struct kernfs_node *kn = NULL;
+ struct request_queue *q = NULL;
+ struct queue_limits lim;
+ struct queue_limits *limp = NULL;
bool suspend = false;
ssize_t rv;
struct mddev *mddev = READ_ONCE(rdev->mddev);
@@ -3780,15 +3783,37 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
suspend = true;
}
+ /*
+ * These can add a leg back, which stacks its limits; the other
+ * state_store() values never reach ->hot_add_disk(). q->limits_lock
+ * nests outside the lock and the suspend, see md_start_sync().
+ */
+ if ((entry->store == slot_store ||
+ (entry->store == state_store &&
+ (cmd_match(page, "remove") || cmd_match(page, "re-add")))) &&
+ !mddev_is_dm(mddev)) {
+ q = mddev->gendisk->queue;
+ lim = queue_limits_start_update(q);
+ limp = &lim;
+ }
+
rv = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
if (!rv) {
if (rdev->mddev == NULL)
rv = -ENODEV;
else
- rv = entry->store(rdev, page, length, NULL);
+ rv = entry->store(rdev, page, length, limp);
+ /* apply the limits before the array takes I/O again */
+ if (limp) {
+ queue_limits_commit_update(q, limp);
+ limp = NULL;
+ }
suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
}
+ if (limp)
+ queue_limits_commit_update(q, limp);
+
if (kn)
sysfs_unbreak_active_protection(kn);
@@ -7575,7 +7600,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
return 0;
}
-int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info)
+int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info,
+ struct queue_limits *lim)
{
struct md_rdev *rdev;
dev_t dev = MKDEV(info->major,info->minor);
@@ -7723,11 +7749,11 @@ int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info)
if (err)
mddev->cluster_ops->add_new_disk_cancel(mddev);
else
- err = add_bound_rdev(rdev);
+ err = add_bound_rdev(rdev, lim);
}
} else if (!err)
- err = add_bound_rdev(rdev);
+ err = add_bound_rdev(rdev, lim);
return err;
}
@@ -7780,7 +7806,8 @@ int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info)
return 0;
}
-static int hot_remove_disk(struct mddev *mddev, dev_t dev)
+static int hot_remove_disk(struct mddev *mddev, dev_t dev,
+ struct queue_limits *lim)
{
struct md_rdev *rdev;
@@ -7795,7 +7822,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
goto kick_rdev;
clear_bit(Blocked, &rdev->flags);
- remove_and_add_spares(mddev, rdev, NULL);
+ remove_and_add_spares(mddev, rdev, lim);
if (rdev->raid_disk >= 0)
goto busy;
@@ -8380,6 +8407,22 @@ static inline int md_ioctl_valid(unsigned int cmd)
}
}
+/*
+ * Commands that can reach ->hot_add_disk(). ADD_NEW_DISK only does so for a
+ * journal device or a personality without ->hot_remove_disk, but that depends
+ * on disk info still in user memory here, so it is included as a whole.
+ */
+static bool md_ioctl_may_add_disk(unsigned int cmd)
+{
+ switch (cmd) {
+ case ADD_NEW_DISK:
+ case HOT_REMOVE_DISK:
+ return true;
+ default:
+ return false;
+ }
+}
+
static bool md_ioctl_need_suspend(unsigned int cmd)
{
switch (cmd) {
@@ -8435,6 +8478,9 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
unsigned int noio_flags = 0;
void __user *argp = (void __user *)arg;
struct mddev *mddev = NULL;
+ struct request_queue *q = NULL;
+ struct queue_limits lim;
+ struct queue_limits *limp = NULL;
bool suspend;
err = md_ioctl_valid(cmd);
@@ -8485,11 +8531,20 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
if (!md_is_rdwr(mddev))
flush_work(&mddev->sync_work);
+ /* q->limits_lock nests outside both, see md_start_sync() */
+ if (md_ioctl_may_add_disk(cmd) && !mddev_is_dm(mddev)) {
+ q = mddev->gendisk->queue;
+ lim = queue_limits_start_update(q);
+ limp = &lim;
+ }
+
suspend = md_ioctl_need_suspend(cmd);
err = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
if (err) {
pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
err, cmd);
+ if (limp)
+ queue_limits_cancel_update(q);
goto out;
}
if (suspend)
@@ -8531,7 +8586,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
goto unlock;
case HOT_REMOVE_DISK:
- err = hot_remove_disk(mddev, new_decode_dev(arg));
+ err = hot_remove_disk(mddev, new_decode_dev(arg), limp);
goto unlock;
case ADD_NEW_DISK:
@@ -8547,7 +8602,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
/* Need to clear read-only for this */
break;
else
- err = md_add_new_disk(mddev, &info);
+ err = md_add_new_disk(mddev, &info, limp);
goto unlock;
}
break;
@@ -8585,7 +8640,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
if (copy_from_user(&info, argp, sizeof(info)))
err = -EFAULT;
else
- err = md_add_new_disk(mddev, &info);
+ err = md_add_new_disk(mddev, &info, limp);
goto unlock;
}
@@ -8618,6 +8673,9 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
err != -EINVAL)
mddev->hold_active = 0;
+ if (limp)
+ queue_limits_commit_update(q, limp);
+
if (suspend) {
memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
@@ -10346,6 +10404,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares,
static void md_start_sync(struct work_struct *ws)
{
struct mddev *mddev = container_of(ws, struct mddev, sync_work);
+ struct request_queue *q = NULL;
+ struct queue_limits lim;
+ struct queue_limits *limp = NULL;
int spares = 0;
bool suspend = false;
unsigned int noio_flags = 0;
@@ -10357,6 +10418,17 @@ static void md_start_sync(struct work_struct *ws)
*/
if ((mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
md_spares_need_change(mddev)) {
+ /*
+ * Adding a spare below stacks its limits, which needs
+ * q->limits_lock. Take it before suspending: its holder
+ * waits in blk_mq_freeze_queue() for I/O that
+ * mddev->suspended holds back, so the other order deadlocks.
+ */
+ if (!mddev_is_dm(mddev)) {
+ q = mddev->gendisk->queue;
+ lim = queue_limits_start_update(q);
+ limp = &lim;
+ }
suspend = true;
mddev_suspend(mddev, false);
noio_flags = memalloc_noio_save();
@@ -10371,6 +10443,12 @@ static void md_start_sync(struct work_struct *ws)
if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
md_spares_need_change(mddev)) {
mddev_unlock(mddev);
+ /* see above: q->limits_lock nests outside both */
+ if (!mddev_is_dm(mddev)) {
+ q = mddev->gendisk->queue;
+ lim = queue_limits_start_update(q);
+ limp = &lim;
+ }
mddev_suspend_and_lock_nointr(mddev);
suspend = true;
noio_flags = memalloc_noio_save();
@@ -10384,11 +10462,11 @@ static void md_start_sync(struct work_struct *ws)
* As we only add devices that are already in-sync, we can
* activate the spares immediately.
*/
- remove_and_add_spares(mddev, NULL, NULL);
+ remove_and_add_spares(mddev, NULL, limp);
goto not_running;
}
- if (!md_choose_sync_action(mddev, &spares, NULL))
+ if (!md_choose_sync_action(mddev, &spares, limp))
goto not_running;
if (!mddev->pers->sync_request)
@@ -10419,6 +10497,8 @@ static void md_start_sync(struct work_struct *ws)
* https://bugzilla.kernel.org/show_bug.cgi?id=218200
* Therefore, use __mddev_resume(mddev, false).
*/
+ if (limp)
+ queue_limits_commit_update(q, limp);
if (suspend) {
memalloc_noio_restore(noio_flags);
__mddev_resume(mddev, false);
@@ -10441,6 +10521,8 @@ static void md_start_sync(struct work_struct *ws)
* https://bugzilla.kernel.org/show_bug.cgi?id=218200
* Therefore, use __mddev_resume(mddev, false).
*/
+ if (limp)
+ queue_limits_commit_update(q, limp);
if (suspend) {
memalloc_noio_restore(noio_flags);
__mddev_resume(mddev, false);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 9e3bd5ab5519..ceca8cfcdcb9 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -1042,7 +1042,8 @@ struct mdu_disk_info_s;
extern int mdp_major;
void md_autostart_arrays(int part);
int md_set_array_info(struct mddev *mddev, struct mdu_array_info_s *info);
-int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info);
+int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info,
+ struct queue_limits *lim);
int do_md_run(struct mddev *mddev);
#define MDDEV_STACK_INTEGRITY (1u << 0)
int mddev_stack_rdev_limits(struct mddev *mddev, struct queue_limits *lim,
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 3faa2a94c03b..22759c631c4d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7288,6 +7288,9 @@ static ssize_t
raid5_store_skip_copy(struct mddev *mddev, const char *page, size_t len)
{
struct r5conf *conf;
+ struct request_queue *q = NULL;
+ struct queue_limits lim;
+ struct queue_limits *limp = NULL;
unsigned long new;
int err;
@@ -7297,23 +7300,33 @@ raid5_store_skip_copy(struct mddev *mddev, const char *page, size_t len)
return -EINVAL;
new = !!new;
+ /* q->limits_lock nests outside both, see md_start_sync() */
+ if (!mddev_is_dm(mddev)) {
+ q = mddev->gendisk->queue;
+ lim = queue_limits_start_update(q);
+ limp = &lim;
+ }
+
err = mddev_suspend_and_lock(mddev);
- if (err)
+ if (err) {
+ if (limp)
+ queue_limits_cancel_update(q);
return err;
+ }
conf = mddev->private;
if (!conf)
err = -ENODEV;
else if (new != conf->skip_copy) {
- struct request_queue *q = mddev->gendisk->queue;
- struct queue_limits lim = queue_limits_start_update(q);
-
conf->skip_copy = new;
- if (new)
- lim.features |= BLK_FEAT_STABLE_WRITES;
- else
- lim.features &= ~BLK_FEAT_STABLE_WRITES;
- err = queue_limits_commit_update(q, &lim);
+ if (limp) {
+ if (new)
+ limp->features |= BLK_FEAT_STABLE_WRITES;
+ else
+ limp->features &= ~BLK_FEAT_STABLE_WRITES;
+ }
}
+ if (limp)
+ err = queue_limits_commit_update(q, limp) ?: err;
mddev_unlock_and_resume(mddev);
return err ?: len;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
` (5 preceding siblings ...)
2026-09-07 13:39 ` [PATCH 6/6] md: take q->limits_lock before locking and suspending the array Jack Wang
@ 2026-09-08 11:15 ` Nilay Shroff
2026-09-08 12:09 ` Jinpu Wang
2026-09-08 17:00 ` Johannes Thumshirn
7 siblings, 1 reply; 18+ messages in thread
From: Nilay Shroff @ 2026-09-08 11:15 UTC (permalink / raw)
To: Jack Wang, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
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.
Please find the lockdep splat below for reference:
[1]
======================================================
WARNING: possible circular locking dependency detected
7.0.0+ #33 Not tainted
------------------------------------------------------
mdadm/15956 is trying to acquire lock:
c000000144f14358 (&disk->open_mutex){+.+.}-{4:4}, at: bdev_open+0x94/0x504
but task is already holding lock:
c000000122a13370 (&mddev->reconfig_mutex){+.+.}-{4:4}, at: md_ioctl+0xb18/0x1f44
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (&mddev->reconfig_mutex){+.+.}-{4:4}:
lock_acquire+0x16c/0x498
__mutex_lock+0xc8/0xf1c
md_ioctl+0xb18/0x1f44
blkdev_ioctl+0x5e8/0x1950
sys_ioctl+0x494/0x11c0
system_call_exception+0x138/0x380
system_call_vectored_common+0x15c/0x2ec
-> #1 (&q->limits_lock){+.+.}-{4:4}:
lock_acquire+0x16c/0x498
__mutex_lock+0xc8/0xf1c
sd_revalidate_disk+0xe4/0x2d74 [sd_mod]
sd_open+0x1a8/0x21c [sd_mod]
blkdev_get_whole+0x54/0x124
bdev_open+0x3cc/0x504
bdev_file_open_by_dev+0x114/0x1b4
disk_scan_partitions+0xc4/0x1d8
add_disk_fwnode+0x1ec/0x1f0
sd_probe+0x3a0/0x644 [sd_mod]
scsi_bus_probe+0x38/0x5c
really_probe+0x100/0x550
__driver_probe_device+0xb4/0x224
driver_probe_device+0x50/0x128
__driver_attach_async_helper+0x78/0x14c
async_run_entry_fn+0x5c/0x1f8
process_one_work+0x2c0/0x8d0
worker_thread+0x218/0x444
kthread+0x16c/0x1a0
start_kernel_thread+0x14/0x18
-> #0 (&disk->open_mutex){+.+.}-{4:4}:
check_prev_add+0x170/0x1248
__lock_acquire+0x17d0/0x2144
lock_acquire+0x16c/0x498
__mutex_lock+0xc8/0xf1c
bdev_open+0x94/0x504
bdev_file_open_by_dev+0x114/0x1b4
md_import_device+0x15c/0x2c8
md_add_new_disk+0x130/0x810
md_ioctl+0x1840/0x1f44
blkdev_ioctl+0x5e8/0x1950
sys_ioctl+0x494/0x11c0
system_call_exception+0x138/0x380
system_call_vectored_common+0x15c/0x2ec
other info that might help us debug this:
Chain exists of:
&disk->open_mutex --> &q->limits_lock --> &mddev->reconfig_mutex
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mddev->reconfig_mutex);
lock(&q->limits_lock);
lock(&mddev->reconfig_mutex);
lock(&disk->open_mutex);
*** DEADLOCK ***
2 locks held by mdadm/15956:
#0: c0000001629d6a18 (&q->limits_lock){+.+.}-{4:4}, at: md_ioctl+0xa74/0x1f44
#1: c000000122a13370 (&mddev->reconfig_mutex){+.+.}-{4:4}, at: md_ioctl+0xb18/0x1f44
stack backtrace:
CPU: 3 UID: 0 PID: 15956 Comm: mdadm Kdump: loaded Not tainted 7.0.0+ #33 PREEMPTLAZY
Hardware name: IBM,9105-22A Power11 (architected) 0x820200 0xf000007 of:IBM,FW1120.00 (RB1120_179) hv:phyp pSeries
Call Trace:
[c00000015919f1e0] [c000000001303538] dump_stack_lvl+0xe8/0x150 (unreliable)
[c00000015919f210] [c0000000002f204c] print_circular_bug+0x44c/0x608
[c00000015919f2c0] [c0000000002f23d4] check_noncircular+0x1cc/0x1ec
[c00000015919f390] [c0000000002f3f9c] check_prev_add+0x170/0x1248
[c00000015919f450] [c0000000002f876c] __lock_acquire+0x17d0/0x2144
[c00000015919f580] [c0000000002f9f4c] lock_acquire+0x16c/0x498
[c00000015919f680] [c000000001371fcc] __mutex_lock+0xc8/0xf1c
[c00000015919f7d0] [c0000000009de664] bdev_open+0x94/0x504
[c00000015919f850] [c0000000009debe8] bdev_file_open_by_dev+0x114/0x1b4
[c00000015919f8a0] [c000000000e539a8] md_import_device+0x15c/0x2c8
[c00000015919f930] [c000000000e62718] md_add_new_disk+0x130/0x810
[c00000015919f9c0] [c000000000e6a3a0] md_ioctl+0x1840/0x1f44
[c00000015919fc30] [c000000000a185e4] blkdev_ioctl+0x5e8/0x1950
[c00000015919fd10] [c000000000823274] sys_ioctl+0x494/0x11c0
[c00000015919fe10] [c0000000000318d8] system_call_exception+0x138/0x380
[c00000015919fe50] [c00000000000cedc] system_call_vectored_common+0x15c/0x2ec
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 11:15 ` [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Nilay Shroff
@ 2026-09-08 12:09 ` Jinpu Wang
2026-09-08 18:25 ` Nilay Shroff
2026-09-08 18:37 ` Abd-Alrhman Masalkhi
0 siblings, 2 replies; 18+ messages in thread
From: Jinpu Wang @ 2026-09-08 12:09 UTC (permalink / raw)
To: Nilay Shroff
Cc: abd.masalkhi, linux-raid, linux-block, Song Liu, Jens Axboe,
Christoph Hellwig, Damien Le Moal, Yu Kuai, tom.leiming
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.
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
` (6 preceding siblings ...)
2026-09-08 11:15 ` [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Nilay Shroff
@ 2026-09-08 17:00 ` Johannes Thumshirn
2026-09-09 6:30 ` Christoph Hellwig
7 siblings, 1 reply; 18+ messages in thread
From: Johannes Thumshirn @ 2026-09-08 17:00 UTC (permalink / raw)
To: Jack Wang, Nilay Shroff, abd.masalkhi
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming, Jack Wang
On 9/7/26 3:39 PM, Jack Wang wrote:
> 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
This sounds like something that is worth a blktest, isn't it?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 12:09 ` Jinpu Wang
@ 2026-09-08 18:25 ` Nilay Shroff
2026-09-09 4:25 ` Jinpu Wang
2026-09-08 18:37 ` Abd-Alrhman Masalkhi
1 sibling, 1 reply; 18+ messages in thread
From: Nilay Shroff @ 2026-09-08 18:25 UTC (permalink / raw)
To: Jinpu Wang
Cc: abd.masalkhi, linux-raid, linux-block, Song Liu, Jens Axboe,
Christoph Hellwig, Damien Le Moal, Yu Kuai, tom.leiming
On 9/8/26 5:39 PM, 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().
>
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.
> 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.
Thanks,
--Nilay
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 12:09 ` Jinpu Wang
2026-09-08 18:25 ` Nilay Shroff
@ 2026-09-08 18:37 ` Abd-Alrhman Masalkhi
2026-09-08 21:51 ` Abd-Alrhman Masalkhi
1 sibling, 1 reply; 18+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-09-08 18:37 UTC (permalink / raw)
To: Jinpu Wang, Nilay Shroff
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 18:37 ` Abd-Alrhman Masalkhi
@ 2026-09-08 21:51 ` Abd-Alrhman Masalkhi
2026-09-09 4:28 ` Jinpu Wang
0 siblings, 1 reply; 18+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-09-08 21:51 UTC (permalink / raw)
To: Jinpu Wang, Nilay Shroff
Cc: linux-raid, linux-block, Song Liu, Jens Axboe, Christoph Hellwig,
Damien Le Moal, Yu Kuai, tom.leiming
On Tue, Sep 08, 2026 at 20:37 +0200, Abd-Alrhman Masalkhi wrote:
> 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.
>
Sorry, changing the order would complicate it and result in a deadlock.
In those cases, normal I/O can be waiting for a superblock update which
needs to acquire the reconfig_mutex. At the same time, the task adding
the new rdev would be holding the reconfig_mutex while waiting for that
exact I/O to drain.
>> 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
--
Best Regards,
Abd-Alrhman
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 18:25 ` Nilay Shroff
@ 2026-09-09 4:25 ` Jinpu Wang
0 siblings, 0 replies; 18+ messages in thread
From: Jinpu Wang @ 2026-09-09 4:25 UTC (permalink / raw)
To: Nilay Shroff
Cc: abd.masalkhi, linux-raid, linux-block, Song Liu, Jens Axboe,
Christoph Hellwig, Damien Le Moal, Yu Kuai, tom.leiming
On Tue, Sep 8, 2026 at 8:25 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>
> On 9/8/26 5:39 PM, 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().
> >
>
> 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.
>
> > 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 21:51 ` Abd-Alrhman Masalkhi
@ 2026-09-09 4:28 ` Jinpu Wang
0 siblings, 0 replies; 18+ messages in thread
From: Jinpu Wang @ 2026-09-09 4:28 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi
Cc: Nilay Shroff, linux-raid, linux-block, Song Liu, Jens Axboe,
Christoph Hellwig, Damien Le Moal, Yu Kuai, tom.leiming
On Tue, Sep 8, 2026 at 11:51 PM Abd-Alrhman Masalkhi
<abd.masalkhi@gmail.com> wrote:
>
> On Tue, Sep 08, 2026 at 20:37 +0200, Abd-Alrhman Masalkhi wrote:
> > 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.
> >
>
> Sorry, changing the order would complicate it and result in a deadlock.
> In those cases, normal I/O can be waiting for a superblock update which
> needs to acquire the reconfig_mutex. At the same time, the task adding
> the new rdev would be holding the reconfig_mutex while waiting for that
> exact I/O to drain.
Hi Abd-Alrhman,
Yes, there are different deadlock scenarios.
Thank you for your input.
>
> >> 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
>
> --
> Best Regards,
> Abd-Alrhman
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] block: add queue_limits_start_update_trylock()
2026-09-07 13:39 ` [PATCH 1/6] block: add queue_limits_start_update_trylock() Jack Wang
@ 2026-09-09 6:29 ` Christoph Hellwig
2026-09-09 10:35 ` Jinpu Wang
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2026-09-09 6:29 UTC (permalink / raw)
To: Jack Wang
Cc: Nilay Shroff, abd.masalkhi, linux-raid, linux-block, Song Liu,
Jens Axboe, Christoph Hellwig, Damien Le Moal, Yu Kuai,
tom.leiming, Jack Wang
On Mon, Sep 07, 2026 at 03:39:24PM +0200, Jack Wang wrote:
> From: Jack Wang <jinpu.wang@cloud.ionos.com>
>
> Some callers must not wait for q->limits_lock, because they hold
> something the current holder waits for. md is one: its
> check_sb_changes() runs with reconfig_mutex held, while a
> queue_attr_store() holding limits_lock waits in blk_mq_freeze_queue()
> for I/O that can be waiting for a superblock update needing that mutex.
>
> Add a trylock variant of queue_limits_start_update() for them.
I don't think this is a good idea, please fix the lock ordering
instead.
> Assisted-by: Claude:claude-opus-5
But if not please try to come up with helpers like this based on
your own. We don't really need this amount of AI slop.
> + * Like queue_limits_start_update(), but fails instead of waiting when another
> + * update is in flight. For callers that must not block on q->limits_lock
> + * because they hold something its current owner is waiting for.
> + *
> + * Context: process context.
> + */
> +static inline bool
> +queue_limits_start_update_trylock(struct request_queue *q,
> + struct queue_limits *lim)
> + __cond_acquires(true, &q->limits_lock)
> +{
> + if (!mutex_trylock(&q->limits_lock))
> + return false;
> +
> + *lim = q->limits;
> +
> + return true;
> +}
> +
> int queue_limits_commit_update_frozen(struct request_queue *q,
> struct queue_limits *lim) __releases(&q->limits_lock);
> int queue_limits_commit_update(struct request_queue *q,
> --
> 2.43.0
---end quoted text---
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O
2026-09-08 17:00 ` Johannes Thumshirn
@ 2026-09-09 6:30 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2026-09-09 6:30 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Jack Wang, Nilay Shroff, abd.masalkhi, linux-raid, linux-block,
Song Liu, Jens Axboe, Christoph Hellwig, Damien Le Moal, Yu Kuai,
tom.leiming, Jack Wang
On Tue, Sep 08, 2026 at 07:00:55PM +0200, Johannes Thumshirn wrote:
>> 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
>
> This sounds like something that is worth a blktest, isn't it?
Yes. Or the mdadm testsuite, although that one drіves me crazy.
Migrating those tests to blktests might be a nice little side
project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] block: add queue_limits_start_update_trylock()
2026-09-09 6:29 ` Christoph Hellwig
@ 2026-09-09 10:35 ` Jinpu Wang
0 siblings, 0 replies; 18+ messages in thread
From: Jinpu Wang @ 2026-09-09 10:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Nilay Shroff, abd.masalkhi, linux-raid, linux-block, Song Liu,
Jens Axboe, Damien Le Moal, Yu Kuai, tom.leiming
On Wed, Sep 9, 2026 at 8:29 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Sep 07, 2026 at 03:39:24PM +0200, Jack Wang wrote:
> > From: Jack Wang <jinpu.wang@cloud.ionos.com>
> >
> > Some callers must not wait for q->limits_lock, because they hold
> > something the current holder waits for. md is one: its
> > check_sb_changes() runs with reconfig_mutex held, while a
> > queue_attr_store() holding limits_lock waits in blk_mq_freeze_queue()
> > for I/O that can be waiting for a superblock update needing that mutex.
> >
> > Add a trylock variant of queue_limits_start_update() for them.
>
> I don't think this is a good idea, please fix the lock ordering
> instead.
Fair enough, I will drop the trylock.
The ordering fix is the rest of the series: the callers that add a new
leg take q->limits_lock before reconfig_mutex and the suspend.
>
> > Assisted-by: Claude:claude-opus-5
>
> But if not please try to come up with helpers like this based on
> your own. We don't really need this amount of AI slop.
Noted.
Thanks!
>
> > + * Like queue_limits_start_update(), but fails instead of waiting when another
> > + * update is in flight. For callers that must not block on q->limits_lock
> > + * because they hold something its current owner is waiting for.
> > + *
> > + * Context: process context.
> > + */
> > +static inline bool
> > +queue_limits_start_update_trylock(struct request_queue *q,
> > + struct queue_limits *lim)
> > + __cond_acquires(true, &q->limits_lock)
> > +{
> > + if (!mutex_trylock(&q->limits_lock))
> > + return false;
> > +
> > + *lim = q->limits;
> > +
> > + return true;
> > +}
> > +
> > int queue_limits_commit_update_frozen(struct request_queue *q,
> > struct queue_limits *lim) __releases(&q->limits_lock);
> > int queue_limits_commit_update(struct request_queue *q,
> > --
> > 2.43.0
> ---end quoted text---
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-09-09 10:36 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 13:39 [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Jack Wang
2026-09-07 13:39 ` [PATCH 1/6] block: add queue_limits_start_update_trylock() Jack Wang
2026-09-09 6:29 ` Christoph Hellwig
2026-09-09 10:35 ` Jinpu Wang
2026-09-07 13:39 ` [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk() Jack Wang
2026-09-07 13:39 ` [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes() Jack Wang
2026-09-07 13:39 ` [PATCH 4/6] md: pass a queue_limits through the rdev sysfs stores Jack Wang
2026-09-07 13:39 ` [PATCH 5/6] md: don't wait for q->limits_lock in mddev_update_io_opt() Jack Wang
2026-09-07 13:39 ` [PATCH 6/6] md: take q->limits_lock before locking and suspending the array Jack Wang
2026-09-08 11:15 ` [PATCH 0/6] md: don't wait for q->limits_lock while md holds back I/O Nilay Shroff
2026-09-08 12:09 ` Jinpu Wang
2026-09-08 18:25 ` Nilay Shroff
2026-09-09 4:25 ` Jinpu Wang
2026-09-08 18:37 ` Abd-Alrhman Masalkhi
2026-09-08 21:51 ` Abd-Alrhman Masalkhi
2026-09-09 4:28 ` Jinpu Wang
2026-09-08 17:00 ` Johannes Thumshirn
2026-09-09 6:30 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox