>On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@xxxxxxxxx> wrote: > >> On Tue, 5 Jun 2012 18:28:13 neil wrote: >> >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 >> > >> >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. > >You've added a test for 'p->rdev != NULL' - is that all? > No, I also add >> + for (disk = first; disk <= last; disk++) { + p = conf->disks + disk; >> + if (p->rdev != NULL && >> + test_bit(WantReplacement, &p->rdev->flags) && >> + p->replacement == NULL) { You lost : p = conf->disks + disk; in next loop. Thanks! >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. > > >> 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 > > > >> >> 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; >> } >> > > -------------- majianpeng ?韬{.n?????%??檩??w?{.n???{炳盯w???塄}?财??j:+v??????2??璀??摺?囤??z夸z罐?+?????w棹f