On Fri, Oct 13, 2017 at 04:22:20PM +1100, Dave Chinner wrote:
> On Thu, Oct 12, 2017 at 01:23:03PM -0700, Darrick J. Wong wrote:
> > On Wed, Oct 11, 2017 at 06:41:15PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > >
> > > Create some helper functions to check that inode pointers point to
> > > somewhere within the filesystem and not at the static AG metadata.
> > > Move xfs_internal_inum and create a directory inode check function.
> > > We will use these functions in scrub and elsewhere.
> ....
> > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > > index 988bb3f..da3652b 100644
> > > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > > @@ -2664,3 +2664,84 @@ xfs_ialloc_pagi_init(
> > > xfs_trans_brelse(tp, bp);
> > > return 0;
> > > }
> > > +
> > > +/* Calculate the first and last possible inode number in an AG. */
> > > +void
> > > +xfs_ialloc_aginode_range(
>
> agino_range?
>
> > > + struct xfs_mount *mp,
> > > + xfs_agnumber_t agno,
> > > + xfs_agino_t *first,
> > > + xfs_agino_t *last)
> > > +{
> > > + xfs_agblock_t eoag;
> > > +
> > > + eoag = xfs_ag_block_count(mp, agno);
> > > + *first = round_up(XFS_OFFBNO_TO_AGINO(mp, XFS_AGFL_BLOCK(mp) + 1, 0),
> > > + XFS_INODES_PER_CHUNK);
> > > + *last = round_down(XFS_OFFBNO_TO_AGINO(mp, eoag, 0),
> > > + XFS_INODES_PER_CHUNK) - 1;
> >
> > This is incorrect; we allocate inode chunks aligned to
> > xfs_ialloc_cluster_alignment blocks, which doesn't necessarily result in
> > ir_startino being aligned to XFS_INODES_PER_CHUNK.
>
> *nod*
>
> > I think the correct code is this:
> >
> > /* Calculate the first inode. */
> > bno = round_up(XFS_AGFL_BLOCK(mp) + 1,
> > xfs_ialloc_cluster_alignment(mp));
> > *first = XFS_OFFBNO_TO_AGINO(mp, bno, 0);
>
> *nod*
>
> > /* Calculate the last inode. */
> > bno = round_down(eoag, xfs_ialloc_cluster_alignment(mp));
> > *last = XFS_OFFBNO_TO_AGINO(mp, bno, 0) - 1;
>
> Bit tricky - I'm not sure that this will give the same inode number
> in all cases as rounding down to last valid chunk start offset and
> then adding (MAX(XFS_INODES_PER_CHUNK, inodes-per-block) - 1) to
> it....
>
> > ...which unfortunately I didn't realize until trying to play with
> > nondefault geometry options (1k blocks, no sparse inodes).
>
> Ok, that might explain a bunch of inode noise on my 1k block size
> test runs...
>
> > > +/*
> > > + * Verify that an AG inode number pointer neither points outside the AG
> > > + * nor points at static metadata.
> > > + */
> > > +bool
> > > +xfs_verify_agino_ptr(
>
> Again, I'd probably drop the _ptr suffix here.
>
> > > + struct xfs_mount *mp,
> > > + xfs_agnumber_t agno,
> > > + xfs_agino_t agino)
> > > +{
> > > + xfs_agino_t first;
> > > + xfs_agino_t last;
> > > + int ioff;
> > > +
> > > + ioff = XFS_AGINO_TO_OFFSET(mp, agino);
> > > + xfs_ialloc_aginode_range(mp, agno, &first, &last);
> > > + return agino >= first && agino <= last &&
> > > + ioff < (1 << mp->m_sb.sb_inopblog);
>
> This ioff check will always evaluate as true, yes?
>
> ioff = XFS_AGINO_TO_OFFSET(i)
> = ((i) & XFS_INO_MASK(XFS_INO_OFFSET_BITS(mp)))
> = (i & XFS_INO_MASK((mp)->m_sb.sb_inopblog))
> = (i & (uint32_t)((1ULL << (mp)->m_sb.sb_inopblog)) - 1)
>
> say sb_inopblog = 8:
>
> ioff = (i & 0xFF)
>
> And so:
>
> ioff < (1 << 8)
>
> will always be true.
>
> So I'm not sure this check is needed?
Indeed not.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
