xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] mkfs.xfs: don't increase agblocks past maximum
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 20 Sep 2011 15:00:28 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Boris Ranto <branto@xxxxxxxxxx>
In-reply-to: <4E77B7E2.6020805@xxxxxxxxxx>
References: <4E77B7E2.6020805@xxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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.

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>