xfs
[Top] [All Lists]

Re: [PATCH] xfs: split xfs_bmap_btalloc

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: split xfs_bmap_btalloc
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 17 Feb 2010 15:37:21 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100215233442.GA28817@xxxxxxxxxxxxx>
References: <20100215233442.GA28817@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Feb 15, 2010 at 06:34:42PM -0500, Christoph Hellwig wrote:
> Split out the nullfb case into a separate function to reduce the stack
> footprint and make the code more readable.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

I'm not sure this does reduce stack footprint - whenever we allocate
the first block of a file we hit the nullfb case, and this adds
another function call to that stack.

However, from the cleanup POV, I can't complain ;)

> 
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c        2010-01-17 14:46:43.326254284 +0100
> +++ xfs/fs/xfs/xfs_bmap.c     2010-01-17 15:04:47.016005768 +0100
> @@ -2550,22 +2550,134 @@ xfs_bmap_rtalloc(
>  }
>  
>  STATIC int
> +xfs_bmap_btalloc_nullfb(
> +     struct xfs_bmalloca     *ap,
> +     struct xfs_alloc_arg    *args,
> +     xfs_extlen_t            *blen)
> +{
> +     struct xfs_mount        *mp = ap->ip->i_mount;
> +     struct xfs_perag        *pag;
> +     xfs_agnumber_t          ag, startag;
> +     int                     notinit = 0;
> +     int                     error;
> +
> +     if (ap->userdata && xfs_inode_is_filestream(ap->ip))
> +             args->type = XFS_ALLOCTYPE_NEAR_BNO;
> +     else
> +             args->type = XFS_ALLOCTYPE_START_BNO;
> +     args->total = ap->total;
> +
> +     /*
> +      * Search for an allocation group with a single extent large enough
> +      * for the request.  If one isn't found, then adjust the minimum
> +      * allocation size to the largest space found.
> +      */
> +     startag = ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> +     if (startag == NULLAGNUMBER)
> +             startag = ag = 0;
> +
> +     pag = xfs_perag_get(mp, ag);
> +     while (*blen < ap->alen) {
> +             if (!pag->pagf_init) {
> +                     error = xfs_alloc_pagf_init(mp, args->tp, ag,
> +                                                 XFS_ALLOC_FLAG_TRYLOCK);
> +                     if (error) {
> +                             xfs_perag_put(pag);
> +                             return error;
> +                     }
> +             }
> +
> +             /*
> +              * See xfs_alloc_fix_freelist...
> +              */

This comment is pretty much useless - if we are changing code then
I'd say kill it. Also the other comments in this function could be
reformatted as the indent has changed and so might be a bit more
readable with longer lines. This is not really that critical,
though, so it's good as it stands.

Reviewed-by: Dave Chinner <david@xxxxxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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