Re: [PATCH v2 6/8] md: pass a queue_limits through ->run()

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

> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index 35e103f0c2c3..59141e4299a8 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -379,7 +379,8 @@ static void raid0_free(struct mddev *mddev, void *priv)
>   	kfree(conf);
>   }
>   
> -static int raid0_set_limits(struct mddev *mddev)
> +static int raid0_set_limits(struct mddev *mddev,
> +			    struct queue_limits *caller_lim)
>   {
>   	struct queue_limits lim;
>   	int err;
> @@ -398,10 +399,19 @@ static int raid0_set_limits(struct mddev *mddev)
>   	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>   	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);
>   }
>   
[...]

> -static int raid1_set_limits(struct mddev *mddev)
> +static int raid1_set_limits(struct mddev *mddev,
> +			    struct queue_limits *caller_lim)
>   {
>   	struct queue_limits lim;
>   	int err;
> @@ -3185,10 +3186,19 @@ static int raid1_set_limits(struct mddev *mddev)
>   	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>   	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);
>   }
>   

[...]

>   
> -static int raid10_set_queue_limits(struct mddev *mddev)
> +static int raid10_set_queue_limits(struct mddev *mddev,
> +				   struct queue_limits *caller_lim)
>   {
>   	struct r10conf *conf = mddev->private;
>   	struct queue_limits lim;
> @@ -3948,10 +3949,19 @@ static int raid10_set_queue_limits(struct mddev *mddev)
>   	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>   	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);
>   }
>   

[...]

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 22759c631c4d..28bd81de86c1 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7944,7 +7944,8 @@ static int raid5_create_ctx_pool(struct r5conf *conf)
>   	return conf->ctx_pool ? 0 : -ENOMEM;
>   }
>   
> -static int raid5_set_limits(struct mddev *mddev)
> +static int raid5_set_limits(struct mddev *mddev,
> +			    struct queue_limits *caller_lim)
>   {
>   	struct r5conf *conf = mddev->private;
>   	struct queue_limits lim;
> @@ -7996,10 +7997,19 @@ static int raid5_set_limits(struct mddev *mddev)
>   	/* No restrictions on the number of segments in the request */
>   	lim.max_segments = USHRT_MAX;
>   
> +	/*
> +	 * 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);
>   }
>   
[...]
I'd propose the same changes as I suggested in patch 1/8, for
raid5_set_limits(), raid10_set_queue_limits(), raid1_set_limits()
and raid0_set_limits().

In particular, I think these functions should always operate on
a caller-provided struct queue_limits and only prepare/update the
limits, without deciding whether to commit them. The caller should
own the limits update and commit it as appropriate for its locking
context.

Thanks,
--Nilay