diff mbox series

[RFC,V2,4/4] md/raid5: Don't check crossing reshape when reshape hasn't started

Message ID 20240220153059.11233-5-xni@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Mike Snitzer
Headers show
Series Fix regression bugs | expand

Commit Message

Xiao Ni Feb. 20, 2024, 3:30 p.m. UTC 
stripe_ahead_of_reshape is used to check if a stripe region cross the
reshape position. So first, change the function name to
stripe_across_reshape to describe the usage of this function.

For reshape backwards, it starts reshape from the end of array and conf->
reshape_progress is init to raid5_size. During reshape, if previous is true
(set in make_stripe_request) and max_sector >= conf->reshape_progress, ios
should wait until reshape window moves forward. But ios don't need to wait
if max_sector is raid5_size.

And put the conditions into the function directly to make understand the
codes easily.

This can be reproduced easily by lvm2 test shell/lvconvert-raid-reshape.sh
For dm raid reshape, before starting sync thread, it needs to reload table
some times. In one time dm raid uses MD_RECOVERY_WAIT to delay reshape and
it doesn't start sync thread this time. Then one io comes in and it waits
because stripe_ahead_of_reshape returns true because it's a backward
reshape and max_sectors > conf->reshape_progress. But the reshape hasn't
started. So skip this check when reshape_progress is raid5_size

Fixes: 486f60558607 ("md/raid5: Check all disks in a stripe_head for reshape progress")
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/raid5.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Yu Kuai Feb. 23, 2024, 3:08 a.m. UTC | #1 
Hi,

在 2024/02/20 23:30, Xiao Ni 写道:
> stripe_ahead_of_reshape is used to check if a stripe region cross the
> reshape position. So first, change the function name to
> stripe_across_reshape to describe the usage of this function.
> 
> For reshape backwards, it starts reshape from the end of array and conf->
> reshape_progress is init to raid5_size. During reshape, if previous is true
> (set in make_stripe_request) and max_sector >= conf->reshape_progress, ios
> should wait until reshape window moves forward. But ios don't need to wait
> if max_sector is raid5_size.
> 
> And put the conditions into the function directly to make understand the
> codes easily.
> 
> This can be reproduced easily by lvm2 test shell/lvconvert-raid-reshape.sh
> For dm raid reshape, before starting sync thread, it needs to reload table
> some times. In one time dm raid uses MD_RECOVERY_WAIT to delay reshape and
> it doesn't start sync thread this time. Then one io comes in and it waits
> because stripe_ahead_of_reshape returns true because it's a backward
> reshape and max_sectors > conf->reshape_progress. But the reshape hasn't
> started. So skip this check when reshape_progress is raid5_size

Like I said before, after following merged patch:

ad39c08186f8 md: Don't register sync_thread for reshape directly

You should not see that sync_thread fo reshape is registered while
MD_RECOVERY_WAIT is set, so this patch should not be needed.

Thanks,
Kuai

