[REVIEW] fix mkfs.xfs agcount between 64 and 128MB

[REVIEW] fix mkfs.xfs agcount between 64 and 128MB

[Barry Naujok]

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

With the AG count = 4 changes for a single disk, device sizes between 64MB  
and 128MB still scale from 4 to 8 AGs. This is causing some questions!

The attached patch enforces 4 AGs for devices between 64 and 128MB  
(including multi-disk setups, which should be extremely rare at this  
size). This means the AG size will vary from 16 to 32MB depending on the  
device size.

Barry.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix_agcount_in_mkfs_for_small_sizes.patch --]
[-- Type: text/x-patch; name=fix_agcount_in_mkfs_for_small_sizes.patch, Size: 1336 bytes --]

Index: ci/xfsprogs/mkfs/xfs_mkfs.c
===================================================================
--- ci.orig/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 14:36:56.000000000 +1100
+++ ci/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 14:39:38.249637450 +1100
@@ -412,7 +412,7 @@
 	/*
 	 * First handle the extremes - the points at which we will
 	 * always use the maximum AG size, the points at which we
-	 * always use the minimum, and a "small-step" for 16-128MB.
+	 * always use the minimum, and a "small-step" for 16-64MB.
 	 *
 	 * These apply regardless of storage configuration.
 	 */
@@ -423,13 +423,13 @@
 		blocks = dblocks;
 		count = 1;
 		goto done;
-	} else if (dblocks < MEGABYTES(128, blocklog)) {
+	} else if (dblocks < MEGABYTES(64, blocklog)) {
 		blocks = MEGABYTES(16, blocklog);
 		goto done;
 	}
 
 	/*
-	 * Sizes between 128MB and 32TB:
+	 * Sizes between 64MB and 32TB:
 	 *
 	 * For the remainder we choose an AG size based on the
 	 * number of data blocks available, trying to keep the
@@ -444,7 +444,7 @@
 	 * scale up smoothly between min/max AG sizes.
 	 */
 
