Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.

[NeilBrown <neilb@suse.de>]

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

On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <majianpeng@gmail.com> wrote:

> In Commit 7bfec5f35c68121e7b1849f3f4166dd96c8da5b3:
> "if there is a spare and a want_replacement device, start replacement."
> But it did not consider the raid was degraded at the same time.
> When we add spare disk in order to recovery, unless raid was ok and then
> started replacement or vice versa.
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d267672..f74c9a5 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5447,6 +5447,8 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>  	struct disk_info *p;
>  	int first = 0;
>  	int last = conf->raid_disks - 1;
> +	int null_disk = -1;
> +	int wantreplace_disk = -1;
>  
>  	if (mddev->recovery_disabled == conf->recovery_disabled)
>  		return -EBUSY;
> @@ -5468,27 +5470,35 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>  		disk = rdev->saved_raid_disk;
>  	else
>  		disk = first;
> -	for ( ; disk <= last ; disk++) {
> +	for ( ; disk <= last; disk++) {
>  		p = conf->disks + disk;
> -		if (p->rdev == NULL) {
> -			clear_bit(In_sync, &rdev->flags);
> -			rdev->raid_disk = disk;
> -			err = 0;
> -			if (rdev->saved_raid_disk != disk)
> -				conf->fullsync = 1;
> -			rcu_assign_pointer(p->rdev, rdev);
> -			break;
> -		}
> -		if (test_bit(WantReplacement, &p->rdev->flags) &&
> -		    p->replacement == NULL) {
> -			clear_bit(In_sync, &rdev->flags);
> -			set_bit(Replacement, &rdev->flags);
> -			rdev->raid_disk = disk;
> -			err = 0;
> +		if (p->rdev == NULL && null_disk == -1)
> +			null_disk = disk;
> +		else if (p->rdev != NULL &&
> +			test_bit(WantReplacement, &p->rdev->flags) &&
> +			p->replacement == NULL &&
> +			wantreplace_disk == -1)
> +			wantreplace_disk = disk;
> +	}
> +
> +	if (null_disk != -1 && (rdev->raid_disk < 0 ||
> +			wantreplace_disk == -1)) {
> +		p = conf->disks + null_disk;
> +		clear_bit(In_sync, &rdev->flags);
> +		rdev->raid_disk = null_disk;
> +		err = 0;
> +		if (rdev->saved_raid_disk != null_disk)
>  			conf->fullsync = 1;
> -			rcu_assign_pointer(p->replacement, rdev);
> -			break;
> -		}
> +		rcu_assign_pointer(p->rdev, rdev);
> +	} else if (wantreplace_disk != -1 && (rdev->raid_disk >= 0 ||
> +				null_disk == -1)) {
> +		p = conf->disks + wantreplace_disk;
> +		clear_bit(In_sync, &rdev->flags);
> +		set_bit(Replacement, &rdev->flags);
> +		rdev->raid_disk = wantreplace_disk;
> +		err = 0;
> +		conf->fullsync = 1;
> +		rcu_assign_pointer(p->replacement, rdev);
>  	}
>  	print_raid5_conf(conf);
>  	return err;

Good point, but the code feels a little ... clumsy.

How about this?

NeilBrown


diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d267672..4f0861e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5465,10 +5465,9 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
 	if (rdev->saved_raid_disk >= 0 &&
 	    rdev->saved_raid_disk >= first &&
 	    conf->disks[rdev->saved_raid_disk].rdev == NULL)
-		disk = rdev->saved_raid_disk;
-	else
-		disk = first;
-	for ( ; disk <= last ; disk++) {
+		first = rdev->saved_raid_disk;
+
+	for (disk = first; disk <= last; disk++) {
 		p = conf->disks + disk;
 		if (p->rdev == NULL) {
 			clear_bit(In_sync, &rdev->flags);
@@ -5477,8 +5476,10 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
 			if (rdev->saved_raid_disk != disk)
 				conf->fullsync = 1;
 			rcu_assign_pointer(p->rdev, rdev);
-			break;
+			goto out;
 		}
+	}
+	for (disk = first; disk <= last; disk++) {
 		if (test_bit(WantReplacement, &p->rdev->flags) &&
 		    p->replacement == NULL) {
 			clear_bit(In_sync, &rdev->flags);
@@ -5490,6 +5491,7 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
 			break;
 		}
 	}
+out:
 	print_raid5_conf(conf);
 	return err;
 }


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