Re: [BUG / PATCH] raid1: set BIO_UPTODATE after read error

[Paul Clements <paul.clements@steeleye.com>]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

Neil Brown wrote:
> On Friday September 10, paul.clements@steeleye.com wrote:
> 
>>Neil,
>>
>>unless you've already done so, I believe there is a little fix needed in 
>>the raid1 read reschedule code. As the code currently works, a read that 
>>is retried will continue to fail and cause raid1 to go into an infinite 
>>retry loop:
> 
> 
> Thanks.  I must have noticed this when writing the raid10 module
> because it gets it right.  Obviously I didn't "back-port" it to raid1.
> 
> A few other fields need to be reset for safety.

Well, it turns out that even that is not enough. Even with your patch, 
we're still seeing ext3-fs errors, which means we're getting bogus data 
on the read retry (the filesystem is re-created every test run, so 
there's no chance of lingering filesystem corruption causing the errors).

Rather than getting down in the guts of the bio and trying to reset all 
the fields that potentially could have been touched, I think it's 
probably safer to simply discard the bio that had the failed I/O 
attempted against it and clone a new bio, setting it up just as we did 
for the original read attempt. This seems to work better and will also 
protect us against any future changes in the bio code (or bio handling 
in any driver sitting below raid1), which could break read retry again. 
Patch attached.

--
Paul

[-- Attachment #2: md_raid1_bio_clone_on_read_error.diff --]
[-- Type: text/plain, Size: 930 bytes --]

--- linux-2.6.5-7.97/drivers/md/raid1.c.orig	2004-09-15 12:12:16.658276872 -0400
+++ linux-2.6.5-7.97/drivers/md/raid1.c		2004-09-15 12:11:38.644055912 -0400
@@ -962,14 +962,19 @@ static void raid1d(mddev_t *mddev)
 			} else {
 				r1_bio->bios[r1_bio->read_disk] = NULL;
 				r1_bio->read_disk = disk;
+				/* discard the failed bio and clone a new one */
+				bio_put(bio);
+				bio = bio_clone(r1_bio->master_bio, GFP_NOIO);
 				r1_bio->bios[r1_bio->read_disk] = bio;
 				printk(KERN_ERR "raid1: %s: redirecting sector %llu to"
 				       " another mirror\n",
 				       bdevname(rdev->bdev,b),
 				       (unsigned long long)r1_bio->sector);
-				bio->bi_bdev = rdev->bdev;
 				bio->bi_sector = r1_bio->sector + rdev->data_offset;
+				bio->bi_bdev = rdev->bdev;
+				bio->bi_end_io = raid1_end_read_request;
 				bio->bi_rw = READ;
+				bio->bi_private = r1_bio;
 				unplug = 1;
 				generic_make_request(bio);
 			}