On Wed, 6 Jun 2012 13:06:53 +0800 majianpeng <majianpeng@xxxxxxxxx> wrote:
> >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.
>
Ahhh... yes, of course.
I've added that to that patch.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
