| Message ID | 1472783257-15941-3-git-send-email-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 --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 */