md/raid1:Fix a logic bug in fix_sync_read_error().
From 0fe15c8e1bd5e46234d37573f3322312d8da325d Mon Sep 17 00:00:00 2001
From: majianpeng <[email protected]>
Date: Sat, 31 Mar 2012 10:27:33 +0800
Subject: [PATCH] md/raid1:Fix a logic bug in fix_sync_read_error(). If d==read_disk && success == 1 and then break, so d =
read_disk. When exec this judgement: >>start = d; >>/* write it back and re-read */ >>while (d != r1_bio->read_disk) { Because d == read_disk,so write and re-add did not exec.
--- drivers/md/raid1.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/md/raid1.c +0 -1
@@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio) bio->bi_io_vec[idx].bv_page, READ, false)) { success =1 ; -break ; } } d++;
Signature · 4 lines
1.7.5.4 kedacomkernel 2012-03-31
Quoted 9 lines · On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <[email protected]>…
Quoted 1 line
From
0fe15c8e1bd5e46234d37573f3322312d8da325dMon Sep 17 00:00:00 2001From: majianpeng <[email protected]>
Date: Sat, 31 Mar 2012 10:27:33 +0800
Subject: [PATCH] md/raid1:Fix a logic bug infix_sync_read_error(). If d==read_disk && success == 1 and then break, so d =
read_disk. When exec this judgement: >>start = d; >>/* write it back and re-read */ >>while (d !=r1_bio->read_disk) { Because d == read_disk,so write and re-add did not exec.
That is correct, and that is how it should be.
If we get a read error, then try again and get a successful read, then there is nothing more that we need to do. No need to write or anything.
So: current code is correct.
Thanks,
NeilBrown
Quoted 19 lines
Signed-off-by majianpeng <[email protected]>--- drivers/md/raid1.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)drivers/md/raid1.c +0
-1@@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio) bio->bi_io_vec[idx].bv_page, READ, false)) { success =1 ; -break ; } } d++;
Quoted 9 lines
Quoted 9 lines
That is correct, and that is how it should be.
If we get a read error, then try again and get a successful read, then there is nothing more that we need to do. No need to write or anything.
So: current code is correct.
Thanks,
NeilBrown
Sorry Neil,I don't understand what's your mean?
I think this function is want to do something like follow when all sync_read failed. 1:want to read partly by PAGE_SIZE from all read devices.
2:if read success and write write to other and read back, in order to repair those disks.
for example :
disk A,B ,read 64k from 0 to 128 sector.
supposed: disk A read failed because sector 0-7 was bad sector.
disk B read failed because sector 8-15 was bad sector.
by this function,we can fix sector 0-7 of A and sector 8-15 of B.
Is ok?
If B is read_disk,so read 0-7 is ok and want to write it to A.But becasue the code ,do not write to disk A.
------------------
kedacomkernel
2012-04-05
-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-04-02 10:05:34
收件人：kedacomkernel
抄送：linux-raid
主题：Re: md/raid1:Fix a logic bug in fix_sync_read_error().
Quoted 9 lines · On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <[email protected]>…
Quoted 1 line
From
0fe15c8e1bd5e46234d37573f3322312d8da325dMon Sep 17 00:00:00 2001From: majianpeng <[email protected]>
Date: Sat, 31 Mar 2012 10:27:33 +0800
Subject: [PATCH] md/raid1:Fix a logic bug infix_sync_read_error(). If d==read_disk && success == 1 and then break, so d =
read_disk. When exec this judgement: >>start = d; >>/* write it back and re-read */ >>while (d !=r1_bio->read_disk) { Because d == read_disk,so write and re-add did not exec.
That is correct, and that is how it should be.
If we get a read error, then try again and get a successful read, then there is nothing more that we need to do. No need to write or anything.
So: current code is correct.
Thanks,
NeilBrown
Quoted 19 lines
Signed-off-by majianpeng <[email protected]>--- drivers/md/raid1.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)drivers/md/raid1.c +0
-1@@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio) bio->bi_io_vec[idx].bv_page, READ, false)) { success =1 ; -break ; } } d++;
Quoted 5 lines · On Thu, 5 Apr 2012 10:23:47 +0800 "kedacomkernel" <[email protected]>…
Quoted 4 lines
Quoted 4 lines
That is correct, and that is how it should be.
If we get a read error, then try again and get a successful read, then there is nothing more that we need to do. No need to write or anything.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
So: current code is correct.
Thanks,
NeilBrownSorry Neil,I don't understand what's your mean?
I think this function is want to do something like follow when allsync_readfailed. 1:want to read partly byPAGE_SIZEfrom all read devices.
Not exactly. fix_sync_read_error() only gets called if all reads that were attempted (often just one, but for 'check' or 'repair' it will have tried all devicess) have failed.
So what we need to do in this case is just to get the valid data from somewhere. As soon as we have found it we can be happy.
Quoted 1 line
2:if read success and write write to other and read back, in order to repair those disks.
yes ... and no. The code might be a bit confusing. There may even be room for improvement.
But after fix_sync_read_error succeeds, the rest of sync_request_write will write out the data that it recovered to other devices if necessary.
so it doesn't matter that fix_sync_read_error() doesn't write out.
Quoted 7 lines
for example :
disk A,B ,read 64k from 0 to 128 sector.
supposed: disk A read failed because sector 0-7 was bad sector.
disk B read failed because sector 8-15 was bad sector.
by this function,we can fix sector 0-7 of A and sector 8-15 of B.
Is ok?
Hmm... not sure.fix_sync_read_error should read all the data from wherever it is. I guess I'm not certain that sync_request_write() will write it out everywhere.
So maybe there is a problem there.
However you should make sure that your analysis covered both fix_sync_read_error and sync_request_write.
NeilBrown
Quoted 58 lines
If B is read_disk,so read 0-7 is ok and want to write it to A.But becasue the code ,do not write to disk A.
------------------
kedacomkernel
2012-04-05-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-04-02 10:05:34
收件人：kedacomkernel
抄送：linux-raid
主题：Re: md/raid1:Fix a logic bug infix_sync_read_error().Quoted 9 lines · On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <[email protected]>…
Quoted 1 line
From
0fe15c8e1bd5e46234d37573f3322312d8da325dMon Sep 17 00:00:00 2001From: majianpeng <[email protected]>
Date: Sat, 31 Mar 2012 10:27:33 +0800
Subject: [PATCH] md/raid1:Fix a logic bug infix_sync_read_error(). If d==read_disk && success == 1 and then break, so d =
read_disk. When exec this judgement: >>start = d; >>/* write it back and re-read */ >>while (d !=r1_bio->read_disk) { Because d == read_disk,so write and re-add did not exec.That is correct, and that is how it should be.
If we get a read error, then try again and get a successful read, then there is nothing more that we need to do. No need to write or anything.
So: current code is correct.
Thanks,
NeilBrownQuoted 19 lines
Signed-off-by majianpeng <[email protected]>--- drivers/md/raid1.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)drivers/md/raid1.c +0
-1@@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio) bio->bi_io_vec[idx].bv_page, READ, false)) { success =1 ; -break ; } } d++;