xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: Accounting for AGFL blocks in xfs_spaceman

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH 1/4] xfs: Accounting for AGFL blocks in xfs_spaceman
From: Dhruvesh Rathore <adrscube@xxxxxxxxx>
Date: Wed, 11 Feb 2015 21:37:47 +0530
Cc: Dave Chinner <david@xxxxxxxxxxxxx>
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 :cc:content-type; bh=qVMEjmuox3UBh+vYfjdIVJY07ibJf9gtB4S7cDuuClM=; b=d107TN+fQG3r2aef1J+lAMNryFhgMpD8XXkHmcfD9RpF+rn5m5daKvOkoOTQiowBWo KkfJqUBf1ia2sWVrUMX4KvRR9EAo8Ro7a6LXjV76uhSyarutRfuG9lMvdYgJvWXJcSYP E6zvhZEWMrg+vCYtKpAEaY/8Q5JHt7+fHQ+0yJH6mAjnMj3ifIcFS0IRRDJyZpuFAqE6 pDZA14cGyoZxqIUkHivMiZfEj9dPCe2znkGDZIz38kReqUryd9UYwGSXAod69dka0j70 rxGXIffHDDwR5gD00imeMwYeiHDtu0MO7q5BIK6Z6VTYk/U78I0r/NkedWuzT7wfGDJ0 Xjvg==
In-reply-to: <20150210072559.GA12722@dastard>
References: <54ca343a.43df440a.3c11.4474@xxxxxxxxxxxxx> <20150210072559.GA12722@dastard>
>> +     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?

We had addressed this issue in the previous code version, but
changed it when a doubt was raised in the previous patch comments.
We will ensure that we verify and test the feedback we receive from
now on. :)

> 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

We thank you for taking the time out and working on the patches. We will
apply the above reworked patch and test it, and get back to you.

Regards,
A-DRS

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