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

[majianpeng <majianpeng@gmail.com>]

On  Tue, 5 Jun 2012 18:28:13 neil wrote:
>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
>
>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;
> }
>
>
I tested and found a bug.I corrected it like this.
But I had a question:why  p->rdev not protect by rcu_read_lock?
I think it should be.

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d267672..24162c1 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,10 +5476,15 @@ 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;
                }
-               if (test_bit(WantReplacement, &p->rdev->flags) &&
-                   p->replacement == NULL) {
+       }
+
+       for (disk = first; disk <= last; disk++) {
+               p = conf->disks + disk;
+               if (p->rdev != NULL &&
+                       test_bit(WantReplacement, &p->rdev->flags) &&
+                       p->replacement == NULL) {
                        clear_bit(In_sync, &rdev->flags);
                        set_bit(Replacement, &rdev->flags);
                        rdev->raid_disk = disk;
@@ -5490,6 +5494,7 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
                        break;
                }
        }
+out:
        print_raid5_conf(conf);
        return err;
 }