[PATCH 0/5] md: sector_t conversions

[PATCH 0/5] md: sector_t conversions

[Andre Noll]

Hi

here is the first batch of patches that aim to change the internal
representations of sizes of raid devices from 1K blocks to 512 byte
sectors.

Patches 1-3 change the internal functions update_size(),
calc_dev_size() and calc_dev_sboffset() so that they take/return a
sector count instead of a block count. Patch 4 changes the semantics
of rdev->sb_offset so that its value now represents the number of the
start sector instead of the start block. Patch 5 removes some macros
that have become unused due to this conversion.

These changes are all simple and straight-forward, but it's easy to
make mistakes because there's no help from the compiler. So please
review carefully.


 drivers/md/bitmap.c       |   10 ++--
 drivers/md/md.c           |  111 +++++++++++++++++++++------------------------
 include/linux/raid/md_k.h |    2 +-
 include/linux/raid/md_p.h |    3 -
 4 files changed, 58 insertions(+), 68 deletions(-)

Regards
Andre

[PATCH 1/5] md: Make update_size() take the number of sectors.

[Andre Noll]

Changing the internal representations of sizes of raid devices
from 1K blocks to sector counts (512B units) is desirable because
it allows to get rid of many divisions/multiplications and unnecessary
casts that are present in the current code.

This patch is a first step in this direction. It replaces the old
1K-based "size" argument of update_size() by "num_sectors" and
fixes up its two callers.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2580ac1..2ab13a3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2785,7 +2785,7 @@ size_show(mddev_t *mddev, char *page)
 	return sprintf(page, "%llu\n", (unsigned long long)mddev->size);
 }
 
-static int update_size(mddev_t *mddev, unsigned long size);
+static int update_size(mddev_t *mddev, sector_t num_sectors);
 
 static ssize_t
 size_store(mddev_t *mddev, const char *buf, size_t len)
@@ -2802,7 +2802,7 @@ size_store(mddev_t *mddev, const char *buf, size_t len)
 		return -EINVAL;
 
 	if (mddev->pers) {
-		err = update_size(mddev, size);
+		err = update_size(mddev, size * 2);
 		md_update_sb(mddev, 1);
 	} else {
 		if (mddev->size == 0 ||
@@ -4476,24 +4476,24 @@ static int set_array_info(mddev_t * mddev, mdu_array_info_t *info)
 	return 0;
 }
 
-static int update_size(mddev_t *mddev, unsigned long size)
+static int update_size(mddev_t *mddev, sector_t num_sectors)
 {
 	mdk_rdev_t * rdev;
 	int rv;
 	struct list_head *tmp;
-	int fit = (size == 0);
+	int fit = (num_sectors == 0);
 
 	if (mddev->pers->resize == NULL)
 		return -EINVAL;
-	/* The "size" is the amount of each device that is used.
-	 * This can only make sense for arrays with redundancy.
-	 * linear and raid0 always use whatever space is available
-	 * We can only consider changing the size if no resync
-	 * or reconstruction is happening, and if the new size
-	 * is acceptable. It must fit before the sb_offset or,
-	 * if that is <data_offset, it must fit before the
-	 * size of each device.
-	 * If size is zero, we find the largest size that fits.
+	/* The "num_sectors" is the number of sectors of each device that
+	 * is used.  This can only make sense for arrays with redundancy.
+	 * linear and raid0 always use whatever space is available. We can only
+	 * consider changing this number if no resync or reconstruction is
+	 * happening, and if the new size is acceptable. It must fit before the
+	 * sb_offset or, if that is <data_offset, it must fit before the size
+	 * of each device.  If num_sectors is zero, we find the largest size
+	 * that fits.
+
 	 */
 	if (mddev->sync_thread)
 		return -EBUSY;
@@ -4501,12 +4501,12 @@ static int update_size(mddev_t *mddev, unsigned long size)
 		sector_t avail;
 		avail = rdev->size * 2;
 
-		if (fit && (size == 0 || size > avail/2))
-			size = avail/2;
-		if (avail < ((sector_t)size << 1))
+		if (fit && (num_sectors == 0 || num_sectors > avail))
+			num_sectors = avail;
+		if (avail < num_sectors)
 			return -ENOSPC;
 	}
-	rv = mddev->pers->resize(mddev, (sector_t)size *2);
+	rv = mddev->pers->resize(mddev, num_sectors);
 	if (!rv) {
 		struct block_device *bdev;
 
@@ -4588,7 +4588,7 @@ static int update_array_info(mddev_t *mddev, mdu_array_info_t *info)
 			return mddev->pers->reconfig(mddev, info->layout, -1);
 	}
 	if (info->size >= 0 && mddev->size != info->size)
-		rv = update_size(mddev, info->size);
+		rv = update_size(mddev, info->size * 2);
 
 	if (mddev->raid_disks    != info->raid_disks)
 		rv = update_raid_disks(mddev, info->raid_disks);
-- 
1.5.3.8

Re: [PATCH 1/5] md: Make update_size() take the number of sectors.

[Neil Brown]

On Thursday July 10, maan@systemlinux.org wrote:
> @@ -4588,7 +4588,7 @@ static int update_array_info(mddev_t *mddev, mdu_array_info_t *info)
>  			return mddev->pers->reconfig(mddev, info->layout, -1);
>  	}
>  	if (info->size >= 0 && mddev->size != info->size)
> -		rv = update_size(mddev, info->size);
> +		rv = update_size(mddev, info->size * 2);
>  
>  	if (mddev->raid_disks    != info->raid_disks)
>  		rv = update_raid_disks(mddev, info->raid_disks);

