Neil Brown writes:
> On Wednesday August 15, maccetta@xxxxxxxxxxxxxxxxxx wrote:
> > Neil Brown writes:
> > > On Wednesday August 15, maccetta@xxxxxxxxxxxxxxxxxx wrote:
> > > >
> > ...
> > This happens in our old friend sync_request_write()? I'm dealing with
>
> Yes, that would be the place.
>
> > ...
> > This fragment
> >
> > if (j < 0 || test_bit(MD_RECOVERY_CHECK, &mddev->recovery)) {
> > sbio->bi_end_io = NULL;
> > rdev_dec_pending(conf->mirrors[i].rdev, mddev);
> > } else {
> > /* fixup the bio for reuse */
> > ...
> > }
> >
> > looks suspicously like any correction attempt for 'check' is being
> > short-circuited to me, regardless of whether or not there was a read
> > error. Actually, even if the rewrite was not being short-circuited,
> > I still don't see the path that would update 'corrected_errors' in this
> > case. There are only two raid1.c sites that touch 'corrected_errors', one
> > is in fix_read_errors() and the other is later in sync_request_write().
> > With my limited understanding of how this all works, neither of these
> > paths would seem to apply here.
>
> hmmm.... yes....
> I guess I was thinking of the RAID5 code rather than the RAID1 code.
> It doesn't do the right thing does it?
> Maybe this patch is what we need. I think it is right.
>
> Thanks,
> NeilBrown
>
>
> Signed-off-by: Neil Brown <neilb@xxxxxxx>
>
> ### Diffstat output
> ./drivers/md/raid1.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c
> --- .prev/drivers/md/raid1.c 2007-08-16 10:29:58.000000000 +1000
> +++ ./drivers/md/raid1.c 2007-08-17 12:07:35.000000000 +1000
> @@ -1260,7 +1260,8 @@ static void sync_request_write(mddev_t *
> j = 0;
> if (j >= 0)
> mddev->resync_mismatches += r1_bio->sec
> tors;
> - if (j < 0 || test_bit(MD_RECOVERY_CHECK, &mddev
> ->recovery)) {
> + if (j < 0 || (test_bit(MD_RECOVERY_CHECK, &mdde
> v->recovery)
> + && text_bit(BIO_UPTODATE, &sbio->
> bi_flags))) {
> sbio->bi_end_io = NULL;
> rdev_dec_pending(conf->mirrors[i].rdev,
> mddev);
> } else {
I tried this (with the typo fixed) and it indeed issues a re-write.
However, it doesn't seem to do anything with the corrected errors
count if the rewrite succeeds. Since end_sync_write() is only used one
other place when !In_sync, I tried the following and it seems to work
to get the error count updated. I don't know whether this belongs in
end_sync_write() but I'd think it needs to come after the write actually
succeeds so that seems like the earliest it could be done.
--- BUILD/kernel/drivers/md/raid1.c 2007-06-04 13:52:42.000000000 -0400
+++ drivers/md/raid1.c 2007-08-17 16:52:14.219364000 -0400
@@ -1166,6 +1166,7 @@ static int end_sync_write(struct bio *bi
conf_t *conf = mddev_to_conf(mddev);
int i;
int mirror=0;
+ mdk_rdev_t *rdev;
if (bio->bi_size)
return 1;
@@ -1175,6 +1176,8 @@ static int end_sync_write(struct bio *bi
mirror = i;
break;
}
+
+ rdev = conf->mirrors[mirror].rdev;
if (!uptodate) {
int sync_blocks = 0;
sector_t s = r1_bio->sector;
@@ -1186,7 +1189,13 @@ static int end_sync_write(struct bio *bi
s += sync_blocks;
sectors_to_go -= sync_blocks;
} while (sectors_to_go > 0);
- md_error(mddev, conf->mirrors[mirror].rdev);
+ md_error(mddev, rdev);
+ } else if (test_bit(In_sync, &rdev->flags)) {
+ /*
+ * If we are currently in sync, this was a re-write to
+ * correct a read error and we should account for it.
+ */
+ atomic_add(r1_bio->sectors, &rdev->corrected_errors);
}
update_head_pos(mirror, r1_bio);
@@ -1251,7 +1260,8 @@ static void sync_request_write(mddev_t *
}
if (j >= 0)
mddev->resync_mismatches += r1_bio->sectors;
- if (j < 0 || test_bit(MD_RECOVERY_CHECK, &mddev->recovery)) {
+ if (j < 0 || (test_bit(MD_RECOVERY_CHECK, &mddev->recovery)
+ && test_bit(BIO_UPTODATE, &sbio->bi_flags))) {
sbio->bi_end_io = NULL;
rdev_dec_pending(conf->mirrors[i].rdev, mddev);
} else {
--
Mike Accetta
ECI Telecom Ltd.
Transport Networking Division, US (previously Laurel Networks)
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
