Re: fr1-2.16 patch for 2.6.10 kernel

In article <[email protected]> you wrote:
> >>So i suggest : 
> >>#ifdef CONFIG_MD_RAID1_ROBUST_READ
> >>+        /*
> >>+         * Uh, no. Choose the next disk if we can, not the first.
> >>+         */
> >>+        for (i = 0; i < disks; i++) {
> >>+                if (conf->mirrors[i].rdev == *rdevp) {
> > 
> > 
> > Yes - this was the intention. It seems to be like that in my source
> > code, so the problem seems to be that I hand-edited the patch looking
> > at the source, and did not catch up with the source properly.
> 
> > But what sets *rdevp before the call to map()?
> > 
> > I see that in my source there is:
> > 
> > #ifdef CONFIG_MD_RAID1_ROBUST_READ
> >                         rdev = conf->mirrors[r1_bio->read_disk].rdev;
> > #endif /* CONFIG_MD_RAID1_ROBUST_READ */
> >                         if ((disk=map(mddev, &rdev)) == -1) {
> >                                 printk(KERN_ALERT "raid1: %s: unrecoverable I/O"
> >                                        " read error for block %llu\n",
> > 
> > So it looks like I set it, but I doubt that is in the patch.
> 
> Exactly, and the bug was there !  Now i can cycle fails/adds without 

Well, that wouldn't have been a bug that caused that kind of problem.

All it would mean would be that map() would not "know" what was the
original disk on which a read had failed, so it might choose the same
disk again to retry the read on. Which would fail again. Shrug. It'll
give up after n tries, where n is the number of disks in the array.

> crashing ! But now it seems that clean sectors are not skipped....

They would be skipped (in resync) so long as the bitmap is in place.
But then I already remarked to you that when you reinsert your disk
the second time, it looks like its superblock does not contain a uuid
marking that enables it to be recognized. So there would be no reason
to skip anything in the resync.

Please insert code that prints out the uuid on the newly inserted
disk, right before the test of it in md.c.

And a few more printks spread through the rest of that check() routine
would pinpoint where the oops happens!


> Just another point :  in map(), don't really understand these lines :
> 
> 
>          if (i >= disks)
>            i = 0;
>          for (; i < disks; i++)  {
>            mdk_rdev_t *rdev = conf->mirrors[i].rdev;
>            if (rdev && rdev != *rdevp && rdev->in_sync){
>                       ^^^^ Here ! maybe i ?

Well, it wants to avoid rechoosing the original disk to retry the read
on! What do you mean about "i"? I don't understand the comment.


>             *rdevp = rdev;
>             atomic_inc(&rdev->nr_pending);
>             spin_unlock_irq(&conf->device_lock);
>             return i;
>                  }

Peter