<div dir="ltr"><div class="gmail_extra"><br><br>On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <<a href="mailto:david@fromorbit.com">david@fromorbit.com</a>> wrote:<br>> On Fri, Jan 23, 2015 at 09:04:00AM +0530, Dhruvesh Rathore wrote:<br>>> Here is the original discussion that Dave wrote for xfs_spaceman:<br>>><br>>> <a href="http://oss.sgi.com/archives/xfs/2012-10/msg00363.html">http://oss.sgi.com/archives/xfs/2012-10/msg00363.html</a><br>>><br>>> These patches were not posted and were just forward ported to a current<br>>> 3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in Dave's<br>>> kernel tree at:<br>>><br>>> git://<a href="http://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git">git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git</a><br>>><br>>> The original xfs_spaceman tool that Dave wrote is here:<br>>><br>>> <a href="http://oss.sgi.com/archives/xfs/2012-10/msg00366.html">http://oss.sgi.com/archives/xfs/2012-10/msg00366.html</a><br>>><br>>> Dave just updated it to the 3.2.2 code base and pushed it to the<br>>> spaceman branch in this tree:<br>>><br>>> git://<a href="http://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git">git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git</a><br>><br>> As per last patch, this is series description so it doesn't need to<br>> be repeated in every patch ;)<br>><br><br>We will keep that in mind, while sending the updated patches. :)<br><div><br></div><div>> From here:<br>><br>>> This xfs_spaceman utility previously failed to account for AGFL blocks.<br>>> Old output (Before changes).<br>>><br>>> $ sudo xfs_spaceman -c "freesp" /media/xfs<br>>>    from      to extents  blocks    pct<br>>>    1024    2047       1    1262   0.04<br>>>    4096    8191       1    5126   0.15<br>>>    8192   16383       3   35344   1.05<br>>>   32768   65535       1   43989   1.31<br>>>  262144  524287       3 1334894  39.78<br>>>  524288  967428       2 1934840  57.66<br>>><br>>> As you can see the AGFL blocks were unaccounted (4 per AG, and there<br>>> were 4 AGs in this filesystem).<br>>><br>>> This patch is concerned with adding a new function which will walk the free<br>>> extents in AGFL and account for these AGFL blocks, while file system scanning.<br>>><br>>> New output (After implementing this patch).<br>>><br>>> $ uname -a<br>>> 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<br>>> $ sudo xfs_spaceman -V<br>>> xfs_spaceman version 3.2.2<br>>> $ sudo xfs_spaceman -c "freesp" /media/xfs<br>>>    from      to extents  blocks    pct<br>>>       1       1      16      16   0.00<br>>>    1024    2047       1    1262   0.04<br>>>    4096    8191       1    5126   0.15<br>>>    8192   16383       3   35344   1.05<br>>>   32768   65535       1   43989   1.31<br>>>  262144  524287       3 1334894  39.78<br>>>  524288  967428       2 1934840  57.66<br>><br>> to here, this is a good patch description. :)<br>><br>>> Please do comment.<br>><br>> No need to ask in each patch description - the series description if<br>> the place for that....<br>></div><div><br></div><div>We will make a not of this too.</div><div><br>>> +<br>>> +/*<br>>> + * Walk the free extents in the AGFL (AG Free List) of each AG, and dump<br>><br>> No need to explain what AGFL means - anyone reading the code already<br>> knows....<br>><br>>> + * them all into the fieinfo.<br>>> + *<br>>> + * With a freshly made filesystem, 4 blocks are reserved immediately after<br>>> + * the free space B+tree root blocks (blocks 4 to 7). As they are used up<br>><br>> The number of blocks is actually dependent on filesystem geometry,<br>> but again there isn't a need to describe the exact workings of the<br>> AGFL here.<br>><br>>> + * additional blocks are reserved from the AG and added to the free list<br>>> + * array. A typical device will have space for 128 elements in the array.<br>>> + * The actual size can be determined using XFS_AGFL_SIZE macro. The array<br>>> + * is maintained as a circular list and active elements are pointed by<br>>> + * AGF's agf_flfirst, agf_fllast and agf_flcount values.<br>><br>> Yup, you're describing the code, not /why/ the function exists.</div><div><br></div><div>Our bad, we will ensure that the comment, explains why the function is needed.</div><div><br>><br>> /*<br>>  * When we map free space we need to take into account the blocks<br>>  * that are indexed by the AGFL. There aren't found by walking the<br>>  * free space btrees, so we have to walk each AGFL to find them.<br>>  */<br>><br>>> + */<br>>> +static int<br>>> +xfs_alloc_agfl_freespace_map(<br>>> +     struct xfs_buf          **agbp, /* buffer for a.g. freelist header */<br>>> +     struct xfs_btree_cur    *cur,<br>>> +     struct fiemap_extent_info *fieinfo,<br>>> +     xfs_agblock_t           sagbno,<br>>> +     xfs_agblock_t           eagbno)<br>><br>> No need to pass the freespace btree cursor into this function,<br>> nor the agf buffer. What is required is the struct xfs_mount,<br>> the current agno and the agf structure. i.e.<br>> xfs_alloc_agfl_freespace_map(<br>>         struct xfs_mount        *mp,<br>>         struct xfs_agf          *agf,<br>>         struct fiemap_extent_info *fieinfo,<br>>         xfs_agnumber_t          agno,<br>>         xfs_agblock_t           sagbno,<br>>         xfs_agblock_t           eagbno)<br>><br>>> +{<br>>> +     xfs_agf_t       *agf;           /* a.g. freespace structure */<br>>> +     xfs_buf_t       *agflbp;        /* buffer for a.g. freelist structure */<br>>> +     __be32          *agfl_bno;      /* Reference to freelist block */<br>>> +     int             j;<br>>                 ^<br>>> +     int             index;<br>>            ^<br>>> +     int             error = 0;<br>>            ^<br>><br>> Stray whitespace.<br>><br>>> +<br>>> +     agf = XFS_BUF_TO_AGF(*agbp);<br>>> +     error = xfs_alloc_read_agfl(cur->bc_mp, NULL, be32_to_cpu(agf->agf_seqno),<br>>> +                                 &agflbp);<br>><br>>         error = xfs_alloc_read_agfl(mp, NULL, agno, &agflbp);<br>><br>>> +     if (error)<br>>> +             return error;<br>>> +<br>>> +     agfl_bno = XFS_BUF_TO_AGFL_BNO(cur->bc_mp, agflbp);<br>>> +<br>>> +     index = be32_to_cpu(agf->agf_flfirst);<br>>> +<br>>> +     for(j=1 ; j<=be32_to_cpu(agf->agf_flcount); j++)<br>>> +        {<br>><br>> formatting, and no need for j and index parameters.<br>><br>>         for (i = be32_to_cpu(agf->agf_flfirst);<br>>              i < be32_to_cpu(agf->agf_fllast);) {<br>>         ....<br>>                 if (++i == XFS_AGFL_SIZE(mp))<br>>                         i = 0;<br>>         }<br>><br>>> +      xfs_agblock_t  fbno;<br>>> +      xfs_extlen_t   flen;<br>>> +      xfs_daddr_t    dbno;<br>>> +      xfs_fileoff_t  dlen;<br>>> +      int            flags = 0;<br>><br>> all the indenting in the function needs fixing.<br>><br>>> +<br>>> +     /* Relative AG block number */<br>>> +      fbno = be32_to_cpu(agfl_bno[index]);<br>>> +     /* Each entry in the AGFL is a single entry i.e length is 1 block */<br>>> +      flen = 1;<br>><br>> These can all be folded into the code below - no need for<br>> intermediate variables...<br>><br>>> +<br>>> +     /*<br>>> +      * AGFL is maintained as a circular list. Following is needed<br>>> +      * to handle array wrapping correctly.<br>>> +      */<br>>> +         if( index == ( (XFS_AGFL_SIZE(cur->bc_mp))-1 ) )<br>>> +       index = 0;    /* First index of AGFL */<br>>> +         else<br>>> +          index++;   /* Next index in AGFL */<br>><br>> This is handled by the above loop construct so can be ignored.<br>>> +<br>>> +     /*<br>>> +      * Use daddr format for all range/len calculations as that is<br>>> +      * the format the range/len variables are supplied in by<br>>> +      * userspace.<br>>> +      */<br>>> +      dbno = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno, fbno);<br>>> +      dlen = XFS_FSB_TO_BB(cur->bc_mp, flen);<br>>> +      error = -fiemap_fill_next_extent(fieinfo, BBTOB(dbno),<br>>> +                                       BBTOB(dbno), BBTOB(dlen), flags);<br>><br>> This takes bytes as it's values, and we have AG block numbers and<br>> filesystem block lengths. so:<br>><br>><br>> and the entire loop becomes:<br>><br>>         for (i = be32_to_cpu(agf->agf_flfirst);<br>>              i < be32_to_cpu(agf->agf_fllast);) {<br>>                 /*<br>>                  * Use daddr format for all range/len calculations as that is<br>>                  * the format the range/len variables are supplied in by<br>>                  * userspace.<br>>                  */<br>>                 dbno = XFS_AGB_TO_DADDR(mp, agno, be32_to_cpu(agfl_bno[i]));<br>>                 error = fiemap_fill_next_extent(fieinfo,<br>>                                                 BBTOB(dbno), BBTOB(dbno),<br>>                                                 XFS_FSB_TO_B(mp, 1), flags);<br>>                 if (error)<br>>                         break;<br>>                 if (++i == XFS_AGFL_SIZE(mp))<br>>                         i = 0;<br>>         }<br>>         xfs_buf_relse(agflbp);<br>>         return error;<br>> }<br><br>We will incorporate the above mentioned changes, and send the updated</div><div>patch at the earliest.<br><br>> Hmmm - I think something is still missing - what are the sagbno and<br>> eagbno parameters supposed to do?<br><br>Actually the parameters sagbno and eagbno are not needed in this </div><div>function and can be excluded.<br><br>> Cheers,<br>><br>> Dave.<br>> --<br>> Dave Chinner<br>> <a href="mailto:david@fromorbit.com">david@fromorbit.com</a><br><br></div><div>Thank you for the detailed feedback. We will be sending the updated</div><div>patches at the earliest.</div><div><br></div><div>Regards,</div><div>A-DRS</div><div><br></div></div></div>