On Thursday October 12, mjt@xxxxxxxxxx wrote:
> Neil Brown wrote:
> []
> > Fix count of degraded drives in raid10.
> >
> > Signed-off-by: Neil Brown <neilb@xxxxxxx>
> >
> > --- .prev/drivers/md/raid10.c 2006-10-09 14:18:00.000000000 +1000
> > +++ ./drivers/md/raid10.c 2006-10-05 20:10:07.000000000 +1000
> > @@ -2079,7 +2079,7 @@ static int run(mddev_t *mddev)
> > disk = conf->mirrors + i;
> >
> > if (!disk->rdev ||
> > - !test_bit(In_sync, &rdev->flags)) {
> > + !test_bit(In_sync, &disk->rdev->flags)) {
> > disk->head_position = 0;
> > mddev->degraded++;
> > }
>
> Neil, this makes me nervous. Seriously.
Yes. Bugs are a problem.
>
> How many bugs like this has been fixed so far? 10? 50? I stopped counting
> long time ago. And it's the same thing in every case - misuse of rdev vs
> disk->rdev. The same pattern.
I really don't think there have been that many that follow that
pattern that closely. Maybe 2 or 3.
>
> I wonder if it can be avoided in the first place somehow - maybe don't
> declare and use local variable `rdev' (not by name, but by the semantics
> of it), and always use disk->rdev or mddev->whatever in every place,
> explicitly, and let the compiler optimize the deref if possible?
>
There certainly are styles of programming and rules for choosing names
that can help reduce bugs. And the kernel style does encourage some
good practices.
But that won't be enough by itself. We need good style, and a review
process, and testing. And still bugs will get through, but there
should be fewer. You are welcome to help with any of these.
I hope to set up a more structured testing system soon with should be
able to catch this sort of bug at least.
> And btw, this is another 2.6.18.1 candidate (if it's not too late already).
Yes, it was too late for 2.6.18.1. I'll submit it for 2.6.18.2.
Thanks,
NeilBrown
-
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
