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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing queue limit stacking for hot-added disks on cluster peer nodes leaves the array with potentially invalid queue limits.
--

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

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

This commit modifies check_sb_changes() to bypass queue limit stacking for
a newly activated spare by passing MDDEV_STACK_SKIP to remove_and_add_spares().
This is intended to prevent deadlocks when reconfig_mutex is held.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 28fc903ffeea3..e60dc2c7eb90e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10885,7 +10885,14 @@ 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 q->limits_lock
> +				 * cannot be taken here.  The device is
> +				 * already a member, its limits are stacked,
> +				 * so add it without touching them.
> +				 */
> +				ret = remove_and_add_spares(mddev, rdev2,
> +							    MDDEV_STACK_SKIP);

[Severity: High]
Does this code permanently bypass limit stacking for disks hot-added on
cluster peer nodes?

In a clustered MD array, when a remote node adds a new disk, the local peer
processes the NEWDISK message via md_add_new_disk() and add_bound_rdev().
Because personalities like raid1 and raid10 implement hot_remove_disk,
add_bound_rdev() intentionally skips calling hot_add_disk(), meaning the new
disk's limits are not stacked at that time.

When the remote node later activates the disk and sends a METADATA_UPDATED
message, this check_sb_changes() code handles it. By passing
MDDEV_STACK_SKIP, the personality's hot_add_disk() detects the flag and
bypasses mddev_stack_new_rdev():

drivers/md/raid1.c:raid1_add_disk() {
    ...
    if (lim == MDDEV_STACK_SKIP)
        err = 0;
    else if (lim)
        err = mddev_stack_rdev_into(mddev, rdev, lim);
    ...
}

Since the disk is marked with raid_disk >= 0, the sync thread will ignore it,
so it seems its limits will never be stacked. Can this leave the array with
invalid queue limits, potentially causing the local block layer to submit
incompatible I/O to the newly activated disk?

Also, the commit message states "The device is already a member, so its
limits are stacked", which appears to be inaccurate for Candidate disks
added remotely. I noticed that none of the follow-up patches in this series
correct this missing limits update.

>  				pr_info("Activated spare: %pg\n",
>  					rdev2->bdev);
>  				/* wakeup mddev->thread here, so array could

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