On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <majianpeng@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
