public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mkfs.xfs: don't increase agblocks past maximum
@ 2011-09-19 21:45 Eric Sandeen
  2011-09-20 20:00 ` Alex Elder
  2011-09-20 20:30 ` Christoph Hellwig
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Sandeen @ 2011-09-19 21:45 UTC (permalink / raw)
  To: xfs-oss; +Cc: Boris Ranto

RH QA discovered this bug:

Steps to Reproduce:
1. Create 4 TB - 1 B partition
dd if=/dev/zero of=x.img bs=1 count=0 seek=4398046511103
2. Create xfs fs with 512 B block size on the partition
mkfs.xfs -b size=512 xfs.img

Actual results:
Agsize is computed incorrectly resulting in fs creation fail:
agsize (2147483648b) too big, maximum is 2147483647 blocks

This is due to the "rounding up" at the very end of the calculations;
there may be other places to alleviate the problem, but it seems
most obvious to simply skip the rounding up if it would create too
many blocks in the AG.  Worst case, we lose 1 block per AG.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 5b3b9a7..856a261 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -658,7 +659,9 @@ calc_default_ag_geometry(
 	 * last bit of the filesystem. The same principle applies
 	 * to the AG count, so we don't lose the last AG!
 	 */
-	blocks = (dblocks >> shift) + ((dblocks & xfs_mask32lo(shift)) != 0);
+	blocks = (dblocks >> shift);
+	if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
+		blocks += ((dblocks & xfs_mask32lo(shift)) != 0);
 
 done:
 	*agsize = blocks;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
  2011-09-19 21:45 [PATCH] mkfs.xfs: don't increase agblocks past maximum Eric Sandeen
@ 2011-09-20 20:00 ` Alex Elder
  2011-09-20 20:03   ` Eric Sandeen
  2011-09-20 20:30 ` Christoph Hellwig
  1 sibling, 1 reply; 5+ messages in thread
From: Alex Elder @ 2011-09-20 20:00 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Boris Ranto, xfs-oss

On Mon, 2011-09-19 at 16:45 -0500, Eric Sandeen wrote:
> RH QA discovered this bug:
> 
> Steps to Reproduce:
> 1. Create 4 TB - 1 B partition
> dd if=/dev/zero of=x.img bs=1 count=0 seek=4398046511103
> 2. Create xfs fs with 512 B block size on the partition
> mkfs.xfs -b size=512 xfs.img
> 
> Actual results:
> Agsize is computed incorrectly resulting in fs creation fail:
> agsize (2147483648b) too big, maximum is 2147483647 blocks
> 
> This is due to the "rounding up" at the very end of the calculations;
> there may be other places to alleviate the problem, but it seems
> most obvious to simply skip the rounding up if it would create too
> many blocks in the AG.  Worst case, we lose 1 block per AG.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

The fix is the right way about it.

This may seem petty, but I think this would be better:

	blocks = dblocks >> shift
	if (blocks & xfs_mask32lo(shift)) {
		if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
			blocks++;
	}

It emphasizes more why we'd be doing the increment,
plus I'd rather see a "real" increment rather than
adding a Boolean value.

Either way:
Reviewed-by: Alex Elder <aelder@sgi.com>

> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5b3b9a7..856a261 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -658,7 +659,9 @@ calc_default_ag_geometry(
>  	 * last bit of the filesystem. The same principle applies
>  	 * to the AG count, so we don't lose the last AG!
>  	 */
> -	blocks = (dblocks >> shift) + ((dblocks & xfs_mask32lo(shift)) != 0);
> +	blocks = (dblocks >> shift);
> +	if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
> +		blocks += ((dblocks & xfs_mask32lo(shift)) != 0);
>  
>  done:
>  	*agsize = blocks;
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
  2011-09-20 20:00 ` Alex Elder
@ 2011-09-20 20:03   ` Eric Sandeen
  2011-09-20 20:04     ` Alex Elder
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2011-09-20 20:03 UTC (permalink / raw)
  To: aelder; +Cc: Boris Ranto, xfs-oss

On 09/20/2011 03:00 PM, Alex Elder wrote:
> On Mon, 2011-09-19 at 16:45 -0500, Eric Sandeen wrote:
>> RH QA discovered this bug:
>>
>> Steps to Reproduce:
>> 1. Create 4 TB - 1 B partition
>> dd if=/dev/zero of=x.img bs=1 count=0 seek=4398046511103
>> 2. Create xfs fs with 512 B block size on the partition
>> mkfs.xfs -b size=512 xfs.img
>>
>> Actual results:
>> Agsize is computed incorrectly resulting in fs creation fail:
>> agsize (2147483648b) too big, maximum is 2147483647 blocks
>>
>> This is due to the "rounding up" at the very end of the calculations;
>> there may be other places to alleviate the problem, but it seems
>> most obvious to simply skip the rounding up if it would create too
>> many blocks in the AG.  Worst case, we lose 1 block per AG.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
> 
> The fix is the right way about it.
> 
> This may seem petty, but I think this would be better:
> 
> 	blocks = dblocks >> shift
> 	if (blocks & xfs_mask32lo(shift)) {
> 		if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
> 			blocks++;
> 	}
> 
> It emphasizes more why we'd be doing the increment,
> plus I'd rather see a "real" increment rather than
> adding a Boolean value.

Yes, that's probably better.  More code change ... making it more readable.

I'll check it in that way (or, maybe you can, since I can't reach the
git repo)?

