On Sun, Sep 05, 2010 at 11:23:35PM +0400, Kulikov Vasiliy wrote:
> On Sun, Sep 05, 2010 at 21:01 +0200, Sam Ravnborg wrote:
> > On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
> > > From: Vasiliy Kulikov <segooon@xxxxxxxxx>
> > >
> > > rcu_dereference() is macro, so it might use its argument twice.
> > > Argument must not has side effects.
> > >
> > > It was found by compiler warning:
> > > drivers/md/raid1.c: In function ‘read_balance’:
> > > drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> >
> > This change looks wrong.
> > In the original implementation new_disk is incremented and
> > then we do the array lookup.
> > With your implementation it looks like we increment it after
> > the array lookup.
>
> No, the original code increments new_disk and then dereferences mirrors.
>
> The full code:
>
> for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> r1_bio->bios[new_disk] == IO_BLOCKED ||
> !rdev || !test_bit(In_sync, &rdev->flags)
> || test_bit(WriteMostly, &rdev->flags);
> rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
>
> if (rdev && test_bit(In_sync, &rdev->flags) &&
> r1_bio->bios[new_disk] != IO_BLOCKED)
> wonly_disk = new_disk;
>
> if (new_disk == conf->raid_disks - 1) {
> new_disk = wonly_disk;
> break;
> }
> }
>
> so,
>
> for (a; b; c = f(++g)) {
> ...
> }
Thanks - that explains it.
This code really screams for a helper function but thats another matter.
Sam
--
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
