When testing if a previous stripe has had reshape expand past it, use
the earliest or latest logical sector in all the disks for that stripe
head. This will allow adding multiple disks at a time in a subesquent
patch.
To do this cleaner, refactor the check into a helper function called
stripe_ahead_of_reshape().
Signed-off-by: Logan Gunthorpe <[email protected]>
---
drivers/md/raid5.c | 55 ++++++++++++++++++++++++++++++++++------------
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 1fa82d8fa89e..40a25c4b80bd 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5823,6 +5823,42 @@ static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
return sector >= reshape_sector;
}
+static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
+ sector_t max, sector_t reshape_sector)
+{
+ if (mddev->reshape_backwards)
+ return max < reshape_sector;
+ else
+ return min >= reshape_sector;
+}
+
+static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
+ struct stripe_head *sh)
+{
+ sector_t max_sector = 0, min_sector = MaxSector;
+ bool ret = false;
+ int dd_idx;
+
+ for (dd_idx = 0; dd_idx < sh->disks; dd_idx++) {
+ if (dd_idx == sh->pd_idx)
+ continue;
+
+ min_sector = min(min_sector, sh->dev[dd_idx].sector);
+ max_sector = min(max_sector, sh->dev[dd_idx].sector);
+ }
+
+ spin_lock_irq(&conf->device_lock);
+
+ if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
+ conf->reshape_progress))
+ /* mismatch, need to try again */
+ ret = true;
+
+ spin_unlock_irq(&conf->device_lock);
+
+ return ret;
+}
+
enum stripe_result {
STRIPE_SUCCESS = 0,
STRIPE_RETRY,
@@ -5883,26 +5919,17 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
return STRIPE_FAIL;
}
- if (unlikely(previous)) {
- /* expansion might have moved on while waiting for a
- * stripe, so we must do the range check again.
+ if (unlikely(previous) &&
+ stripe_ahead_of_reshape(mddev, conf, sh)) {
+ /* Expansion moved on while waiting for a stripe.
* Expansion could still move past after this
* test, but as we are holding a reference to
* 'sh', we know that if that happens,
* STRIPE_EXPANDING will get set and the expansion
* won't proceed until we finish with the stripe.
*/
- int must_retry = 0;
- spin_lock_irq(&conf->device_lock);
- if (!ahead_of_reshape(mddev, logical_sector,
- conf->reshape_progress))
- /* mismatch, need to try again */
- must_retry = 1;
- spin_unlock_irq(&conf->device_lock);
- if (must_retry) {
- raid5_release_stripe(sh);
- return STRIPE_SCHEDULE_AND_RETRY;
- }
+ raid5_release_stripe(sh);
+ return STRIPE_SCHEDULE_AND_RETRY;
}
if (read_seqcount_retry(&conf->gen_lock, seq)) {
--
2.30.2
On Wed, Apr 20, 2022 at 01:54:24PM -0600, Logan Gunthorpe wrote:
> +static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
> + sector_t max, sector_t reshape_sector)
> +{
> + if (mddev->reshape_backwards)
> + return max < reshape_sector;
> + else
> + return min >= reshape_sector;
> +}
Nit: no need for the return.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On 4/21/22 3:54 AM, Logan Gunthorpe wrote:
> When testing if a previous stripe has had reshape expand past it, use
> the earliest or latest logical sector in all the disks for that stripe
> head. This will allow adding multiple disks at a time in a subesquent
> patch.
>
> To do this cleaner, refactor the check into a helper function called
> stripe_ahead_of_reshape().
>
> Signed-off-by: Logan Gunthorpe<[email protected]>
> ---
> drivers/md/raid5.c | 55 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 1fa82d8fa89e..40a25c4b80bd 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5823,6 +5823,42 @@ static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
> return sector >= reshape_sector;
> }
>
> +static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
> + sector_t max, sector_t reshape_sector)
> +{
> + if (mddev->reshape_backwards)
> + return max < reshape_sector;
> + else
> + return min >= reshape_sector;
> +}
> +
> +static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
> + struct stripe_head *sh)
> +{
> + sector_t max_sector = 0, min_sector = MaxSector;
> + bool ret = false;
> + int dd_idx;
> +
> + for (dd_idx = 0; dd_idx < sh->disks; dd_idx++) {
> + if (dd_idx == sh->pd_idx)
> + continue;
> +
> + min_sector = min(min_sector, sh->dev[dd_idx].sector);
> + max_sector = min(max_sector, sh->dev[dd_idx].sector);
> + }
> +
> + spin_lock_irq(&conf->device_lock);
> +
> + if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
> + conf->reshape_progress))
> + /* mismatch, need to try again */
> + ret = true;
I think we can just open code range_ahead_of_reshape.
And seems the above is not same as below original checking which compare
logical_sector with reshape_progress. Is it intentional or am I miss
something?
...
> - int must_retry = 0;
> - spin_lock_irq(&conf->device_lock);
> - if (!ahead_of_reshape(mddev, logical_sector,
> - conf->reshape_progress))
> - /* mismatch, need to try again */
> - must_retry = 1;
> - spin_unlock_irq(&conf->device_lock);
> - if (must_retry) {
> - raid5_release_stripe(sh);
> - return STRIPE_SCHEDULE_AND_RETRY;
> - }
> + raid5_release_stripe(sh);
> + return STRIPE_SCHEDULE_AND_RETRY;
> }
>
> if (read_seqcount_retry(&conf->gen_lock, seq)) {
Thanks,
Guoqing
On 2022-04-26 19:53, Guoqing Jiang wrote:
>> +static bool stripe_ahead_of_reshape(struct mddev *mddev, struct
>> r5conf *conf,
>> + struct stripe_head *sh)
>> +{
>> + sector_t max_sector = 0, min_sector = MaxSector;
>> + bool ret = false;
>> + int dd_idx;
>> +
>> + for (dd_idx = 0; dd_idx < sh->disks; dd_idx++) {
>> + if (dd_idx == sh->pd_idx)
>> + continue;
>> +
>> + min_sector = min(min_sector, sh->dev[dd_idx].sector);
>> + max_sector = min(max_sector, sh->dev[dd_idx].sector);
>> + }
>> +
>> + spin_lock_irq(&conf->device_lock);
>> +
>> + if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
>> + conf->reshape_progress))
>> + /* mismatch, need to try again */
>> + ret = true;
>
> I think we can just open code range_ahead_of_reshape.
I think this way is much easier to read. GCC is much
> And seems the above is not same as below original checking which compare
> logical_sector with reshape_progress. Is it intentional or am I miss
> something?
Yes, this was intentional, the change is necessary for the next patch to
ensure that all pages in a stripe are ahead of the reshape. This was not
intended as just a cleanup patch.
Logan