Thanks,
-Eric

> Either way:
> Reviewed-by: Alex Elder <aelder@sgi.com>
> 
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 5b3b9a7..856a261 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -658,7 +659,9 @@ calc_default_ag_geometry(
>>  	 * last bit of the filesystem. The same principle applies
>>  	 * to the AG count, so we don't lose the last AG!
>>  	 */
>> -	blocks = (dblocks >> shift) + ((dblocks & xfs_mask32lo(shift)) != 0);
>> +	blocks = (dblocks >> shift);
>> +	if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
>> +		blocks += ((dblocks & xfs_mask32lo(shift)) != 0);
>>  
>>  done:
>>  	*agsize = blocks;
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
  2011-09-20 20:03   ` Eric Sandeen
@ 2011-09-20 20:04     ` Alex Elder
  0 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2011-09-20 20:04 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Boris Ranto, xfs-oss

On Tue, 2011-09-20 at 15:03 -0500, Eric Sandeen wrote:
> Yes, that's probably better.  More code change ... making it more
> readable.
> 
> I'll check it in that way (or, maybe you can, since I can't reach the
> git repo)? 

OK, I will check it into oss.sgi.com using the code
the way I suggested.

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
  2011-09-19 21:45 [PATCH] mkfs.xfs: don't increase agblocks past maximum Eric Sandeen
  2011-09-20 20:00 ` Alex Elder
@ 2011-09-20 20:30 ` Christoph Hellwig
  1 sibling, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2011-09-20 20:30 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Boris Ranto, xfs-oss

On Mon, Sep 19, 2011 at 04:45:06PM -0500, Eric Sandeen wrote:
> RH QA discovered this bug:
> 
> Steps to Reproduce:
> 1. Create 4 TB - 1 B partition
> dd if=/dev/zero of=x.img bs=1 count=0 seek=4398046511103
> 2. Create xfs fs with 512 B block size on the partition
> mkfs.xfs -b size=512 xfs.img

Can we please get this into xfstests, just doing the mkfs on a
sparse file should be enough.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-20 20:30 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-19 21:45 [PATCH] mkfs.xfs: don't increase agblocks past maximum Eric Sandeen
2011-09-20 20:00 ` Alex Elder
2011-09-20 20:03   ` Eric Sandeen
2011-09-20 20:04     ` Alex Elder
2011-09-20 20:30 ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox