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

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

[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

[PATCH 1/6] block: add queue_limits_start_update_trylock()

[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

[PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk()

[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

[PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes()

[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

[PATCH 4/6] md: pass a queue_limits through the rdev sysfs stores

[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

[PATCH 5/6] md: don't wait for q->limits_lock in mddev_update_io_opt()

[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

[PATCH 6/6] md: take q->limits_lock before locking and suspending the array

[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

Re: [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] `linear_add()` ignores the new `lim` argument and unconditionally calls `queue_limits_set()`, which will cause a deadlock.

Pre-existing issues:
- [High] Missing limit check and queue limit stacking for replacement disks in `raid1_add_disk()`.
- [High] Missing limits update and stacking when hot-adding a disk in `raid5_add_disk()`.
--

commit 37bd7159af00deb7b9d45549d34775a10b587170
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

    md: pass a queue_limits down to ->hot_add_disk()
    
    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.

> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> index 73b367b61b873..1f44c2d7db93d 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

[Severity: High]
Does linear_add() miss using the new lim argument?

Looking at the body of linear_add(), it unconditionally calls linear_conf() 
without passing the limits down. In the final state of this patch series, when 
the caller already holds q->limits_lock, linear_conf() will eventually call 
queue_limits_set() which tries to take the lock again. Could this lead to a 
deadlock on q->limits_lock when hot-adding a disk?

[ ... ]
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f0646fb24371f..dd348b5695d06 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;

[Severity: High]
This is a pre-existing issue, but I noticed that further down in 
raid1_add_disk(), we add a replacement disk but omit any limit checks:

	if (err && repl_slot >= 0) {
		/* Add this device as a replacement */
		clear_bit(In_sync, &rdev->flags);
		set_bit(Replacement, &rdev->flags);
		raid1_add_conf(conf, rdev, repl_slot, true);
		err = 0;
		conf->fullsync = 1;
	}

Since mddev_stack_new_rdev() or mddev_stack_rdev_into() is skipped for this 
path, could this allow a replacement disk with incompatible queue limits 
(such as differing integrity profiles or larger logical block sizes) to join 
the array, resulting in data corruption or I/O failures?

[ ... ]
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..0ec555ada64a0 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;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that raid5_add_disk()
does not stack or check queue limits at all when hot-adding a disk:

	for (disk = first; disk <= last; disk++) {
		p = conf->disks + disk;
		if (p->rdev == NULL) {
			clear_bit(In_sync, &rdev->flags);
			rdev->raid_disk = disk;
			if (rdev->saved_raid_disk != disk)
				conf->fullsync = 1;
			WRITE_ONCE(p->rdev, rdev);

			err = log_modify(conf, rdev, true);

			goto out;
		}
	}

Because mddev_stack_new_rdev() or mddev_stack_rdev_into() are not called
here, and the new lim argument is ignored, could this allow disks with
incompatible limits to be successfully added to the array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=2

Re: [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The return value of queue_limits_commit_update() is ignored, allowing a disk to be permanently added to the array even if its queue limits are rejected.
--

commit 9dfb5955d3bac2bdadfd9671f084d71c343b76e2
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

md: don't wait for q->limits_lock in check_sb_changes()

This patch updates check_sb_changes() to use a non-blocking trylock when
stacking queue limits for a newly added spare. If the lock is acquired, the
limits are stacked inline to avoid blocking on I/O while holding the
reconfig_mutex.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -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);

[Severity: High]
Is it safe to ignore the return value of queue_limits_commit_update() here?

If blk_validate_limits() fails inside queue_limits_commit_update(), it returns
an error and the limits are not applied. Because the return value is ignored
here, the disk could still be permanently added to the array even if its queue
limits (such as incompatible integrity profiles or alignments) were rejected.

Previously, a failure when committing limits would abort the disk addition
(for example, in mddev_stack_new_rdev()). Does splitting the limit stacking
from the commit and dropping the return value bypass this safety check?

> +				} 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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=3

Re: [PATCH 6/6] md: take q->limits_lock before locking and suspending the array

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] queue_limits_commit_update() return value is ignored and called unconditionally on error paths instead of queue_limits_cancel_update()

Pre-existing issues:
- [High] ABBA deadlock between q->limits_lock and mddev_suspend in level_store()
--

commit ecb6e260e45c52011fdf103645ad33814409ccd9
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

md: take q->limits_lock before locking and suspending the array

This commit updates the block layer limits locking order for MD devices to
prevent a deadlock when writing queue limits attributes during disk operations.
It takes the limits update before locking and suspending the array, passing it
down into personality stacks.

> 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.

[Severity: High]
This is a pre-existing issue, but is there a plan to address the ABBA
deadlock in level_store()? When the array is suspended and locked, normal
I/O parks on the queue usage counter. Concurrently, a udev sysfs write can
take q->limits_lock and wait in blk_mq_freeze_queue() for the counter to
drain, while level_store() holds the suspend lock and waits for
q->limits_lock via pers->run() -> queue_limits_set(), causing a system
hang.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3780,15 +3783,37 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
[ ... ]
>  	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;
> +		}

[Severity: Medium]
Are we missing a check for the return value of queue_limits_commit_update()
here? If the block layer rejects the new limits because blk_validate_limits()
fails, the return value is ignored. This could allow the array to proceed
using the new disk with old limits, risking I/O errors if requests exceed
the disk's hardware limits.

>  		suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
>  	}
>  
> +	if (limp)
> +		queue_limits_commit_update(q, limp);
> +

[Severity: Medium]
Does this code incorrectly call queue_limits_commit_update() unconditionally
on error paths? If mddev_suspend_and_lock() or mddev_lock() failed, rv will
be non-zero but limp will still be set. In this situation, the code will
commit unchanged limits instead of cancelling them via
queue_limits_cancel_update().

[ ... ]
> @@ -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);
> +

[Severity: Medium]
Similar to the rdev_attr_store() issue above, should this use
queue_limits_cancel_update() on error paths? If an inner ioctl command
fails, this will erroneously commit the unchanged limits rather than
cancelling the update.

[ ... ]
> @@ -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);

[Severity: Medium]
Does this safely handle limit validation failures when adding spares? The
return value is ignored, which could leave the array with outdated limits if
the limits fail to stack correctly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=6

Re: [PATCH 3/6] md: don't wait for q->limits_lock in check_sb_changes()

[Jinpu Wang]

On Mon, Sep 7, 2026 at 3:56 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The return value of queue_limits_commit_update() is ignored, allowing a disk to be permanently added to the array even if its queue limits are rejected.
will fix in newer version
> --
>
> commit 9dfb5955d3bac2bdadfd9671f084d71c343b76e2
> Author: Jack Wang <jinpu.wang@cloud.ionos.com>
>
> md: don't wait for q->limits_lock in check_sb_changes()
>
> This patch updates check_sb_changes() to use a non-blocking trylock when
> stacking queue limits for a newly added spare. If the lock is acquired, the
> limits are stacked inline to avoid blocking on I/O while holding the
> reconfig_mutex.
>
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> [ ... ]
> > @@ -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);
>
> [Severity: High]
> Is it safe to ignore the return value of queue_limits_commit_update() here?
>
> If blk_validate_limits() fails inside queue_limits_commit_update(), it returns
> an error and the limits are not applied. Because the return value is ignored
> here, the disk could still be permanently added to the array even if its queue
> limits (such as incompatible integrity profiles or alignments) were rejected.
>
> Previously, a failure when committing limits would abort the disk addition
> (for example, in mddev_stack_new_rdev()). Does splitting the limit stacking
> from the commit and dropping the return value bypass this safety check?
>
> > +                             } 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);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=3

