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:

quoted

On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:

quoted

From: Vasiliy Kulikov <redacted>

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