diff mbox

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

Message ID 1472783257-15941-3-git-send-email-david@fromorbit.com
State New, archived
Headers show

Commit Message

Dave Chinner Sept. 2, 2016, 2:27 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 4 ++++
 fs/xfs/xfs_mount.h        | 2 ++
 2 files changed, 6 insertions(+)

Comments

Eric Sandeen Sept. 2, 2016, 1:25 p.m. UTC | #1
On 9/1/16 9:27 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>
> ---
>  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
Dave Chinner Sept. 2, 2016, 11:06 p.m. UTC | #2
On Fri, Sep 02, 2016 at 08:25:12AM -0500, Eric Sandeen wrote:
> On 9/1/16 9:27 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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 <dchinner@redhat.com>
> > ---
> >  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:

Gets cleaned up in a later patch.

> sync, should there be a wrapper to encapsulate it all?
> 
> like:
> 
> xfs_agf_{advance/drop/remove}_first(mp, agf, pagf)

Not really necessary for single use functions whose express purpose
is manipulating the agfl indexes

Cheers,

Dave.
diff mbox

Patch

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;
@@ -2363,6 +2364,7 @@  xfs_alloc_put_freelist(
 	be32_add_cpu(&agf->agf_flcount, 1);
 	xfs_trans_agflist_delta(tp, 1);
 	pag->pagf_flcount++;
+	pag->pagf_fllast = be32_to_cpu(agf->agf_fllast);
 
 	logflags = XFS_AGF_FLLAST | XFS_AGF_FLCOUNT;
 	if (btreeblk) {
@@ -2547,6 +2549,8 @@  xfs_alloc_read_agf(
 		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
 		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
 		pag->pagf_flcount = be32_to_cpu(agf->agf_flcount);
+		pag->pagf_flfirst = be32_to_cpu(agf->agf_flfirst);
+		pag->pagf_fllast = be32_to_cpu(agf->agf_fllast);
 		pag->pagf_longest = be32_to_cpu(agf->agf_longest);
 		pag->pagf_levels[XFS_BTNUM_BNOi] =
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNOi]);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index b36676c..3eb1b20 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -340,6 +340,8 @@  typedef struct xfs_perag {
 	__uint8_t	pagf_levels[XFS_BTNUM_AGF];
 					/* # of levels in bno & cnt btree */
 	__uint32_t	pagf_flcount;	/* count of blocks in freelist */
+	__uint32_t	pagf_flfirst;	/* first freelist block's index */
+	__uint32_t	pagf_fllast;	/* last freelist block's index */
 	xfs_extlen_t	pagf_freeblks;	/* total free blocks */
 	xfs_extlen_t	pagf_longest;	/* longest free space */
 	__uint32_t	pagf_btreeblks;	/* # of blocks held in AGF btrees */