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

On Mon, Sep 7, 2026 at 4:00 PM [off-list ref] 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 [off-list ref]

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.

quoted

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.

quoted

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,

[ ... ]

quoted

      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.

quoted

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

[ ... ]

quoted

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

[ ... ]

quoted

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