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

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

    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.

quoted

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?

[ ... ]

quoted

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?

[ ... ]

quoted

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