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

PATCH In review majianpeng 2 participants 6 messages Jun 5, 2012 – Jun 27, 2012 MD RAID

Share & tools

majianpeng

author

2012-06-05 07:32 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

drivers/md/raid5.c +29 ~~-19~~

index d267672..f74c9a5 100644
@@ -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;

Signature · 6 lines

1.7.5.4



--------------
majianpeng

NeilBrown

2012-06-06 01:28 UTC #1 lore raw

Quoted 69 lines · On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <[email protected]> wrote:

Signed-off-by majianpeng <[email protected]>

---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

drivers/md/raid5.c +22 ~~-19~~

index d267672..f74c9a5 100644
@@ -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)) {

drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5504

Quoted 10 lines

drivers/md/raid5.c +7 -0

+		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

drivers/md/raid5.c +7 -5

index d267672..4f0861e 100644
@@ -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;
 }

majianpeng

author

2012-06-06 03:24 UTC #2 lore raw

Quoted 58 lines · On Tue, 5 Jun 2012 18:28:13 neil wrote:

drivers/md/raid5.c

Quoted 15 lines · On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)
drivers/md/raid5.c +0 -0
index d267672..f74c9a5 100644

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

How about this?

NeilBrown

drivers/md/raid5.c +6 -5

index d267672..4f0861e 100644
@@ -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);

drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5497

Quoted 7 lines

drivers/md/raid5.c +1 -0

@@ -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.

drivers/md/raid5.c +12 -7

index d267672..24162c1 100644
@@ -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;
 }

NeilBrown

2012-06-06 03:55 UTC #3 lore raw

Quoted 59 lines · On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <[email protected]> wrote:

Quoted 58 lines · On Tue, 5 Jun 2012 18:28:13 neil wrote:
drivers/md/raid5.c
Quoted 15 lines · On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)
drivers/md/raid5.c +0 -0
index d267672..f74c9a5 100644
Good point, but the code feels a little ... clumsy.
How about this?
NeilBrown
drivers/md/raid5.c +6 -5
index d267672..4f0861e 100644
@@ -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;
 		}

Quoted 8 lines

drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5500
Quoted 7 lines
drivers/md/raid5.c +1 -0
 	}
+out:
 	print_raid5_conf(conf);
 	return err;
 }
I tested and found a bug.I corrected it like this.

You've added a test for 'p->rdev != NULL' - is that all?

That isn't necessary. If any p->rdev were NULL then the first loop would find it and the second loop would never be entered.

Quoted 2 lines

But I had a question:why p->rdev not protect by rcu_read_lock? I think it should be.

rcu is not necessary here. We hold mddev->mutex as does the code which removes devices, so we cannot race with it. We only need rcu when not holding the mutex, and when not performing resync/recovery/etc as that prevents ->rdev from being removed too.

Thanks,
NeilBrown

Quoted 47 lines

drivers/md/raid5.c +12 -7

index d267672..24162c1 100644
@@ -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;
 }

majianpeng

author

2012-06-06 05:06 UTC #4 lore raw

Quoted 70 lines · On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <[email protected]> wrote:

Quoted 60 lines · On Tue, 5 Jun 2012 18:28:13 neil wrote:
Quoted 58 lines · On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <[email protected]> wrote:
drivers/md/raid5.c
Quoted 16 lines
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 <[email protected]>
---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)
drivers/md/raid5.c +0 -0
index d267672..f74c9a5 100644
Good point, but the code feels a little ... clumsy.
How about this?
NeilBrown
drivers/md/raid5.c +6 -5
index d267672..4f0861e 100644
@@ -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;
 		}
Quoted 8 lines
drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5500
Quoted 7 lines
drivers/md/raid5.c +1 -0
 	}
+out:
 	print_raid5_conf(conf);
 	return err;
 }
I tested and found a bug.I corrected it like this.

Quoted 2 lines

You've added a test for 'p->rdev != NULL' - is that all?

No, I also add

Quoted 1 line

Quoted 1 line
+ for (disk = first; disk <= last; disk++) {

+ p = conf->disks + disk;

Quoted 3 lines

drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5487
Quoted 3 lines
drivers/md/raid5.c +3 -0
+               if (p->rdev != NULL &&
+                       test_bit(WantReplacement, &p->rdev->flags) &&
+                       p->replacement == NULL) {

You lost :
p = conf->disks + disk;
in next loop.
Thanks!

Quoted 56 lines

That isn't necessary. If any p->rdev were NULL then the first loop would find it and the second loop would never be entered.

Quoted 2 lines

But I had a question:why p->rdev not protect by rcu_read_lock? I think it should be.

Thanks,
NeilBrown

Quoted 39 lines

drivers/md/raid5.c +11 -7

index d267672..24162c1 100644
@@ -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)

Quoted 10 lines

drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5500
Quoted 8 lines
drivers/md/raid5.c +1 -0
                        break;
                }
        }
+out:
        print_raid5_conf(conf);
        return err;
 }

--------------
majianpeng

NeilBrown

2012-06-27 03:47 UTC #5 lore raw

Quoted 73 lines · On Wed, 6 Jun 2012 13:06:53 +0800 majianpeng <[email protected]> wrote:

Quoted 70 lines · On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <[email protected]> wrote:
Quoted 60 lines · On Tue, 5 Jun 2012 18:28:13 neil wrote:
Quoted 58 lines · On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <[email protected]> wrote:
drivers/md/raid5.c
Quoted 16 lines
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 <[email protected]>
---
 drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)
drivers/md/raid5.c +0 -0
index d267672..f74c9a5 100644
Good point, but the code feels a little ... clumsy.
How about this?
NeilBrown
drivers/md/raid5.c +6 -5
index d267672..4f0861e 100644
@@ -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;
 		}
Quoted 8 lines
drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5500
Quoted 7 lines
drivers/md/raid5.c +1 -0
 	}
+out:
 	print_raid5_conf(conf);
 	return err;
 }
I tested and found a bug.I corrected it like this.
Quoted 2 lines
You've added a test for 'p->rdev != NULL' - is that all?
No, I also add

Quoted 9 lines

Quoted 1 line
Quoted 1 line
+ for (disk = first; disk <= last; disk++) {
+ p = conf->disks + disk;
Quoted 3 lines
drivers/md/raid5.c static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev) · line 5487
Quoted 3 lines
drivers/md/raid5.c +3 -0
+               if (p->rdev != NULL &&
+                       test_bit(WantReplacement, &p->rdev->flags) &&
+                       p->replacement == NULL) {
You lost :
p = conf->disks + disk;
in next loop.

Ahhh... yes, of course.
I've added that to that patch.

Thanks,
NeilBrown