Re: [PATCH v2 1/8] md: pass a queue_limits down to ->hot_add_disk()

[Nilay Shroff <nilay@linux.ibm.com>]

On 9/10/26 1:41 PM, Jack Wang wrote:
> 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 holding reconfig_mutex or a
> suspended array cannot allow that, and must own the update instead.
> 
> Give ->hot_add_disk(), remove_and_add_spares() and
> md_choose_sync_action() a struct queue_limits argument with three
> states: an update to stack into, NULL to let the personality take the
> lock as before, or MDDEV_STACK_SKIP to add the leg without touching the
> limits, for callers that can do neither.  mddev_stack_rdev_into() stacks
> into a caller-owned update without the lock.
> 
> Every caller still passes NULL and nothing passes the sentinel yet, so
> there is no functional change; the users follow.
> 
> Assisted-by: LLM
> Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> ---
>   drivers/md/dm-raid.c   |  2 +-
>   drivers/md/md-linear.c | 28 ++++++++++++++----
>   drivers/md/md.c        | 66 ++++++++++++++++++++++++++++++++----------
>   drivers/md/md.h        | 11 ++++++-
>   drivers/md/raid1.c     | 10 +++++--
>   drivers/md/raid10.c    | 19 +++++++++---
>   drivers/md/raid5.c     |  5 ++--
>   7 files changed, 110 insertions(+), 31 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..da82c313d459 100644
> --- a/drivers/md/md-linear.c
> +++ b/drivers/md/md-linear.c
> @@ -65,11 +65,16 @@ static sector_t linear_size(struct mddev *mddev, sector_t sectors, int raid_disk
>   	return array_sectors;
>   }
>   
> -static int linear_set_limits(struct mddev *mddev)
> +static int linear_set_limits(struct mddev *mddev,
> +			     struct queue_limits *caller_lim)
>   {
>   	struct queue_limits lim;
>   	int err;
>   
> +	/* the caller can neither stack nor take q->limits_lock */
> +	if (caller_lim == MDDEV_STACK_SKIP)
> +		return 0;
> +
>   	md_init_stacking_limits(&lim);
>   	lim.features |= BLK_FEAT_NOWAIT;
>   	lim.max_hw_sectors = mddev->chunk_sectors;
> @@ -82,10 +87,20 @@ static int linear_set_limits(struct mddev *mddev)
>   	if (err)
>   		return err;
>   
> +	/*
> +	 * The caller owns an update and commits it itself; taking
> +	 * q->limits_lock here would take it a second time.
> +	 */
> +	if (caller_lim) {
> +		*caller_lim = lim;
> +		return 0;
> +	}
> +
>   	return queue_limits_set(mddev->gendisk->queue, &lim);
>   }
> 
This looks overly complicated with three different cases where
linear_set_limits() either ignores the limits update, updates the limits
provided by the caller without committing them, or updates and commits the
limits itself.

Why can't we instead have the callers always pass a struct queue_limits
pointer, and make linear_set_limits() only update the limits provided by
its caller without committing them?

The caller can then decide what to do with the resulting limits: either
ignore them or commit them as appropriate. This also avoids introducing
MDDEV_STACK_SKIP as a special sentinel value.

In this model, linear_set_limits() would only be responsible for preparing
the limits. This also keeps the locking and limits-commit logic in one common
place. The caller is then responsible for acquiring the appropriate locks and
committing the limits in the correct order for its particular context.

So the core logic is: personality should describe what the limits need to
become and the MD core/caller should decide when those limits become visible.

[...]

>   
> +/*
> + * 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);
> +
This API is correctly moving in that direction which I proposed above.
But rather than adding new API, I'd update mddev_stack_new_rdev() (or
rename it to mddev_stack_rdev_into()) which would stack the rdev into
the caller-provided struct queue_limits without taking q->limits_lock
or committing the limits.

[...]
> +/*
> + * Sentinel for the queue_limits argument of ->hot_add_disk().  The caller has
> + * no update to stack into and must not take q->limits_lock itself, so the leg
> + * is added with the array's current limits.
> + */
> +#define MDDEV_STACK_SKIP	((struct queue_limits *)ERR_PTR(-EAGAIN))

If we follow the design as I suggested above then we can get away with
above sentinel.

[...]


> -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,12 @@ 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 == MDDEV_STACK_SKIP)
> +				err = 0;
> +			else if (lim)
> +				err = mddev_stack_rdev_into(mddev, rdev, lim);
> +			else
> +				err = mddev_stack_new_rdev(mddev, rdev);
>   			if (err)
>   				return err;
>   
Here as well the same comment as linear_set_limits().

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 1093c798d9dd..222bd7badcff 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,12 @@ static int raid10_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>   			continue;
>   		}
>   
> -		err = mddev_stack_new_rdev(mddev, rdev);
> +		if (lim == MDDEV_STACK_SKIP)
> +			err = 0;
> +		else 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 +2153,12 @@ 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 == MDDEV_STACK_SKIP)
> +			err = 0;
> +		else 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;

Again same comment as linear_set_limits().

Thanks,
--Nilay