* [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
@ 2012-06-05 7:32 majianpeng
2012-06-06 1:28 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: majianpeng @ 2012-06-05 7:32 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid
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
@@ -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;
--
1.7.5.4
--------------
majianpeng
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
2012-06-05 7:32 [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time majianpeng
@ 2012-06-06 1:28 ` NeilBrown
2012-06-06 3:24 ` majianpeng
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2012-06-06 1:28 UTC (permalink / raw)
To: majianpeng; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 4173 bytes --]
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
> @@ -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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
2012-06-06 1:28 ` NeilBrown
@ 2012-06-06 3:24 ` majianpeng
2012-06-06 3:55 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: majianpeng @ 2012-06-06 3:24 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
2012-06-06 3:24 ` majianpeng
@ 2012-06-06 3:55 ` NeilBrown
2012-06-06 5:06 ` majianpeng
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2012-06-06 3:55 UTC (permalink / raw)
To: majianpeng; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 4974 bytes --]
On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@gmail.com> wrote:
> 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.
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.
> 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;
> }
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
2012-06-06 3:55 ` NeilBrown
@ 2012-06-06 5:06 ` majianpeng
2012-06-27 3:47 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: majianpeng @ 2012-06-06 5:06 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid
>On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> 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.
>
>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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.
2012-06-06 5:06 ` majianpeng
@ 2012-06-27 3:47 ` NeilBrown
0 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2012-06-27 3:47 UTC (permalink / raw)
To: majianpeng; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 3228 bytes --]
On Wed, 6 Jun 2012 13:06:53 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >
> >> 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.
> >
> >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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-27 3:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-05 7:32 [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time majianpeng
2012-06-06 1:28 ` NeilBrown
2012-06-06 3:24 ` majianpeng
2012-06-06 3:55 ` NeilBrown
2012-06-06 5:06 ` majianpeng
2012-06-27 3:47 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).