Re: [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk()

[Jinpu Wang]

On Mon, Sep 7, 2026 at 3:56 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] `linear_add()` ignores the new `lim` argument and unconditionally calls `queue_limits_set()`, which will cause a deadlock.

Will fix in newer version.
>
> Pre-existing issues:
> - [High] Missing limit check and queue limit stacking for replacement disks in `raid1_add_disk()`.
> - [High] Missing limits update and stacking when hot-adding a disk in `raid5_add_disk()`.
> --
>
> commit 37bd7159af00deb7b9d45549d34775a10b587170
> Author: Jack Wang <jinpu.wang@cloud.ionos.com>
>
>     md: pass a queue_limits down to ->hot_add_disk()
>
>     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.
>
> > diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> > index 73b367b61b873..1f44c2d7db93d 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
>
> [Severity: High]
> Does linear_add() miss using the new lim argument?
>
> Looking at the body of linear_add(), it unconditionally calls linear_conf()
> without passing the limits down. In the final state of this patch series, when
> the caller already holds q->limits_lock, linear_conf() will eventually call
> queue_limits_set() which tries to take the lock again. Could this lead to a
> deadlock on q->limits_lock when hot-adding a disk?
>
> [ ... ]
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index f0646fb24371f..dd348b5695d06 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;
>
> [Severity: High]
> This is a pre-existing issue, but I noticed that further down in
> raid1_add_disk(), we add a replacement disk but omit any limit checks:
>
>         if (err && repl_slot >= 0) {
>                 /* Add this device as a replacement */
>                 clear_bit(In_sync, &rdev->flags);
>                 set_bit(Replacement, &rdev->flags);
>                 raid1_add_conf(conf, rdev, repl_slot, true);
>                 err = 0;
>                 conf->fullsync = 1;
>         }
>
> Since mddev_stack_new_rdev() or mddev_stack_rdev_into() is skipped for this
> path, could this allow a replacement disk with incompatible queue limits
> (such as differing integrity profiles or larger logical block sizes) to join
> the array, resulting in data corruption or I/O failures?
>
> [ ... ]
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index b91545ce090de..0ec555ada64a0 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;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but I noticed that raid5_add_disk()
> does not stack or check queue limits at all when hot-adding a disk:
>
>         for (disk = first; disk <= last; disk++) {
>                 p = conf->disks + disk;
>                 if (p->rdev == NULL) {
>                         clear_bit(In_sync, &rdev->flags);
>                         rdev->raid_disk = disk;
>                         if (rdev->saved_raid_disk != disk)
>                                 conf->fullsync = 1;
>                         WRITE_ONCE(p->rdev, rdev);
>
>                         err = log_modify(conf, rdev, true);
>
>                         goto out;
>                 }
>         }
>
> Because mddev_stack_new_rdev() or mddev_stack_rdev_into() are not called
> here, and the new lim argument is ignored, could this allow disks with
> incompatible limits to be successfully added to the array?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=2

