On Fri, 07 May 2010 12:37:12 -0500 Jordan Russell <jr-list-2010@xxxxxx> wrote: > On 4/22/2010 1:21 PM, I wrote: > > raid1.c's read_balance() function has: > > > > const unsigned long this_sector = r1_bio->sector; > > > > Doesn't that need to be: > > > > const sector_t this_sector = r1_bio->sector; > > > > for compatibility with LBD on 32-bit platforms? > > Any comments on this? > > At very least it appears the truncation to 32 bits will break the "disk > whose head is closest" logic. > Thanks for the reminder. Yes, it should be sector_t. I've queued up a patch for the next merge window. This bug will only affect devices greater than 2TB and as you say it will upset the read-balancing logic. During a resync, it could possibly cause a read to be served from a non-primary device which might have different data than the primary device. It is fairly unlikely that this will actually cause a problem though (if the array is recoverying to a spare, rather than resyncing after a crash, there is no such risk). Thanks, NeilBrown commit eba3d84bb8d7692edc3eaa4d3589e6e4a692e50f Author: NeilBrown <neilb@xxxxxxx> Date: Sat May 8 08:20:17 2010 +1000 md: Fix read balancing in RAID1 and RAID10 on drives > 2TB read_balance uses a "unsigned long" for a sector number which will get truncated beyond 2TB. This will cause read-balancing to be non-optimal, and can cause data to be read from the 'wrong' branch during a resync. This has a very small chance of returning wrong data. Reported-by: Jordan Russell <jr-list-2010@xxxxxx> Cc: stable@xxxxxxxxxx Signed-off-by: NeilBrown <neilb@xxxxxxx> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 49a2219..7d22838 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -418,7 +418,7 @@ static void raid1_end_write_request(struct bio *bio, int error) */ static int read_balance(conf_t *conf, r1bio_t *r1_bio) { - const unsigned long this_sector = r1_bio->sector; + const sector_t this_sector = r1_bio->sector; int new_disk = conf->last_used, disk = new_disk; int wonly_disk = -1; const int sectors = r1_bio->sectors; @@ -434,7 +434,7 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio) retry: if (conf->mddev->recovery_cp < MaxSector && (this_sector + sectors >= conf->next_resync)) { - /* Choose the first operation device, for consistancy */ + /* Choose the first operational device, for consistancy */ new_disk = 0; for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev); diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index e843e12..f6a9885 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -495,7 +495,7 @@ static int raid10_mergeable_bvec(struct request_queue *q, */ static int read_balance(conf_t *conf, r10bio_t *r10_bio) { - const unsigned long this_sector = r10_bio->sector; + const sector_t this_sector = r10_bio->sector; int disk, slot, nslot; const int sectors = r10_bio->sectors; sector_t new_distance, current_distance; -- 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