xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

To: Dhruvesh Rathore <adrscube@xxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 28 Jan 2015 12:57:57 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <54c1c12e.230a460a.4729.11fc@xxxxxxxxxxxxx>
References: <54c1c12e.230a460a.4729.11fc@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Jan 23, 2015 at 09:04:00AM +0530, Dhruvesh Rathore wrote:
> Here is the original discussion that Dave wrote for xfs_spaceman:
> 
> http://oss.sgi.com/archives/xfs/2012-10/msg00363.html
> 
> These patches were not posted and were just forward ported to a current 
> 3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in Dave's
> kernel tree at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git
> 
> The original xfs_spaceman tool that Dave wrote is here:
> 
> http://oss.sgi.com/archives/xfs/2012-10/msg00366.html
> 
> Dave just updated it to the 3.2.2 code base and pushed it to the
> spaceman branch in this tree:
> 
> git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git

As per last patch, this is series description so it doesn't need to
be repeated in every patch ;)

>From here:

> This xfs_spaceman utility previously failed to account for AGFL blocks.
> Old output (Before changes).
> 
> $ sudo xfs_spaceman -c "freesp" /media/xfs
>    from      to extents  blocks    pct
>    1024    2047       1    1262   0.04
>    4096    8191       1    5126   0.15
>    8192   16383       3   35344   1.05
>   32768   65535       1   43989   1.31
>  262144  524287       3 1334894  39.78
>  524288  967428       2 1934840  57.66
> 
> As you can see the AGFL blocks were unaccounted (4 per AG, and there 
> were 4 AGs in this filesystem).
> 
> This patch is concerned with adding a new function which will walk the free
> extents in AGFL and account for these AGFL blocks, while file system scanning.
> 
> New output (After implementing this patch).
> 
> $ uname -a
> Linux dhruv-MacBookAir 3.18.0-rc7+ #3 SMP Thu Dec 25 15:29:59 IST 2014 x86_64 
> x86_64 x86_64 GNU/Linux
> $ sudo xfs_spaceman -V
> xfs_spaceman version 3.2.2
> $ sudo xfs_spaceman -c "freesp" /media/xfs
>    from      to extents  blocks    pct
>       1       1      16      16   0.00
>    1024    2047       1    1262   0.04
>    4096    8191       1    5126   0.15
>    8192   16383       3   35344   1.05
>   32768   65535       1   43989   1.31
>  262144  524287       3 1334894  39.78
>  524288  967428       2 1934840  57.66

to here, this is a good patch description. :)

> Please do comment.

No need to ask in each patch description - the series description if
the place for that....

> +
> +/*
> + * Walk the free extents in the AGFL (AG Free List) of each AG, and dump

No need to explain what AGFL means - anyone reading the code already
knows....

> + * them all into the fieinfo.
> + * 
> + * With a freshly made filesystem, 4 blocks are reserved immediately after
> + * the free space B+tree root blocks (blocks 4 to 7). As they are used up

The number of blocks is actually dependent on filesystem geometry,
but again there isn't a need to describe the exact workings of the
AGFL here.

> + * additional blocks are reserved from the AG and added to the free list
> + * array. A typical device will have space for 128 elements in the array.
> + * The actual size can be determined using XFS_AGFL_SIZE macro. The array
> + * is maintained as a circular list and active elements are pointed by 
> + * AGF's agf_flfirst, agf_fllast and agf_flcount values.

Yup, you're describing the code, not /why/ the function exists.

/*
 * When we map free space we need to take into account the blocks
 * that are indexed by the AGFL. There aren't found by walking the
 * free space btrees, so we have to walk each AGFL to find them.
 */

> + */
> +static int
> +xfs_alloc_agfl_freespace_map(
> +     struct xfs_buf          **agbp, /* buffer for a.g. freelist header */
> +     struct xfs_btree_cur    *cur,
> +     struct fiemap_extent_info *fieinfo,
> +     xfs_agblock_t           sagbno,
> +     xfs_agblock_t           eagbno)

