xfs
[Top] [All Lists]

Re: [PATCH 2/4] xfs: push rounddown_pow_of_two() to after prealloc throt

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: push rounddown_pow_of_two() to after prealloc throttle
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 13 Dec 2012 11:39:13 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1354726078-31793-3-git-send-email-bfoster@xxxxxxxxxx>
References: <1354726078-31793-1-git-send-email-bfoster@xxxxxxxxxx> <1354726078-31793-3-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Dec 05, 2012 at 11:47:56AM -0500, Brian Foster wrote:
> The round down occurs towards the beginning of the function. Push
> it down after throttling has occurred. This is to support adding
> further transformations to 'alloc_blocks' that might not preserve
> power-of-two alignment (and thus could lead to rounding down
> multiple times).
> 
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_iomap.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index bd7c060..d381326 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -329,13 +329,11 @@ xfs_iomap_prealloc_size(
>               goto check_writeio;
>  
>       /*
> -      * rounddown_pow_of_two() returns an undefined result
> -      * if we pass in alloc_blocks = 0. Hence the "+ 1" to
> -      * ensure we always pass in a non-zero value.
> +      * MAXEXTLEN is 21 bits, add one to protect against the rounddown
> +      * further down.
>        */
> -     alloc_blocks = XFS_B_TO_FSB(mp, XFS_ISIZE(ip)) + 1;
> -     alloc_blocks = XFS_FILEOFF_MIN(MAXEXTLEN,
> -                             rounddown_pow_of_two(alloc_blocks));
> +     alloc_blocks = XFS_FILEOFF_MIN(MAXEXTLEN + 1,
> +                             XFS_B_TO_FSB(mp, XFS_ISIZE(ip)));

I suspect this is will lead to a bug - if the round down doesn't
modify the value when it is MAXEXTLEN + 1, then we are returning a
value greater than MAXEXTLEN to the caller....

>       xfs_icsb_sync_counters(mp, XFS_ICSB_LAZY_COUNT);
>       freesp = mp->m_sb.sb_fdblocks;
> @@ -352,6 +350,8 @@ xfs_iomap_prealloc_size(
>       }
>       if (shift)
>               alloc_blocks >>= shift;
> +     if (alloc_blocks)
> +             alloc_blocks = rounddown_pow_of_two(alloc_blocks);

This needs the comment about rounddown_pow_of_two() and zero values.

It then needs to cap alloc_blocks to MAXEXTLEN, because it can
clearly be larger thanks to the above (MAXEXTLEN + 1) code.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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