Re: [PATCH 4/8] xfs: limit extent length for allocation to AG size

[Alex Elder <aelder@sgi.com>]

On Tue, 2011-01-25 at 19:50 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Delayed allocation extents can be larger than AGs, so when trying to
> convert a large range we may scan every AG inside
> xfs_bmap_alloc_nullfb() trying to find an AG with a size larger than
> an AG. We should stop when we find the first AG with a maximum
> possible allocation size. This causes excessive CPU usage when there
> are lots of AGs.
> 
> The same problem occurs when doing preallocation of a range larger
> than an AG.
> 
> Fix the problem by limiting real allocation lengths to the maximum
> that an AG can support. This means if we have empty AGs, we'll stop
> the search at the first of them. If there are no empty AGs, we'll
> still scan them all, but that is a different problem....

Maybe I'm wrong but I think you need to change a "+"
to a "-" (shown below).

And I have a few really minor suggestions:
- You should update a comment (which I point
  out below) to match your change.
- Maybe make use of a local variable, at least
  in xfs_bmap_btalloc_nullfb(), such as:
    xfs_extlen_t requested = args->maxlen;

Otherwise it looks good to me.

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

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_alloc.h |   16 ++++++++++++++++
>  fs/xfs/xfs_bmap.c  |   16 +++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc.h b/fs/xfs/xfs_alloc.h
> index 0ab56b3..6ad45b9 100644
> --- a/fs/xfs/xfs_alloc.h
> +++ b/fs/xfs/xfs_alloc.h
> @@ -75,6 +75,22 @@ typedef unsigned int xfs_alloctype_t;
>  #define XFS_ALLOC_SET_ASIDE(mp)  (4 + ((mp)->m_sb.sb_agcount * 4))
>  
>  /*
> + * When deciding how much space to allocate out of an AG, we limit the
> + * allocation maximum size to the size the AG. However, we cannot use all the
> + * blocks in the AG - some are permanently used by metadata. These
> + * blocks are generally:
> + *	- the AG superblock, AGF, AGI and AGFL
> + *	- the AGF (bno and cnt) and AGI btree root blocks
> + *	- 4 blocks on the AGFL according to XFS_ALLOC_SET_ASIDE() limits
> + *
> + * The AG headers are sector sized, so the amount of space they take up is
> + * dependent on filesystem geometry. The others are all single blocks.
> + */
> +#define XFS_ALLOC_AG_MAX_USABLE(mp)	\
> +	((mp)->m_sb.sb_agblocks - XFS_BB_TO_FSB(mp, XFS_FSS_TO_BB(mp, 4)) + 7)

Is this right?  Shouldn't the 7 be subtracted (or combined using
parentheses with the 4 FS sectors)?

> +
> +
> +/*
>   * Argument structure for xfs_alloc routines.
>   * This is turned into a structure to avoid having 20 arguments passed
>   * down several levels of the stack.
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 4111cd3..74861c6 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
. . .
> @@ -2498,14 +2498,14 @@ xfs_bmap_btalloc_nullfb(
>  	 * If the best seen length is less than the request
>  	 * length, use the best as the minimum.
>  	 */
> -	else if (*blen < ap->alen)
> +	else if (*blen < args->maxlen)
>  		args->minlen = *blen;
>  	/*
>  	 * Otherwise we've seen an extent as big as alen,

Ought to adjust this comment to better reflect your updated
code ("alen" doesn't really fit any more).

>  	 * use that as the minimum.
>  	 */
>  	else
> -		args->minlen = ap->alen;
> +		args->minlen = args->maxlen;
>  
>  	/*
>  	 * set the failure fallback case to look in the selected


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