Re: [PATCH 2/6] xfs: shadow agfl indexes in the per-ag structures

On 9/1/16 9:27 PM, Dave Chinner wrote:
> From: Dave Chinner <[email protected]>
> 
> To verify that the AGFL contents is sane, we need to have access to
> the indexes that tell us what part of the AGFL is active. We cannot
> access the AGF buffer from the AGFL verifier, so we need to shadow
> these values in the struct xfs_perag so we check them when required.
> 
> Signed-off-by: Dave Chinner <[email protected]>
> ---
>  fs/xfs/libxfs/xfs_alloc.c | 4 ++++
>  fs/xfs/xfs_mount.h        | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 23559b9..1aef556 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2255,6 +2255,7 @@ xfs_alloc_get_freelist(
>  	be32_add_cpu(&agf->agf_flcount, -1);
>  	xfs_trans_agflist_delta(tp, -1);
>  	pag->pagf_flcount--;
> +	pag->pagf_flfirst = be32_to_cpu(agf->agf_flfirst);
>  	xfs_perag_put(pag);
>  
>  	logflags = XFS_AGF_FLFIRST | XFS_AGF_FLCOUNT;

Still reviewing, but this kind of jumped out at me, seems like the get/put
functions are a bit jumbled up:

        /*
         * Get the block number and update the data structures.
         */
        agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
        bno = be32_to_cpu(agfl_bno[be32_to_cpu(agf->agf_flfirst)]);
        be32_add_cpu(&agf->agf_flfirst, 1);
        xfs_trans_brelse(tp, agflbp);
        if (be32_to_cpu(agf->agf_flfirst) == XFS_AGFL_SIZE(mp))
                agf->agf_flfirst = 0;

        pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
        be32_add_cpu(&agf->agf_flcount, -1);
        xfs_trans_agflist_delta(tp, -1);
        pag->pagf_flcount--;
        xfs_perag_put(pag);


why is there a trans_brelse in between two lines which handle proper setting
of flfirst?  I was looking at this thinking about where the pag structures
get updated, then saw the kind of swiss-cheese placement of the first/count
manipulation...

If pagf_flcount/agf_flcount, pagf_flfirst/agf_flfirst etc need to stay in
sync, should there be a wrapper to encapsulate it all?

like:

xfs_agf_{advance/drop/remove}_first(mp, agf, pagf)
{
        be32_add_cpu(&agf->agf_flfirst, 1);
        if (be32_to_cpu(agf->agf_flfirst) == XFS_AGFL_SIZE(mp))
                agf->agf_flfirst = 0;
	pag->pagf_flfirst = be32_to_cpu(agf->agf_flfirst);
        be32_add_cpu(&agf->agf_flcount, -1);
        pag->pagf_flcount--;
}

or something similar?

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs