xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks
From: ADRS PICT <adrscube@xxxxxxxxx>
Date: Wed, 28 Jan 2015 18:35:22 +0530
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=tHYHCj3cA8WjsDY15xxr81hr16G7rV7a6Wk+HcLUL3A=; b=wMsKUFi+QS1F/D9KsPycl0Erk535ylr2MBzOLXaxxKQGDDO89xM0pY99009w5kbt9A pmcuQDK/VBaR8ZUSTC9xumewwI6XBZ9ChIS2pfz+P5nu3tdJbV4DVabPd9STN+gIAOpR 4Z1KREDFz88CIdbb87kmnkTBWs4QpZk8tGel6KgwsKCnv4hyiBTcNj1UHngldI85rDTK 50meMcpEFt7TNhqtbGunJZy87pQi5/3Ln2P8CPFYOWuHnTmI9CKDSPoZbarcOzTJiFMw lYQVJ5qDcEBoc0iqoRDpXBaMOMuJIJukYXaZR10xa01c8P6Uv8O4Xt/X7OYcAJYkMMZw mFWg==
In-reply-to: <20150128015757.GT16552@dastard>
References: <54c1c12e.230a460a.4729.11fc@xxxxxxxxxxxxx> <20150128015757.GT16552@dastard>


On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> 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 ;)
>

We will keep that in mind, while sending the updated patches. :)

> 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....
>

We will make a not of this too.

>> +
>> +/*
>> + * 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.

Our bad, we will ensure that the comment, explains why the function is needed.

>
> /*
> Â* 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;
> }

We will incorporate the above mentioned changes, and send the updated
patch at the earliest.

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

Actually the parameters sagbno and eagbno are not needed in thisÂ
function and can be excluded.

> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx

Thank you for the detailed feedback. We will be sending the updated
patches at the earliest.

Regards,
A-DRS

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