I've made this
		rv = update_size(mddev, (sector_t)info->size * 2);

as info->size is an int and without the cast, a size of 2TB 
(2^31 K) would become 0.

Otherwise, it's good.  Thanks.

NeilBrown

[PATCH 2/5] md: Replace calc_dev_size() by calc_num_sectors().

[Andre Noll]

Number of sectors is the preferred unit for sizes of raid devices,
so change calc_dev_size() so that it returns this unit instead of
the number of 1K blocks.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2ab13a3..1a6001e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -353,15 +353,13 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
 	return MD_NEW_SIZE_BLOCKS(size);
 }
 
-static sector_t calc_dev_size(mdk_rdev_t *rdev, unsigned chunk_size)
+static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
 {
-	sector_t size;
-
-	size = rdev->sb_offset;
+	sector_t num_sectors = rdev->sb_offset * 2;
 
 	if (chunk_size)
-		size &= ~((sector_t)chunk_size/1024 - 1);
-	return size;
+		num_sectors &= ~((sector_t)chunk_size/512 - 1);
+	return num_sectors;
 }
 
 static int alloc_disk_sb(mdk_rdev_t * rdev)
@@ -759,7 +757,7 @@ static int super_90_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version
 		else 
 			ret = 0;
 	}
-	rdev->size = calc_dev_size(rdev, sb->chunk_size);
+	rdev->size = calc_num_sectors(rdev, sb->chunk_size) / 2;
 
 	if (rdev->size < sb->size && sb->level > 1)
 		/* "this cannot possibly happen" ... */
@@ -4215,7 +4213,7 @@ static int add_new_disk(mddev_t * mddev, mdu_disk_info_t *info)
 			rdev->sb_offset = rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
 		} else 
 			rdev->sb_offset = calc_dev_sboffset(rdev->bdev);
-		rdev->size = calc_dev_size(rdev, mddev->chunk_size);
+		rdev->size = calc_num_sectors(rdev, mddev->chunk_size) / 2;
 
 		err = bind_rdev_to_array(rdev, mddev);
 		if (err) {
@@ -4257,7 +4255,6 @@ static int hot_add_disk(mddev_t * mddev, dev_t dev)
 {
 	char b[BDEVNAME_SIZE];
 	int err;
-	unsigned int size;
 	mdk_rdev_t *rdev;
 
 	if (!mddev->pers)
@@ -4290,8 +4287,7 @@ static int hot_add_disk(mddev_t * mddev, dev_t dev)
 		rdev->sb_offset =
 			rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
 
-	size = calc_dev_size(rdev, mddev->chunk_size);
-	rdev->size = size;
+	rdev->size = calc_num_sectors(rdev, mddev->chunk_size) / 2;
 
 	if (test_bit(Faulty, &rdev->flags)) {
 		printk(KERN_WARNING 
-- 
1.5.3.8

[PATCH 3/5] md: Make calc_dev_sboffset() return a sector count.

[Andre Noll]

As BLOCK_SIZE_BITS is 10 and

	MD_NEW_SIZE_SECTORS(2 * x) = 2 * NEW_SIZE_BLOCKS(x),

the return value of calc_dev_sboffset() doubles. Fix up all three
callers accordingly.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1a6001e..6179e8f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -347,10 +347,11 @@ static struct mdk_personality *find_pers(int level, char *clevel)
 	return NULL;
 }
 
+/* return the offset of the super block in 512byte sectors */
 static inline sector_t calc_dev_sboffset(struct block_device *bdev)
 {
-	sector_t size = bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
-	return MD_NEW_SIZE_BLOCKS(size);
+	sector_t num_sectors = bdev->bd_inode->i_size / 512;
+	return MD_NEW_SIZE_SECTORS(num_sectors);
 }
 
 static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
@@ -679,7 +680,7 @@ static int super_90_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version
 	 *
 	 * It also happens to be a multiple of 4Kb.
 	 */
-	sb_offset = calc_dev_sboffset(rdev->bdev);
+	sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
 	rdev->sb_offset = sb_offset;
 
 	ret = read_disk_sb(rdev, MD_SB_BYTES);
@@ -4212,7 +4213,7 @@ static int add_new_disk(mddev_t * mddev, mdu_disk_info_t *info)
 			printk(KERN_INFO "md: nonpersistent superblock ...\n");
 			rdev->sb_offset = rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
 		} else 
-			rdev->sb_offset = calc_dev_sboffset(rdev->bdev);
+			rdev->sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
 		rdev->size = calc_num_sectors(rdev, mddev->chunk_size) / 2;
 
 		err = bind_rdev_to_array(rdev, mddev);
@@ -4282,7 +4283,7 @@ static int hot_add_disk(mddev_t * mddev, dev_t dev)
 	}
 
 	if (mddev->persistent)
-		rdev->sb_offset = calc_dev_sboffset(rdev->bdev);
+		rdev->sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
 	else
 		rdev->sb_offset =
 			rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
-- 
1.5.3.8

Re: [PATCH 3/5] md: Make calc_dev_sboffset() return a sector count.

[Neil Brown]

On Thursday July 10, maan@systemlinux.org wrote:
> As BLOCK_SIZE_BITS is 10 and
> 
> 	MD_NEW_SIZE_SECTORS(2 * x) = 2 * NEW_SIZE_BLOCKS(x),
> 
> the return value of calc_dev_sboffset() doubles. Fix up all three
> callers accordingly.

Only I have four callers :-)
There is new code in my tree that isn't in Linus' tree which you are
basing on.
Maybe it would be worth basing on linux-next? or pull the "for-next"
branch from git://neil.brown.name/md/

I've fixed the extra caller and push it into my tree.

Thanks,
NeilBrown

[PATCH 4/5] md: Turn rdev->sb_offset into a sector-based quantity.

[Andre Noll]

Rename it to sb_start to make sure all users have been converted.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/bitmap.c       |   10 ++++----
 drivers/md/md.c           |   56 +++++++++++++++++++++------------------------
 include/linux/raid/md_k.h |    2 +-
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index b26927c..f1c44ff 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -225,7 +225,7 @@ static struct page *read_sb_page(mddev_t *mddev, long offset, unsigned long inde
 		    || test_bit(Faulty, &rdev->flags))
 			continue;
 
-		target = (rdev->sb_offset << 1) + offset + index * (PAGE_SIZE/512);
+		target = rdev->sb_start + offset + index * (PAGE_SIZE/512);
 
 		if (sync_page_io(rdev->bdev, target, PAGE_SIZE, page, READ)) {
 			page->index = index;
@@ -262,12 +262,12 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 					/* bitmap runs in to metadata */
 					return -EINVAL;
 				if (rdev->data_offset + mddev->size*2
-				    > rdev->sb_offset*2 + bitmap->offset)
+				    > rdev->sb_start + bitmap->offset)
 					/* data runs in to bitmap */
 					return -EINVAL;
-			} else if (rdev->sb_offset*2 < rdev->data_offset) {
+			} else if (rdev->sb_start < rdev->data_offset) {
 				/* METADATA BITMAP DATA */
-				if (rdev->sb_offset*2
+				if (rdev->sb_start
 				    + bitmap->offset
 				    + page->index*(PAGE_SIZE/512) + size/512
 				    > rdev->data_offset)
@@ -277,7 +277,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 				/* DATA METADATA BITMAP - no problems */
 			}
 			md_super_write(mddev, rdev,
-				       (rdev->sb_offset<<1) + bitmap->offset
+				       rdev->sb_start + bitmap->offset
 				       + page->index * (PAGE_SIZE/512),
 				       size,
 				       page);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 6179e8f..7b6e238 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -356,7 +356,7 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
 
 static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
 {
-	sector_t num_sectors = rdev->sb_offset * 2;
+	sector_t num_sectors = rdev->sb_start;
 
 	if (chunk_size)
 		num_sectors &= ~((sector_t)chunk_size/512 - 1);
@@ -383,7 +383,7 @@ static void free_disk_sb(mdk_rdev_t * rdev)
 		put_page(rdev->sb_page);
 		rdev->sb_loaded = 0;
 		rdev->sb_page = NULL;
-		rdev->sb_offset = 0;
+		rdev->sb_start = 0;
 		rdev->size = 0;
 	}
 }
@@ -529,7 +529,7 @@ static int read_disk_sb(mdk_rdev_t * rdev, int size)
 		return 0;
 
 
-	if (!sync_page_io(rdev->bdev, rdev->sb_offset<<1, size, rdev->sb_page, READ))
+	if (!sync_page_io(rdev->bdev, rdev->sb_start, size, rdev->sb_page, READ))
 		goto fail;
 	rdev->sb_loaded = 1;
 	return 0;
@@ -672,16 +672,14 @@ static int super_90_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version
 	char b[BDEVNAME_SIZE], b2[BDEVNAME_SIZE];
 	mdp_super_t *sb;
 	int ret;
-	sector_t sb_offset;
 
 	/*
-	 * Calculate the position of the superblock,
+	 * Calculate the position of the superblock (512byte sectors),
 	 * it's at the end of the disk.
 	 *
 	 * It also happens to be a multiple of 4Kb.
 	 */
-	sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
-	rdev->sb_offset = sb_offset;
+	rdev->sb_start = calc_dev_sboffset(rdev->bdev);
 
 	ret = read_disk_sb(rdev, MD_SB_BYTES);
 	if (ret) return ret;