> 
> Fixes: 486f60558607 ("md/raid5: Check all disks in a stripe_head for reshape progress")
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   drivers/md/raid5.c | 22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8497880135ee..4c71df4e2370 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5832,17 +5832,12 @@ static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>   					  sector >= reshape_sector;
>   }
>   
> -static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
> -				   sector_t max, sector_t reshape_sector)
> -{
> -	return mddev->reshape_backwards ? max < reshape_sector :
> -					  min >= reshape_sector;
> -}
> -
> -static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
> +static sector_t raid5_size(struct mddev *mddev, sector_t sectors, int raid_disks);
> +static bool stripe_across_reshape(struct mddev *mddev, struct r5conf *conf,
>   				    struct stripe_head *sh)
>   {
>   	sector_t max_sector = 0, min_sector = MaxSector;
> +	sector_t reshape_pos = 0;
>   	bool ret = false;
>   	int dd_idx;
>   
> @@ -5856,9 +5851,12 @@ static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
>   
>   	spin_lock_irq(&conf->device_lock);
>   
> -	if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
> -				     conf->reshape_progress))
> -		/* mismatch, need to try again */
> +	reshape_pos = conf->reshape_progress;
> +	if (mddev->reshape_backwards) {
> +		if (max_sector >= reshape_pos &&
> +		    reshape_pos != raid5_size(mddev, 0, 0))
> +			ret = true;
> +	} else if (min_sector < reshape_pos)
>   		ret = true;
>   
>   	spin_unlock_irq(&conf->device_lock);
> @@ -5969,7 +5967,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
>   	}
>   
>   	if (unlikely(previous) &&
> -	    stripe_ahead_of_reshape(mddev, conf, sh)) {
> +	    stripe_across_reshape(mddev, conf, sh)) {
>   		/*
>   		 * Expansion moved on while waiting for a stripe.
>   		 * Expansion could still move past after this
>
Xiao Ni Feb. 23, 2024, 2 p.m. UTC | #2 
On Fri, Feb 23, 2024 at 11:09 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2024/02/20 23:30, Xiao Ni 写道:
> > stripe_ahead_of_reshape is used to check if a stripe region cross the
> > reshape position. So first, change the function name to
> > stripe_across_reshape to describe the usage of this function.
> >
> > For reshape backwards, it starts reshape from the end of array and conf->
> > reshape_progress is init to raid5_size. During reshape, if previous is true
> > (set in make_stripe_request) and max_sector >= conf->reshape_progress, ios
> > should wait until reshape window moves forward. But ios don't need to wait
> > if max_sector is raid5_size.
> >
> > And put the conditions into the function directly to make understand the
> > codes easily.
> >
> > This can be reproduced easily by lvm2 test shell/lvconvert-raid-reshape.sh
> > For dm raid reshape, before starting sync thread, it needs to reload table
> > some times. In one time dm raid uses MD_RECOVERY_WAIT to delay reshape and
> > it doesn't start sync thread this time. Then one io comes in and it waits
> > because stripe_ahead_of_reshape returns true because it's a backward
> > reshape and max_sectors > conf->reshape_progress. But the reshape hasn't
> > started. So skip this check when reshape_progress is raid5_size
>
> Like I said before, after following merged patch:
>
> ad39c08186f8 md: Don't register sync_thread for reshape directly

Hi Kuai

The reason I send this patch set is I don't agree the patch 01 of your
patch set. Does the patch (md: Don't register sync_thread for reshape
directly) rely on patch 01 from your patch set? Because your patch 01
only changes one logic and other patches rely on it. So it's the
reason I can't accept the following patches.

Regards
Xiao
>
> You should not see that sync_thread fo reshape is registered while
> MD_RECOVERY_WAIT is set, so this patch should not be needed.
>
> Thanks,
> Kuai
>
> >
> > Fixes: 486f60558607 ("md/raid5: Check all disks in a stripe_head for reshape progress")
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >   drivers/md/raid5.c | 22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 8497880135ee..4c71df4e2370 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -5832,17 +5832,12 @@ static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
> >                                         sector >= reshape_sector;
> >   }
> >
> > -static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
> > -                                sector_t max, sector_t reshape_sector)
> > -{
> > -     return mddev->reshape_backwards ? max < reshape_sector :
> > -                                       min >= reshape_sector;
> > -}
> > -
> > -static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
> > +static sector_t raid5_size(struct mddev *mddev, sector_t sectors, int raid_disks);
> > +static bool stripe_across_reshape(struct mddev *mddev, struct r5conf *conf,
> >                                   struct stripe_head *sh)
> >   {
> >       sector_t max_sector = 0, min_sector = MaxSector;
> > +     sector_t reshape_pos = 0;
> >       bool ret = false;
> >       int dd_idx;
> >
> > @@ -5856,9 +5851,12 @@ static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
> >
> >       spin_lock_irq(&conf->device_lock);
> >
> > -     if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
> > -                                  conf->reshape_progress))
> > -             /* mismatch, need to try again */
> > +     reshape_pos = conf->reshape_progress;
> > +     if (mddev->reshape_backwards) {
> > +             if (max_sector >= reshape_pos &&
> > +                 reshape_pos != raid5_size(mddev, 0, 0))
> > +                     ret = true;
> > +     } else if (min_sector < reshape_pos)
> >               ret = true;
> >
> >       spin_unlock_irq(&conf->device_lock);
> > @@ -5969,7 +5967,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
> >       }
> >
> >       if (unlikely(previous) &&
> > -         stripe_ahead_of_reshape(mddev, conf, sh)) {
> > +         stripe_across_reshape(mddev, conf, sh)) {
> >               /*
> >                * Expansion moved on while waiting for a stripe.
> >                * Expansion could still move past after this
> >
>
diff mbox series

Patch

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 8497880135ee..4c71df4e2370 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5832,17 +5832,12 @@  static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
 					  sector >= reshape_sector;
 }
 
-static bool range_ahead_of_reshape(struct mddev *mddev, sector_t min,
-				   sector_t max, sector_t reshape_sector)
-{
-	return mddev->reshape_backwards ? max < reshape_sector :
-					  min >= reshape_sector;
-}
-
-static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
+static sector_t raid5_size(struct mddev *mddev, sector_t sectors, int raid_disks);
+static bool stripe_across_reshape(struct mddev *mddev, struct r5conf *conf,
 				    struct stripe_head *sh)
 {
 	sector_t max_sector = 0, min_sector = MaxSector;
+	sector_t reshape_pos = 0;
 	bool ret = false;
 	int dd_idx;
 
@@ -5856,9 +5851,12 @@  static bool stripe_ahead_of_reshape(struct mddev *mddev, struct r5conf *conf,
 
 	spin_lock_irq(&conf->device_lock);
 
-	if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
-				     conf->reshape_progress))
-		/* mismatch, need to try again */
+	reshape_pos = conf->reshape_progress;
+	if (mddev->reshape_backwards) {
+		if (max_sector >= reshape_pos &&
+		    reshape_pos != raid5_size(mddev, 0, 0))
+			ret = true;
+	} else if (min_sector < reshape_pos)
 		ret = true;
 
 	spin_unlock_irq(&conf->device_lock);
@@ -5969,7 +5967,7 @@  static enum stripe_result make_stripe_request(struct mddev *mddev,
 	}
 
 	if (unlikely(previous) &&
-	    stripe_ahead_of_reshape(mddev, conf, sh)) {
+	    stripe_across_reshape(mddev, conf, sh)) {
 		/*
 		 * Expansion moved on while waiting for a stripe.
 		 * Expansion could still move past after this