md: Don't do normal-write on unresync area of replacement-disk.
PATCH 3/4 md: Don't do normal-write on unresync area of replacement-disk.
Replacement is a fullsync which don't depent on bitmap.So regardless of the presence and absence of bitmap, it do full resync.
If offset of normal io is larger than offset of resync,it will write again when resync arrived this offset.
--- drivers/md/raid1.c | 4 +++- drivers/md/raid10.c | 4 +++- drivers/md/raid5.c | 3 ++- 3 files changed, 8 insertions(+), 3 deletions(-)
drivers/md/raid1.c +3 -1
@@ -1173,7 +1173,9 @@ read_again:set_bit (R1BIO_Degraded, &r1_bio->state);continue ; } - +if (test_bit (Replacement, &rdev->flags) && + conf->mddev->curr_resync < r1_bio->sector) +continue ;atomic_inc (&rdev->nr_pending);if (test_bit (WriteErrorSeen, &rdev->flags)) {sector_t first_bad;
drivers/md/raid10.c +3 -1
@@ -1337,7 +1337,9 @@ retry_write: || test_bit(Unmerged, &rdev->flags))) rdev =NULL ;if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector))) rrdev =NULL ; r10_bio->devs[i].bio =NULL ;
drivers/md/raid5.c +2 -1
@@ -602,7 +602,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s) rdev =NULL ;if (rdev)atomic_inc (&rdev->nr_pending); -if (rrdev &&test_bit (Faulty, &rrdev->flags)) +if (rrdev && (test_bit (Faulty, &rrdev->flags) || + conf->mddev->curr_resync < sh->sector)) rrdev =NULL ;if (rrdev)atomic_inc (&rrdev->nr_pending);
Signature · 1 line
1.7.9.5
Quoted 4 lines · On Thu, 28 Feb 2013 15:50:37 +0800 majianpeng <[email protected]> wrote:
Replacement is a fullsync which don't depent on bitmap.So regardless of the presence and absence of bitmap, it do full resync.
If offset of normal io is larger than offset of resync,it will write again when resync arrived this offset.
This might be OK for RAID1 and RAID10 as recover is paused when writes happen, but that is not the case for RAID5, so it isn't safe to test against curr_resync - it gets updated a bit too later.
Also you messed up the formatting in raid10.c
I'm not convinced this optimisation is really worth it.
NeilBrown
Quoted 52 lines
Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/raid1.c | 4 +++- drivers/md/raid10.c | 4 +++- drivers/md/raid5.c | 3 ++- 3 files changed, 8 insertions(+), 3 deletions(-)drivers/md/raid1.c +3
-1@@ -1173,7 +1173,9 @@ read_again:set_bit (R1BIO_Degraded, &r1_bio->state);continue ; } - +if (test_bit (Replacement, &rdev->flags) && + conf->mddev->curr_resync < r1_bio->sector) +continue ;atomic_inc (&rdev->nr_pending);if (test_bit (WriteErrorSeen, &rdev->flags)) {sector_t first_bad;drivers/md/raid10.c +3
-1@@ -1337,7 +1337,9 @@ retry_write: || test_bit(Unmerged, &rdev->flags))) rdev =NULL ;if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector))) rrdev =NULL ; r10_bio->devs[i].bio =NULL ;drivers/md/raid5.c +2
-1@@ -602,7 +602,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s) rdev =NULL ;if (rdev)atomic_inc (&rdev->nr_pending); -if (rrdev &&test_bit (Faulty, &rrdev->flags)) +if (rrdev && (test_bit (Faulty, &rrdev->flags) || + conf->mddev->curr_resync < sh->sector)) rrdev =NULL ;if (rrdev)atomic_inc (&rrdev->nr_pending);
Quoted 11 lines · On Thu, 28 Feb 2013 15:50:37 +0800 majianpeng <[email protected]> wrote:
Quoted 5 lines
Replacement is a fullsync which don't depent on bitmap.So regardless of the presence and absence of bitmap, it do full resync.
If offset of normal io is larger than offset of resync,it will write again when resync arrived this offset.This might be OK for RAID1 and RAID10 as recover is paused when writes happen, but that is not the case for RAID5, so it isn't safe to test against
curr_resync- it gets updated a bit too later.
->curr_resync + STRIPE_SECTORS is the next stripe which willbe replaced. How about the ->curr_resync+STRIPE_SECTORS?
Quoted 2 lines
Also you messed up the formatting in
raid10.c
Can you explain in detail?
Quoted 2 lines
I'm not convinced this optimisation is really worth it.
Maybe for HDD disk, it only improve speed by reducing some write operation. But for ssd disk, it can reduce one write.
Thanks!
Jianpeng Ma
Quoted 57 lines
NeilBrown
Quoted 52 lines
Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/raid1.c | 4 +++- drivers/md/raid10.c | 4 +++- drivers/md/raid5.c | 3 ++- 3 files changed, 8 insertions(+), 3 deletions(-)drivers/md/raid1.c +3
-1@@ -1173,7 +1173,9 @@ read_again:set_bit (R1BIO_Degraded, &r1_bio->state);continue ; } - +if (test_bit (Replacement, &rdev->flags) && + conf->mddev->curr_resync < r1_bio->sector) +continue ;atomic_inc (&rdev->nr_pending);if (test_bit (WriteErrorSeen, &rdev->flags)) {sector_t first_bad;drivers/md/raid10.c +3
-1@@ -1337,7 +1337,9 @@ retry_write: || test_bit(Unmerged, &rdev->flags))) rdev =NULL ;if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector))) rrdev =NULL ; r10_bio->devs[i].bio =NULL ;drivers/md/raid5.c +2
-1@@ -602,7 +602,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s) rdev =NULL ;if (rdev)atomic_inc (&rdev->nr_pending); -if (rrdev &&test_bit (Faulty, &rrdev->flags)) +if (rrdev && (test_bit (Faulty, &rrdev->flags) || + conf->mddev->curr_resync < sh->sector)) rrdev =NULL ;if (rrdev)atomic_inc (&rrdev->nr_pending);
Quoted 7 lines · On Mon, 4 Mar 2013 10:24:16 +0800 majianpeng <[email protected]> wrote:
Quoted 7 lines · On Thu, 28 Feb 2013 15:50:37 +0800 majianpeng <[email protected]> wrote:
Quoted 5 lines
Replacement is a fullsync which don't depent on bitmap.So regardless of the presence and absence of bitmap, it do full resync.
If offset of normal io is larger than offset of resync,it will write again when resync arrived this offset.
Quoted 6 lines
Quoted 4 lines
This might be OK for RAID1 and RAID10 as recover is paused when writes happen, but that is not the case for RAID5, so it isn't safe to test against
curr_resync- it gets updated a bit too later.->curr_resync +
STRIPE_SECTORSis the next stripe which willbe replaced. How about the ->curr_resync+STRIPE_SECTORS?
If you aren't certain, then neither am I.
As resync and normal writes can be intermingled you would need some guarantee that a write wouldn't be missed, and that almost certainly means a test under a lock against some value which is updated under a lock.
Quoted 11 lines
Quoted 3 lines
Also you messed up the formatting in
raid10.cCan you explain in detail?
drivers/md/raid10.c retry_write: · line 1342
Quoted 5 lines
drivers/md/raid10.c +3
-1if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector)))
Text that is inside parentheses (or other brackets) should never be to the left of the opening parenthesis unless that parenthesis is at the end of a line.
In the original code, the "|| test_bit(Unmerged....." was to the right of the '('. In your version it starts to the left, and the lines you added also start to the left.
Quoted 4 lines
Quoted 2 lines
I'm not convinced this optimisation is really worth it.
Maybe for HDD disk, it only improve speed by reducing some write operation. But for ssd disk, it can reduce one write.
Still doesn't sound convincing.
NeilBrown
Quoted 60 lines
Thanks!
Jianpeng MaQuoted 57 lines
NeilBrown
Quoted 52 lines
Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/raid1.c | 4 +++- drivers/md/raid10.c | 4 +++- drivers/md/raid5.c | 3 ++- 3 files changed, 8 insertions(+), 3 deletions(-)drivers/md/raid1.c +3
-1@@ -1173,7 +1173,9 @@ read_again:set_bit (R1BIO_Degraded, &r1_bio->state);continue ; } - +if (test_bit (Replacement, &rdev->flags) && + conf->mddev->curr_resync < r1_bio->sector) +continue ;atomic_inc (&rdev->nr_pending);if (test_bit (WriteErrorSeen, &rdev->flags)) {sector_t first_bad;drivers/md/raid10.c +3
-1@@ -1337,7 +1337,9 @@ retry_write: || test_bit(Unmerged, &rdev->flags))) rdev =NULL ;if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector))) rrdev =NULL ; r10_bio->devs[i].bio =NULL ;drivers/md/raid5.c +2
-1@@ -602,7 +602,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s) rdev =NULL ;if (rdev)atomic_inc (&rdev->nr_pending); -if (rrdev &&test_bit (Faulty, &rrdev->flags)) +if (rrdev && (test_bit (Faulty, &rrdev->flags) || + conf->mddev->curr_resync < sh->sector)) rrdev =NULL ;if (rrdev)atomic_inc (&rrdev->nr_pending);
Quoted 13 lines · On Mon, 4 Mar 2013 10:24:16 +0800 majianpeng <[email protected]> wrote:
Quoted 12 lines
Quoted 12 lines · On Thu, 28 Feb 2013 15:50:37 +0800 majianpeng <[email protected]> wrote:
Quoted 5 lines
Replacement is a fullsync which don't depent on bitmap.So regardless of the presence and absence of bitmap, it do full resync.
If offset of normal io is larger than offset of resync,it will write again when resync arrived this offset.This might be OK for RAID1 and RAID10 as recover is paused when writes happen, but that is not the case for RAID5, so it isn't safe to test against
curr_resync- it gets updated a bit too later.
Quoted 9 lines
Quoted 2 lines
->curr_resync +
STRIPE_SECTORSis the next stripe which willbe replaced. How about the ->curr_resync+STRIPE_SECTORS?If you aren't certain, then neither am I.
As resync and normal writes can be intermingled you would need some guarantee that a write wouldn't be missed, and that almost certainly means a test under a lock against some value which is updated under a lock.
Yes as you said, we must not lost to write.
Because the normal io and replace io don't get the same stripe at the same time. So there are two cases:
A: if (sh->sector < curr_resync)
For this case,withot doubt it must be write.
B: if (sh->sector >= curr_resync)
For sh->sector == curr_resync, this can't happen. Becase replace must wait this stripe. For sh->sector > curr_resync, although the replace increate the ->curr_resync.But it can't
got the stripe which sh->sector == curr_resync.
So i think there isn't need some locks to protect.
For the above result, the base is the resync and normal io don't get the same stripe at the same time.
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Also you messed up the formatting in
raid10.cCan you explain in detail?
Quoted 13 lines
Quoted 5 lines
drivers/md/raid10.c retry_write: · line 1342
Quoted 5 lines
drivers/md/raid10.c +3
-1if (rrdev && (test_bit (Faulty, &rrdev->flags) - ||test_bit (Unmerged, &rrdev->flags))) + ||test_bit (Unmerged, &rrdev->flags) || + (test_bit (Replacement, &rrdev->flags) && + conf->mddev->curr_resync < r10_bio->sector)))Text that is inside parentheses (or other brackets) should never be to the left of the opening parenthesis unless that parenthesis is at the end of a line.
In the original code, the "|| test_bit(Unmerged....." was to the right of the '('. In your version it starts to the left, and the lines you added also start to the left.
Thanks for the advise.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I'm not convinced this optimisation is really worth it.
Maybe for HDD disk, it only improve speed by reducing some write operation. But for ssd disk, it can reduce one write.
Still doesn't sound convincing.
Maybe i do some test.But I still insist that write useless data is useless.
Thanks!
Jianpeng Ma