xfs
[Top] [All Lists]

Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum

To: aelder@xxxxxxx
Subject: Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Tue, 20 Sep 2011 15:03:16 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Boris Ranto <branto@xxxxxxxxxx>
In-reply-to: <1316548828.2912.48.camel@doink>
References: <4E77B7E2.6020805@xxxxxxxxxx> <1316548828.2912.48.camel@doink>
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc13 Lightning/1.0b3pre Thunderbird/3.1.10
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@xxxxxxxxxx>
>> ---
> 
> 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@xxxxxxx>
> 
>> 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@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> 
> 

<Prev in Thread] Current Thread [Next in Thread>