-	if (!multidisk) {
+	if (!multidisk || dblocks < MEGABYTES(128, blocklog)) {
 		if (dblocks >= TERABYTES(4, blocklog)) {
                         blocks = XFS_AG_MAX_BLOCKS(blocklog);
                         goto done;

Re: [REVIEW] fix mkfs.xfs agcount between 64 and 128MB

[Niv Sardi]

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


What's wrong with this:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mkfs.diff --]
[-- Type: text/x-diff, Size: 376 bytes --]

diff --git a/xfsprogs/mkfs/xfs_mkfs.c b/xfsprogs/mkfs/xfs_mkfs.c
index e15d667..13884f9 100644
--- a/xfsprogs/mkfs/xfs_mkfs.c
+++ b/xfsprogs/mkfs/xfs_mkfs.c
@@ -422,7 +422,7 @@ calc_default_ag_geometry(
 		count = 1;
 		goto done;
 	} else if (dblocks < MEGABYTES(128, blocklog)) {
-		blocks = MEGABYTES(16, blocklog);
+		blocks = MEGABYTES(32, blocklog);
 		goto done;
 	}
 

[-- Attachment #3: Type: text/plain, Size: 15 bytes --]


-- 
Niv Sardi

Re: [REVIEW] fix mkfs.xfs agcount between 64 and 128MB

[Barry Naujok]

On Mon, 25 Feb 2008 15:21:25 +1100, Niv Sardi <xaiki@cxhome.ath.cx> wrote:

>
> What's wrong with this:


diff --git a/xfsprogs/mkfs/xfs_mkfs.c b/xfsprogs/mkfs/xfs_mkfs.c
index e15d667..13884f9 100644
--- a/xfsprogs/mkfs/xfs_mkfs.c
+++ b/xfsprogs/mkfs/xfs_mkfs.c
@@ -422,7 +422,7 @@ calc_default_ag_geometry(
  		count = 1;
  		goto done;
  	} else if (dblocks < MEGABYTES(128, blocklog)) {
-		blocks = MEGABYTES(16, blocklog);
+		blocks = MEGABYTES(32, blocklog);
  		goto done;
  	}

And what happens between 16 and 32MB?

[REVIEW #2] fix mkfs.xfs agcount between 16 and 128MB

[Barry Naujok]

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

On Mon, 25 Feb 2008 14:45:32 +1100, Barry Naujok <bnaujok@sgi.com> wrote:

> With the AG count = 4 changes for a single disk, device sizes between  
> 64MB
> and 128MB still scale from 4 to 8 AGs. This is causing some questions!
>
> The attached patch enforces 4 AGs for devices between 64 and 128MB
> (including multi-disk setups, which should be extremely rare at this
> size). This means the AG size will vary from 16 to 32MB depending on the
> device size.
>
> Barry.

With more questioning and playing, it seems < 64MB wasn't exactly optimal
either. Basically, the size was always rounded to a multiple of 16MB
(which is the minimum AG size).

The new patch basically enforces agcount=2 for 32 to 64MB and agcount=1
for less than 32MB.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix_agcount_in_mkfs_for_small_sizes.patch --]
[-- Type: text/x-patch; name=fix_agcount_in_mkfs_for_small_sizes.patch, Size: 2541 bytes --]

Index: ci/xfsprogs/mkfs/xfs_mkfs.c
===================================================================
--- ci.orig/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 14:36:56.000000000 +1100
+++ ci/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 17:59:53.722217734 +1100
@@ -410,27 +410,17 @@
 	int		shift = 0;
 
 	/*
-	 * First handle the extremes - the points at which we will
-	 * always use the maximum AG size, the points at which we
-	 * always use the minimum, and a "small-step" for 16-128MB.
+	 * First handle the high extreme - the point at which we will
+	 * always use the maximum AG size.
 	 *
 	 * These apply regardless of storage configuration.
 	 */
 	if (dblocks >= TERABYTES(32, blocklog)) {
 		blocks = XFS_AG_MAX_BLOCKS(blocklog);
 		goto done;
-	} else if (dblocks < MEGABYTES(16, blocklog)) {
-		blocks = dblocks;
-		count = 1;
-		goto done;
-	} else if (dblocks < MEGABYTES(128, blocklog)) {
-		blocks = MEGABYTES(16, blocklog);
-		goto done;
 	}
 
 	/*
-	 * Sizes between 128MB and 32TB:
-	 *
 	 * For the remainder we choose an AG size based on the
 	 * number of data blocks available, trying to keep the
 	 * number of AGs relatively small (especially compared
@@ -439,25 +429,29 @@
 	 * count can be increased by growfs, so prefer to use
 	 * smaller counts at mkfs time.
 	 *
-	 * For a single underlying storage device less than 4TB
-	 * in size, just use 4 AGs, otherwise (for JBOD/RAIDs)
-	 * scale up smoothly between min/max AG sizes.
+	 * For a single underlying storage device between 128MB
+	 * and 4TB in size, just use 4 AGs, otherwise scale up
+	 * smoothly between min/max AG sizes.
 	 */
 
-	if (!multidisk) {
+	if (!multidisk && dblocks >= MEGABYTES(128, blocklog)) {
 		if (dblocks >= TERABYTES(4, blocklog)) {
-                        blocks = XFS_AG_MAX_BLOCKS(blocklog);
-                        goto done;
-                }
-                shift = 2;
-        } else if (dblocks > GIGABYTES(512, blocklog))
+			blocks = XFS_AG_MAX_BLOCKS(blocklog);
+			goto done;
+		}
+		shift = 2;
+	} else if (dblocks > GIGABYTES(512, blocklog))
 		shift = 5;
 	else if (dblocks > GIGABYTES(8, blocklog))
 		shift = 4;
 	else if (dblocks >= MEGABYTES(128, blocklog))
 		shift = 3;
+	else if (dblocks >= MEGABYTES(64, blocklog))
+		shift = 2;
+	else if (dblocks >= MEGABYTES(32, blocklog))
+		shift = 1;
 	else
-		ASSERT(0);
+		shift = 0;
 	/*
 	 * If dblocks is not evenly divisible by the number of
 	 * desired AGs, round "blocks" up so we don't lose the

[REVIEW #3] fix mkfs.xfs agcount between 16 and 128MB

[Barry Naujok]

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

On Mon, 25 Feb 2008 18:03:31 +1100, Barry Naujok <bnaujok@sgi.com> wrote:

> On Mon, 25 Feb 2008 14:45:32 +1100, Barry Naujok <bnaujok@sgi.com> wrote:
>
>> With the AG count = 4 changes for a single disk, device sizes between
>> 64MB
>> and 128MB still scale from 4 to 8 AGs. This is causing some questions!
>>
>> The attached patch enforces 4 AGs for devices between 64 and 128MB
>> (including multi-disk setups, which should be extremely rare at this
>> size). This means the AG size will vary from 16 to 32MB depending on the
>> device size.
>>
>> Barry.
>
> With more questioning and playing, it seems < 64MB wasn't exactly optimal
> either. Basically, the size was always rounded to a multiple of 16MB
> (which is the minimum AG size).
>
> The new patch basically enforces agcount=2 for 32 to 64MB and agcount=1
> for less than 32MB.

Ok, one more change (the last I hope!!)

With the last patch, the "count" variable becomes redundant. This patch
(which supercedes the last two patches) eliminates the count variable.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix_agcount_in_mkfs_for_small_sizes.patch --]
[-- Type: text/x-patch; name=fix_agcount_in_mkfs_for_small_sizes.patch, Size: 2977 bytes --]

Index: ci/xfsprogs/mkfs/xfs_mkfs.c
===================================================================
--- ci.orig/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 14:36:56.000000000 +1100
+++ ci/xfsprogs/mkfs/xfs_mkfs.c	2008-02-25 18:05:36.581883359 +1100
@@ -406,31 +406,20 @@
 	__uint64_t	*agcount)
 {
 	__uint64_t	blocks = 0;
-	__uint64_t	count = 0;
 	int		shift = 0;
 
 	/*
-	 * First handle the extremes - the points at which we will
-	 * always use the maximum AG size, the points at which we
-	 * always use the minimum, and a "small-step" for 16-128MB.
+	 * First handle the high extreme - the point at which we will
+	 * always use the maximum AG size.
 	 *
-	 * These apply regardless of storage configuration.
+	 * This applies regardless of storage configuration.
 	 */
 	if (dblocks >= TERABYTES(32, blocklog)) {
 		blocks = XFS_AG_MAX_BLOCKS(blocklog);
 		goto done;
-	} else if (dblocks < MEGABYTES(16, blocklog)) {
-		blocks = dblocks;
-		count = 1;
-		goto done;
-	} else if (dblocks < MEGABYTES(128, blocklog)) {
-		blocks = MEGABYTES(16, blocklog);
-		goto done;
 	}
 
 	/*
-	 * Sizes between 128MB and 32TB:
-	 *
 	 * For the remainder we choose an AG size based on the
 	 * number of data blocks available, trying to keep the
 	 * number of AGs relatively small (especially compared
@@ -439,25 +428,29 @@
 	 * count can be increased by growfs, so prefer to use
 	 * smaller counts at mkfs time.
 	 *
-	 * For a single underlying storage device less than 4TB
-	 * in size, just use 4 AGs, otherwise (for JBOD/RAIDs)
-	 * scale up smoothly between min/max AG sizes.
+	 * For a single underlying storage device between 128MB
+	 * and 4TB in size, just use 4 AGs, otherwise scale up
+	 * smoothly between min/max AG sizes.
 	 */
 
-	if (!multidisk) {
+	if (!multidisk && dblocks >= MEGABYTES(128, blocklog)) {
 		if (dblocks >= TERABYTES(4, blocklog)) {
-                        blocks = XFS_AG_MAX_BLOCKS(blocklog);
-                        goto done;
-                }
-                shift = 2;
-        } else if (dblocks > GIGABYTES(512, blocklog))
+			blocks = XFS_AG_MAX_BLOCKS(blocklog);
+			goto done;
+		}
+		shift = 2;
+	} else if (dblocks > GIGABYTES(512, blocklog))
 		shift = 5;
 	else if (dblocks > GIGABYTES(8, blocklog))
 		shift = 4;
 	else if (dblocks >= MEGABYTES(128, blocklog))
 		shift = 3;
+	else if (dblocks >= MEGABYTES(64, blocklog))
+		shift = 2;
+	else if (dblocks >= MEGABYTES(32, blocklog))
+		shift = 1;
 	else
-		ASSERT(0);
+		shift = 0;
 	/*
 	 * If dblocks is not evenly divisible by the number of
 	 * desired AGs, round "blocks" up so we don't lose the
@@ -467,11 +460,8 @@
 	blocks = (dblocks >> shift) + ((dblocks & xfs_mask32lo(shift)) != 0);
 
 done:
-	if (!count)
-		count = dblocks / blocks + (dblocks % blocks != 0);
-
 	*agsize = blocks;
-	*agcount = count;
+	*agcount = dblocks / blocks + (dblocks % blocks != 0);
 }
 
 static void

Re: [REVIEW #3] fix mkfs.xfs agcount between 16 and 128MB

[Niv Sardi]

The logics looks good to me, but you might want someone with a better
understanding of 〞what's to be done〞 than me…

-- 
Niv Sardi