Re: [PATCH 6/6] md: take q->limits_lock before locking and suspending the array

[Jinpu Wang]

On Mon, Sep 7, 2026 at 4:00 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] queue_limits_commit_update() return value is ignored and called unconditionally on error paths instead of queue_limits_cancel_update()
>
Will fix in newer version
> Pre-existing issues:
> - [High] ABBA deadlock between q->limits_lock and mddev_suspend in level_store()

Yes, there is a plan: thread a queue_limits through ->run() in all
five personalities so level_store() owns the update, the way
md_ioctl() now does. I left it out because it's ~40 lines across
raid0/1/5/10/linear and orthogonal to the deadlock being fixed here.
I'll post it as a follow-up. Worth noting the hang you describe is
reachable today without this series, since queue_limits_set() freezes
the queue while level_store() has the array suspended — the freeze
then waits for I/O that mddev->suspended is holding back, no second
task needed.
> --
>
> commit ecb6e260e45c52011fdf103645ad33814409ccd9
> Author: Jack Wang <jinpu.wang@cloud.ionos.com>
>
> md: take q->limits_lock before locking and suspending the array
>
> This commit updates the block layer limits locking order for MD devices to
> prevent a deadlock when writing queue limits attributes during disk operations.
> It takes the limits update before locking and suspending the array, passing it
> down into personality stacks.
>
> > 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.
>
> [Severity: High]
> This is a pre-existing issue, but is there a plan to address the ABBA
> deadlock in level_store()? When the array is suspended and locked, normal
> I/O parks on the queue usage counter. Concurrently, a udev sysfs write can
> take q->limits_lock and wait in blk_mq_freeze_queue() for the counter to
> drain, while level_store() holds the suspend lock and waits for
> q->limits_lock via pers->run() -> queue_limits_set(), causing a system
> hang.
>
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -3780,15 +3783,37 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
> [ ... ]
> >       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;
> > +             }
>
> [Severity: Medium]
> Are we missing a check for the return value of queue_limits_commit_update()
> here? If the block layer rejects the new limits because blk_validate_limits()
> fails, the return value is ignored. This could allow the array to proceed
> using the new disk with old limits, risking I/O errors if requests exceed
> the disk's hardware limits.
>
> >               suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
> >       }
> >
> > +     if (limp)
> > +             queue_limits_commit_update(q, limp);
> > +
>
> [Severity: Medium]
> Does this code incorrectly call queue_limits_commit_update() unconditionally
> on error paths? If mddev_suspend_and_lock() or mddev_lock() failed, rv will
> be non-zero but limp will still be set. In this situation, the code will
> commit unchanged limits instead of cancelling them via
> queue_limits_cancel_update().
>
> [ ... ]
> > @@ -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);
> > +
>
> [Severity: Medium]
> Similar to the rdev_attr_store() issue above, should this use
> queue_limits_cancel_update() on error paths? If an inner ioctl command
> fails, this will erroneously commit the unchanged limits rather than
> cancelling the update.
>
> [ ... ]
> > @@ -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);
>
> [Severity: Medium]
> Does this safely handle limit validation failures when adding spares? The
> return value is ignored, which could leave the array with outdated limits if
> the limits fail to stack correctly.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907133929.1081540-1-jinpu.wang@ionos.com?part=6

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

[Nilay Shroff]

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

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

[Jinpu Wang]

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

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

[Johannes Thumshirn]

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?

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

[Nilay Shroff]

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

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

[Abd-Alrhman Masalkhi]

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

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

[Abd-Alrhman Masalkhi]

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

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

[Jinpu Wang]

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.

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

[Jinpu Wang]

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

Re: [PATCH 1/6] block: add queue_limits_start_update_trylock()

[Christoph Hellwig]

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---

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

[Christoph Hellwig]

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.

Re: [PATCH 1/6] block: add queue_limits_start_update_trylock()

[Jinpu Wang]

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---