No need to pass the freespace btree cursor into this function,
nor the agf buffer. What is required is the struct xfs_mount,
the current agno and the agf structure. i.e.

xfs_alloc_agfl_freespace_map(
        struct xfs_mount        *mp,
        struct xfs_agf          *agf,
        struct fiemap_extent_info *fieinfo,
        xfs_agnumber_t          agno,
        xfs_agblock_t           sagbno,
        xfs_agblock_t           eagbno)

> +{
> +     xfs_agf_t       *agf;           /* a.g. freespace structure */
> +     xfs_buf_t       *agflbp;        /* buffer for a.g. freelist structure */
> +     __be32          *agfl_bno;      /* Reference to freelist block */
> +     int             j;
                ^
> +     int             index;
           ^
> +     int             error = 0;
           ^

Stray whitespace.

> +
> +     agf = XFS_BUF_TO_AGF(*agbp);
> +     error = xfs_alloc_read_agfl(cur->bc_mp, NULL, 
> be32_to_cpu(agf->agf_seqno),
> +                                 &agflbp);

        error = xfs_alloc_read_agfl(mp, NULL, agno, &agflbp);

> +     if (error)
> +             return error;
> +
> +     agfl_bno = XFS_BUF_TO_AGFL_BNO(cur->bc_mp, agflbp);
> +     
> +     index = be32_to_cpu(agf->agf_flfirst);
> +     
> +     for(j=1 ; j<=be32_to_cpu(agf->agf_flcount); j++)
> +        {

formatting, and no need for j and index parameters.

        for (i = be32_to_cpu(agf->agf_flfirst);
             i < be32_to_cpu(agf->agf_fllast);) {
        ....
                if (++i == XFS_AGFL_SIZE(mp))
                        i = 0;
        }

> +      xfs_agblock_t  fbno;
> +      xfs_extlen_t   flen;
> +      xfs_daddr_t    dbno;
> +      xfs_fileoff_t  dlen;   
> +      int            flags = 0;

all the indenting in the function needs fixing.

> +
> +     /* Relative AG block number */
> +      fbno = be32_to_cpu(agfl_bno[index]);
> +     /* Each entry in the AGFL is a single entry i.e length is 1 block */ 
> +      flen = 1;

These can all be folded into the code below - no need for
intermediate variables...

> +
> +     /*
> +      * AGFL is maintained as a circular list. Following is needed
> +      * to handle array wrapping correctly.
> +      */     
> +         if( index == ( (XFS_AGFL_SIZE(cur->bc_mp))-1 ) )
> +       index = 0;    /* First index of AGFL */
> +         else
> +          index++;   /* Next index in AGFL */

This is handled by the above loop construct so can be ignored.
> +        
> +     /*
> +      * Use daddr format for all range/len calculations as that is
> +      * the format the range/len variables are supplied in by
> +      * userspace.
> +      */
> +      dbno = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno, fbno);
> +      dlen = XFS_FSB_TO_BB(cur->bc_mp, flen);
> +      error = -fiemap_fill_next_extent(fieinfo, BBTOB(dbno),
> +                                       BBTOB(dbno), BBTOB(dlen), flags);

This takes bytes as it's values, and we have AG block numbers and
filesystem block lengths. so:


and the entire loop becomes:

        for (i = be32_to_cpu(agf->agf_flfirst);
             i < be32_to_cpu(agf->agf_fllast);) {
                /*
                 * Use daddr format for all range/len calculations as that is
                 * the format the range/len variables are supplied in by
                 * userspace.
                 */
                dbno = XFS_AGB_TO_DADDR(mp, agno, be32_to_cpu(agfl_bno[i]));
                error = fiemap_fill_next_extent(fieinfo,
                                                BBTOB(dbno), BBTOB(dbno),
                                                XFS_FSB_TO_B(mp, 1), flags);
                if (error)
                        break;
                if (++i == XFS_AGFL_SIZE(mp))
                        i = 0;
        }
        xfs_buf_relse(agflbp);
        return error;
}

Hmmm - I think something is still missing - what are the sagbno and
eagbno parameters supposed to do?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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