@@ -1033,12 +1031,12 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 {
 	struct mdp_superblock_1 *sb;
 	int ret;
-	sector_t sb_offset;
+	sector_t sb_start;
 	char b[BDEVNAME_SIZE], b2[BDEVNAME_SIZE];
 	int bmask;
 
 	/*
-	 * Calculate the position of the superblock.
+	 * Calculate the position of the superblock in 512byte sectors.
 	 * It is always aligned to a 4K boundary and
 	 * depeding on minor_version, it can be:
 	 * 0: At least 8K, but less than 12K, from end of device
@@ -1047,22 +1045,21 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 	 */
 	switch(minor_version) {
 	case 0:
-		sb_offset = rdev->bdev->bd_inode->i_size >> 9;
-		sb_offset -= 8*2;
-		sb_offset &= ~(sector_t)(4*2-1);
+		sb_start = rdev->bdev->bd_inode->i_size >> 9;
+		sb_start -= 8*2;
+		sb_start &= ~(sector_t)(4*2-1);
 		/* convert from sectors to K */
-		sb_offset /= 2;
 		break;
 	case 1:
-		sb_offset = 0;
+		sb_start = 0;
 		break;
 	case 2:
-		sb_offset = 4;
+		sb_start = 8;
 		break;
 	default:
 		return -EINVAL;
 	}
-	rdev->sb_offset = sb_offset;
+	rdev->sb_start = sb_start;
 
 	/* superblock is rarely larger than 1K, but it can be larger,
 	 * and it is safe to read 4k, so we do that
@@ -1076,7 +1073,7 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 	if (sb->magic != cpu_to_le32(MD_SB_MAGIC) ||
 	    sb->major_version != cpu_to_le32(1) ||
 	    le32_to_cpu(sb->max_dev) > (4096-256)/2 ||
-	    le64_to_cpu(sb->super_offset) != (rdev->sb_offset<<1) ||
+	    le64_to_cpu(sb->super_offset) != rdev->sb_start ||
 	    (le32_to_cpu(sb->feature_map) & ~MD_FEATURE_ALL) != 0)
 		return -EINVAL;
 
@@ -1112,7 +1109,7 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 		rdev->sb_size = (rdev->sb_size | bmask) + 1;
 
 	if (minor_version
-	    && rdev->data_offset < sb_offset + (rdev->sb_size/512))
+	    && rdev->data_offset < sb_start / 2 + (rdev->sb_size/512))
 		return -EINVAL;
 
 	if (sb->level == cpu_to_le32(LEVEL_MULTIPATH))
@@ -1148,7 +1145,7 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 	if (minor_version)
 		rdev->size = ((rdev->bdev->bd_inode->i_size>>9) - le64_to_cpu(sb->data_offset)) / 2;
 	else
-		rdev->size = rdev->sb_offset;
+		rdev->size = rdev->sb_start / 2;
 	if (rdev->size < le64_to_cpu(sb->data_size)/2)
 		return -EINVAL;
 	rdev->size = le64_to_cpu(sb->data_size)/2;
@@ -1757,11 +1754,11 @@ repeat:
 		dprintk("%s ", bdevname(rdev->bdev,b));
 		if (!test_bit(Faulty, &rdev->flags)) {
 			md_super_write(mddev,rdev,
-				       rdev->sb_offset<<1, rdev->sb_size,
+				       rdev->sb_start, rdev->sb_size,
 				       rdev->sb_page);
 			dprintk(KERN_INFO "(write) %s's sb offset: %llu\n",
 				bdevname(rdev->bdev,b),
-				(unsigned long long)rdev->sb_offset);
+				(unsigned long long)rdev->sb_start);
 			rdev->sb_events = mddev->events;
 
 		} else
@@ -3431,16 +3428,16 @@ static int do_md_run(mddev_t * mddev)
 		 * We don't want the data to overlap the metadata,
 		 * Internal Bitmap issues has handled elsewhere.
 		 */
-		if (rdev->data_offset < rdev->sb_offset) {
+		if (rdev->data_offset < rdev->sb_start / 2) {
 			if (mddev->size &&
 			    rdev->data_offset + mddev->size*2
-			    > rdev->sb_offset*2) {
+			    > rdev->sb_start) {
 				printk("md: %s: data overlaps metadata\n",
 				       mdname(mddev));
 				return -EINVAL;
 			}
 		} else {
-			if (rdev->sb_offset*2 + rdev->sb_size/512
+			if (rdev->sb_start + rdev->sb_size/512
 			    > rdev->data_offset) {
 				printk("md: %s: metadata overlaps data\n",
 				       mdname(mddev));
@@ -4211,9 +4208,9 @@ static int add_new_disk(mddev_t * mddev, mdu_disk_info_t *info)
 
 		if (!mddev->persistent) {
 			printk(KERN_INFO "md: nonpersistent superblock ...\n");
-			rdev->sb_offset = rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
+			rdev->sb_start = rdev->bdev->bd_inode->i_size / 512;
 		} else 
-			rdev->sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
+			rdev->sb_start = calc_dev_sboffset(rdev->bdev);
 		rdev->size = calc_num_sectors(rdev, mddev->chunk_size) / 2;
 
 		err = bind_rdev_to_array(rdev, mddev);
@@ -4283,10 +4280,9 @@ static int hot_add_disk(mddev_t * mddev, dev_t dev)
 	}
 
 	if (mddev->persistent)
-		rdev->sb_offset = calc_dev_sboffset(rdev->bdev) / 2;
+		rdev->sb_start = calc_dev_sboffset(rdev->bdev);
 	else
-		rdev->sb_offset =
-			rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
+		rdev->sb_start = rdev->bdev->bd_inode->i_size / 512;
 
 	rdev->size = calc_num_sectors(rdev, mddev->chunk_size) / 2;
 
@@ -4487,7 +4483,7 @@ static int update_size(mddev_t *mddev, sector_t num_sectors)
 	 * linear and raid0 always use whatever space is available. We can only
 	 * consider changing this number if no resync or reconstruction is
 	 * happening, and if the new size is acceptable. It must fit before the
-	 * sb_offset or, if that is <data_offset, it must fit before the size
+	 * sb_start or, if that is <data_offset, it must fit before the size
 	 * of each device.  If num_sectors is zero, we find the largest size
 	 * that fits.
 
diff --git a/include/linux/raid/md_k.h b/include/linux/raid/md_k.h
index 3dea9f5..3df92af 100644
--- a/include/linux/raid/md_k.h
+++ b/include/linux/raid/md_k.h
@@ -59,7 +59,7 @@ struct mdk_rdev_s
 	int		sb_loaded;
 	__u64		sb_events;
 	sector_t	data_offset;	/* start of data in array */
-	sector_t	sb_offset;
+	sector_t 	sb_start;	/* offset of the super block (in 512byte sectors) */
 	int		sb_size;	/* bytes in the superblock */
 	int		preferred_minor;	/* autorun support */
 
-- 
1.5.3.8

Re: [PATCH 4/5] md: Turn rdev->sb_offset into a sector-based quantity.

[Neil Brown]

On Thursday July 10, maan@systemlinux.org wrote:
> Rename it to sb_start to make sure all users have been converted.

Good idea.

> @@ -1047,22 +1045,21 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
>  	 */
>  	switch(minor_version) {
>  	case 0:
> -		sb_offset = rdev->bdev->bd_inode->i_size >> 9;
> -		sb_offset -= 8*2;
> -		sb_offset &= ~(sector_t)(4*2-1);
> +		sb_start = rdev->bdev->bd_inode->i_size >> 9;
> +		sb_start -= 8*2;
> +		sb_start &= ~(sector_t)(4*2-1);
>  		/* convert from sectors to K */
> -		sb_offset /= 2;
>  		break;

That "convert.." comment can go.

>  
>  	if (minor_version
> -	    && rdev->data_offset < sb_offset + (rdev->sb_size/512))
> +	    && rdev->data_offset < sb_start / 2 + (rdev->sb_size/512))
>  		return -EINVAL;

That doesn't look right.
  rdev->data_offset is in sectors.
  rdev->sb_size/512 is in sectors.
  sb_offset is in K.
We had a bug there.  Better fix it by dropping the "/ 2".

> @@ -3431,16 +3428,16 @@ static int do_md_run(mddev_t * mddev)
>  		 * We don't want the data to overlap the metadata,
>  		 * Internal Bitmap issues has handled elsewhere.
>  		 */
> -		if (rdev->data_offset < rdev->sb_offset) {
> +		if (rdev->data_offset < rdev->sb_start / 2) {

And another bug.  We don't want the "/ 2".  data_offset was always
sectors, and now sb_offset is too.
That's two bugs we've found by doing this conversion - thanks!


NeilBrown

Re: [PATCH 4/5] md: Turn rdev->sb_offset into a sector-based quantity.

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

On 21:15, Neil Brown wrote:
> >  	if (minor_version
> > -	    && rdev->data_offset < sb_offset + (rdev->sb_size/512))
> > +	    && rdev->data_offset < sb_start / 2 + (rdev->sb_size/512))
> >  		return -EINVAL;
> 
> That doesn't look right.
>   rdev->data_offset is in sectors.
>   rdev->sb_size/512 is in sectors.
>   sb_offset is in K.
> We had a bug there.  Better fix it by dropping the "/ 2".
> 
> > @@ -3431,16 +3428,16 @@ static int do_md_run(mddev_t * mddev)
> >  		 * We don't want the data to overlap the metadata,
> >  		 * Internal Bitmap issues has handled elsewhere.
> >  		 */
> > -		if (rdev->data_offset < rdev->sb_offset) {
> > +		if (rdev->data_offset < rdev->sb_start / 2) {
> 
> And another bug.  We don't want the "/ 2".  data_offset was always
> sectors, and now sb_offset is too.

It might be worth to get fixes for these two bugs into -stable.

Here's a patch against mainline. Please forward to stable@... if
you agree.

Andre

commit c1be9a2d52dd2db1cf284e62d5e1cb8423482fb4
Author: Andre Noll <maan@systemlinux.org>
Date:   Thu Jul 17 11:33:40 2008 +0200

    md: Fix two bugs in md.c
    
    sb_offset is kilobytes while data_offset and sb_size/512 are in
    sectors.
    
    Spotted by Neil while reviewing the sector_t conversion patches.

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2580ac1..0c7aeb9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1113,7 +1113,7 @@ static int super_1_load(mdk_rdev_t *rdev, mdk_rdev_t *refdev, int minor_version)
 		rdev->sb_size = (rdev->sb_size | bmask) + 1;
 
 	if (minor_version
-	    && rdev->data_offset < sb_offset + (rdev->sb_size/512))
+	    && rdev->data_offset < sb_offset * 2 + (rdev->sb_size/512))
 		return -EINVAL;
 
 	if (sb->level == cpu_to_le32(LEVEL_MULTIPATH))
@@ -3432,7 +3432,7 @@ static int do_md_run(mddev_t * mddev)
 		 * We don't want the data to overlap the metadata,
 		 * Internal Bitmap issues has handled elsewhere.
 		 */
-		if (rdev->data_offset < rdev->sb_offset) {
+		if (rdev->data_offset < rdev->sb_offset * 2) {
 			if (mddev->size &&
 			    rdev->data_offset + mddev->size*2
 			    > rdev->sb_offset*2) {
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/5] md: Turn rdev->sb_offset into a sector-based quantity.

[Neil Brown]

On Thursday July 17, maan@systemlinux.org wrote:
> 
> It might be worth to get fixes for these two bugs into -stable.
> 
> Here's a patch against mainline. Please forward to stable@... if
> you agree.

A question definitely worth considering.

I think in this case they don't qualify.  The bug are in sanity
checks, and they make the checks too weak.  They don't actually cause
incorrect behaviour in any credible scenario.

As the bugs don't open a security hole, don't cause data corruption,
and don't trigger an 'oops', they don't really qualify for -stable.

Thanks,
NeilBrown

[PATCH 5/5] md: Remove some unused macros.

[Andre Noll]

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 include/linux/raid/md_p.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/include/linux/raid/md_p.h b/include/linux/raid/md_p.h
index 3f2cd98..8b4de4a 100644
--- a/include/linux/raid/md_p.h
+++ b/include/linux/raid/md_p.h
@@ -43,14 +43,11 @@
  */
 #define MD_RESERVED_BYTES		(64 * 1024)
 #define MD_RESERVED_SECTORS		(MD_RESERVED_BYTES / 512)
-#define MD_RESERVED_BLOCKS		(MD_RESERVED_BYTES / BLOCK_SIZE)
 
 #define MD_NEW_SIZE_SECTORS(x)		((x & ~(MD_RESERVED_SECTORS - 1)) - MD_RESERVED_SECTORS)
-#define MD_NEW_SIZE_BLOCKS(x)		((x & ~(MD_RESERVED_BLOCKS - 1)) - MD_RESERVED_BLOCKS)
 
 #define MD_SB_BYTES			4096
 #define MD_SB_WORDS			(MD_SB_BYTES / 4)
-#define MD_SB_BLOCKS			(MD_SB_BYTES / BLOCK_SIZE)
 #define MD_SB_SECTORS			(MD_SB_BYTES / 512)
 
 /*
-- 
1.5.3.8

Re: [PATCH 0/5] md: sector_t conversions

[Neil Brown]

On Thursday July 10, maan@systemlinux.org wrote:
> Hi
> 
> here is the first batch of patches that aim to change the internal
> representations of sizes of raid devices from 1K blocks to 512 byte
> sectors.
> 
> Patches 1-3 change the internal functions update_size(),
> calc_dev_size() and calc_dev_sboffset() so that they take/return a
> sector count instead of a block count. Patch 4 changes the semantics
> of rdev->sb_offset so that its value now represents the number of the
> start sector instead of the start block. Patch 5 removes some macros
> that have become unused due to this conversion.

Thanks.  I've pushed all of these (with some changes as mentioned in
earlier emails) into my for-next tree
   git://neil.brown.name/md/ for-next
   http://neil.brown.name/git?p=md;a=shortlog;h=refs/heads/for-next

Thanks,
NeilBrown