[PATCH] md/raid10: remove unnecessary checks with 'first'

[PATCH] md/raid10: remove unnecessary checks with 'first'

[Guoqing Jiang]

Since the value of first is always '1', we can
set min_offset_diff directly.

Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
 drivers/md/raid10.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0f13d57..edaf8f4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3702,7 +3702,6 @@ static int raid10_run(struct mddev *mddev)
 	struct md_rdev *rdev;
 	sector_t size;
 	sector_t min_offset_diff = 0;
-	int first = 1;
 	bool discard_supported = false;
 
 	if (mddev->private == NULL) {
@@ -3758,8 +3757,7 @@ static int raid10_run(struct mddev *mddev)
 			diff = -diff;
 		if (diff < 0)
 			diff = 0;
-		if (first || diff < min_offset_diff)
-			min_offset_diff = diff;
+		min_offset_diff = diff;
 
 		if (mddev->gendisk)
 			disk_stack_limits(mddev->gendisk, rdev->bdev,
@@ -4140,7 +4138,6 @@ static int raid10_start_reshape(struct mddev *mddev)
 
 	unsigned long before_length, after_length;
 	sector_t min_offset_diff = 0;
-	int first = 1;
 	struct geom new;
 	struct r10conf *conf = mddev->private;
 	struct md_rdev *rdev;
@@ -4169,8 +4166,7 @@ static int raid10_start_reshape(struct mddev *mddev)
 				diff = -diff;
 			if (diff < 0)
 				diff = 0;
-			if (first || diff < min_offset_diff)
-				min_offset_diff = diff;
+			min_offset_diff = diff;
 		}
 	}
 
-- 
2.6.6

Re: [PATCH] md/raid10: remove unnecessary checks with 'first'

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Sat, Apr 01 2017, Guoqing Jiang wrote:

> Since the value of first is always '1', we can
> set min_offset_diff directly.

Alternately, maybe the fact that first is always '1', is a sign of a bug
that should be fixed.

The calculation of "min_offset_diff" doesn't make much sense now.  It is
just the last_offset_diff.

I think the correct fix is to set "first = 0;" at the end of the
rdev_for_each() loop.

Thanks for spotting this.

NeilBrown

>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
>  drivers/md/raid10.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0f13d57..edaf8f4 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3702,7 +3702,6 @@ static int raid10_run(struct mddev *mddev)
>  	struct md_rdev *rdev;
>  	sector_t size;
>  	sector_t min_offset_diff = 0;
> -	int first = 1;
>  	bool discard_supported = false;
>  
>  	if (mddev->private == NULL) {
> @@ -3758,8 +3757,7 @@ static int raid10_run(struct mddev *mddev)
>  			diff = -diff;
>  		if (diff < 0)
>  			diff = 0;
> -		if (first || diff < min_offset_diff)
> -			min_offset_diff = diff;
> +		min_offset_diff = diff;
>  
>  		if (mddev->gendisk)
>  			disk_stack_limits(mddev->gendisk, rdev->bdev,
> @@ -4140,7 +4138,6 @@ static int raid10_start_reshape(struct mddev *mddev)
>  
>  	unsigned long before_length, after_length;
>  	sector_t min_offset_diff = 0;
> -	int first = 1;
>  	struct geom new;
>  	struct r10conf *conf = mddev->private;
>  	struct md_rdev *rdev;
> @@ -4169,8 +4166,7 @@ static int raid10_start_reshape(struct mddev *mddev)
>  				diff = -diff;
>  			if (diff < 0)
>  				diff = 0;
> -			if (first || diff < min_offset_diff)
> -				min_offset_diff = diff;
> +			min_offset_diff = diff;
>  		}
>  	}
>  
> -- 
> 2.6.6

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH V2] md/raid10: reset the 'first' at the end of loop

[Guoqing Jiang]

We need to set "first = 0' at the end of rdev_for_each
loop, so we can get the array's min_offset_diff correctly
otherwise min_offset_diff just means the last rdev's
offset diff.

Suggested-by: NeilBrown <neilb@suse.com>
Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
 drivers/md/raid10.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0f13d57..e055ec9 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3769,6 +3769,7 @@ static int raid10_run(struct mddev *mddev)
 
 		if (blk_queue_discard(bdev_get_queue(rdev->bdev)))
 			discard_supported = true;
+		first = 0;
 	}
 
 	if (mddev->queue) {
@@ -4172,6 +4173,7 @@ static int raid10_start_reshape(struct mddev *mddev)
 			if (first || diff < min_offset_diff)
 				min_offset_diff = diff;
 		}
+		first = 0;
 	}
 
 	if (max(before_length, after_length) > min_offset_diff)
-- 
2.6.6

Re: [PATCH V2] md/raid10: reset the 'first' at the end of loop

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

On Thu, Apr 06 2017, Guoqing Jiang wrote:

> We need to set "first = 0' at the end of rdev_for_each
> loop, so we can get the array's min_offset_diff correctly
> otherwise min_offset_diff just means the last rdev's
> offset diff.
>
> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>

Reviewed-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown

> ---
>  drivers/md/raid10.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0f13d57..e055ec9 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3769,6 +3769,7 @@ static int raid10_run(struct mddev *mddev)
>  
>  		if (blk_queue_discard(bdev_get_queue(rdev->bdev)))
>  			discard_supported = true;
> +		first = 0;
>  	}
>  
>  	if (mddev->queue) {
> @@ -4172,6 +4173,7 @@ static int raid10_start_reshape(struct mddev *mddev)
>  			if (first || diff < min_offset_diff)
>  				min_offset_diff = diff;
>  		}
> +		first = 0;
>  	}
>  
>  	if (max(before_length, after_length) > min_offset_diff)
> -- 
> 2.6.6

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH V2] md/raid10: reset the 'first' at the end of loop

[Shaohua Li]

On Thu, Apr 06, 2017 at 09:12:18AM +0800, Guoqing Jiang wrote:
> We need to set "first = 0' at the end of rdev_for_each
> loop, so we can get the array's min_offset_diff correctly
> otherwise min_offset_diff just means the last rdev's
> offset diff.
> 
> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>

applied, thanks!

> ---
>  drivers/md/raid10.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0f13d57..e055ec9 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3769,6 +3769,7 @@ static int raid10_run(struct mddev *mddev)
>  
>  		if (blk_queue_discard(bdev_get_queue(rdev->bdev)))
>  			discard_supported = true;
> +		first = 0;
>  	}
>  
>  	if (mddev->queue) {
> @@ -4172,6 +4173,7 @@ static int raid10_start_reshape(struct mddev *mddev)
>  			if (first || diff < min_offset_diff)
>  				min_offset_diff = diff;
>  		}
> +		first = 0;
>  	}
>  
>  	if (max(before_length, after_length) > min_offset_diff)
> -- 
> 2.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html