On Thu, Jan 29, 2015 at 06:53:01PM +0530, Dhruvesh Rathore wrote: > > The 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 > Good description, but: > ------------------------------------------------------------------------------------------- Tmhese lines confuse any utility that attempts to parse the patch. Lines starting with "---" have have special meaning to patch. They haven't come from diff or git, so you must have added them manually. That makes me think you composed this email manually from a variety of different sources. Please, learn to use git to make your changes and produce diffs that you send. git send-email is your friend - learn to use it. There's a basic howto on the xfs web site: http://xfs.org/index.php/XFS_git_howto As such, I've hacked the patch into a format I can apply - I've attached the patch generated from my git tree so you see how different it looks. > Signed-off-by: Dhruvesh Rathore <dhruvesh_r@xxxxxxxxxxx> > Signed-off-by: Amey Ruikar <ameyruikar@xxxxxxxxx> > Signed-off-by: Somdeep Dey <somdeepdey10@xxxxxxxxx> > --- > fs/xfs/libxfs/xfs_alloc.c | 69 ++++++++++++++++ > 1 file changed, 69 insertions(+) > > --- a/fs/xfs/libxfs/xfs_alloc.c 2015-01-29 09:07:07.116439198 +0530 > +++ b/fs/xfs/libxfs/xfs_alloc.c 2015-01-29 09:06:39.664440229 +0530 > > @@ -2698,6 +2698,71 @@ > } > > > +/* > + * When we map free space we need to take into account the blocks > + * that are indexed by the AGFL. They 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_mount *mp, > + struct xfs_agf *agf, > + struct fiemap_extent_info *fieinfo, > + xfs_agnumber_t agno, > + xfs_agblock_t sagbno, > + xfs_agblock_t eagbno) > +{ > + xfs_buf_t *agflbp; > + __be32 *agfl_bno; > + int i; > + int error = 0; > + > + error = xfs_alloc_read_agfl(mp, NULL, be32_to_cpu(agf->agf_seqno), &agflbp); Line longer than 80 columns, and the AG number we are working on is passed as a parameter to the function so there is no need to read it from the AGF. > + stray extra line. > + if (error) > + return error; > + > + agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp); > + > + for(i = be32_to_cpu(agf->agf_flfirst); > + i <= be32_to_cpu(agf->agf_fllast);) { Whitespace after the "for" statement, and indentation of logic inside for/if/while/etc needs to be aligned to the same column. As it is, this code doesn't work. Hint: what happens when last wraps around back to the start ofthe freelist array? last < first, and hence i > last.... Yes, I know it the code is similar to what I suggested last time I pointed out the iteration is wrong, but I'm often wrong, especially about intricate details that I've just quickly pulled a solution for off the top of my head. IOWs, don't assume what I say is correct - verifiy and test it, and then tell me I'm wrong. So, perhaps a different loop structure is appropriate? > + > + xfs_agblock_t fbno; > + xfs_extlen_t flen; > + xfs_daddr_t dbno; > + xfs_fileoff_t dlen; Trailing whitespace. > + int flags = 0; > + > + fbno = be32_to_cpu(agfl_bno[i]); > + flen = 1; > + > + /* range check - must be wholly withing requested range */ > + if (fbno < sagbno || > + (eagbno != NULLAGBLOCK && fbno + flen > eagbno)) { > + xfs_warn(mp, "10: %d/%d, %d/%d", sagbno, eagbno, fbno, flen); > + continue; Indentation: if (fbno < sagbno || (eagbno != NULLAGBLOCK && fbno + flen > eagbno)) { xfs_warn(mp, "10: %d/%d, %d/%d", sagbno, eagbno, fbno, flen); > + } > + > + /* > + * 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, fbno); > + dlen = XFS_FSB_TO_BB(mp, flen); > + > + error = -fiemap_fill_next_extent(fieinfo,BBTOB(dbno), > + BBTOB(dbno), BBTOB(dlen), flags); We don't need to negate the error any more, also whitespace after fieinfo. > + Stray whitespace in a stray line.. > + if (error) > + break; > + if (++i == XFS_AGFL_SIZE(mp)) > + i = 0; > + } > + xfs_buf_relse(agflbp); > + return error; > +} > > /* > * Map the freespace from the requested range in the requested order. > @@ -2804,6 +2869,10 @@ > } > XFS_WANT_CORRUPTED_GOTO(i == 1, del_cursor); > > + /* Account for the free blocks in AGFL */ > + error = xfs_alloc_agfl_freespace_map(mp, XFS_BUF_TO_AGF(agbp), fieinfo, agno, sagbno, > + agno == eagno ? eagbno : NULLAGBLOCK); > + Line is much longer than 80 columns. checkpatch will catch trivial things like whitespace and line length.... Also, if there is an error, we need to handle it appropriately. Patch as applied to my tree is below. Please apply it to your tree and test it. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx xfs: Accounting for AGFL blocks in xfs_spaceman From: Dhruvesh Rathore <adrscube@xxxxxxxxx> The xfs_spaceman utility previously failed to account for AGFL blocks. Old output: $ 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 adds a new function which accounts for the the free extents tracked by the AGFL. New output: $ 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 [dchinner: fixed whitespace issues, agfl iteration logic, error handling] Signed-off-by: Dhruvesh Rathore <dhruvesh_r@xxxxxxxxxxx> Signed-off-by: Amey Ruikar <ameyruikar@xxxxxxxxx> Signed-off-by: Somdeep Dey <somdeepdey10@xxxxxxxxx> --- fs/xfs/libxfs/xfs_alloc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 1ae53ad..544c945 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2633,6 +2633,68 @@ error0: } /* + * When we map free space we need to take into account the blocks + * that are indexed by the AGFL. They 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_mount *mp, + struct xfs_agf *agf, + struct fiemap_extent_info *fieinfo, + xfs_agnumber_t agno, + xfs_agblock_t sagbno, + xfs_agblock_t eagbno) +{ + xfs_buf_t *agflbp; + __be32 *agfl_bno; + int i; + int error = 0; + + error = xfs_alloc_read_agfl(mp, NULL, agno, &agflbp); + if (error) + return error; + + agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp); + for (i = be32_to_cpu(agf->agf_flfirst);;) { + xfs_agblock_t fbno; + xfs_extlen_t flen; + xfs_daddr_t dbno; + xfs_fileoff_t dlen; + int flags = 0; + + fbno = be32_to_cpu(agfl_bno[i]); + flen = 1; + + /* range check - must be wholly withing requested range */ + if (fbno < sagbno || + (eagbno != NULLAGBLOCK && fbno + flen > eagbno)) { + xfs_warn(mp, "10: %d/%d, %d/%d", + sagbno, eagbno, fbno, flen); + continue; + } + + /* + * 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, fbno); + dlen = XFS_FSB_TO_BB(mp, flen); + error = fiemap_fill_next_extent(fieinfo, BBTOB(dbno), + BBTOB(dbno), BBTOB(dlen), flags); + if (error) + break; + if (i == be32_to_cpu(agf->agf_fllast)) + break; + if (++i == XFS_AGFL_SIZE(mp)) + i = 0; + } + xfs_buf_relse(agflbp); + return error; +} + +/* * Walk the extents in the tree given by the cursor, and dump them all into the * fieinfo. At the last extent in the tree, set the FIEMAP_EXTENT_LAST flag so * that we return only free space from this tree in a given request. @@ -2793,6 +2855,13 @@ xfs_alloc_freespace_map( goto put_agbp; } + /* Account for the free blocks in AGFL */ + error = xfs_alloc_agfl_freespace_map(mp, XFS_BUF_TO_AGF(agbp), + fieinfo, agno, sagbno, + agno == eagno ? eagbno : NULLAGBLOCK); + if (error) + goto put_agbp; + cur = xfs_allocbt_init_cursor(mp, NULL, agbp, agno, XFS_BTNUM_BNO); error = xfs_alloc_lookup_ge(cur, sagbno, 1, &i); _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
