On Thu, Oct 05, 2017 at 01:56:20PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 01:42:17PM -0700, Darrick J. Wong wrote:
> > +/* Scrub an rmapbt record. */
> > +STATIC int
> > +xfs_scrub_rmapbt_helper(
> > + struct xfs_scrub_btree *bs,
> > + union xfs_btree_rec *rec)
> > +{
> > + struct xfs_mount *mp = bs->cur->bc_mp;
> > + struct xfs_agf *agf;
> > + struct xfs_rmap_irec irec;
> > + unsigned long long rec_end;
> > + xfs_agblock_t eoag;
> > + bool non_inode;
> > + bool is_unwritten;
> > + bool is_bmbt;
> > + bool is_attr;
> > + int error;
> > +
> > + error = xfs_rmap_btrec_to_irec(rec, &irec);
> > + if (!xfs_scrub_btree_op_ok(bs->sc, bs->cur, 0, &error))
> > + goto out;
>
> This got me again. Again I was thinking that this code threw
> away the error from xfs_rmap_btrec_to_irec(). Could we consider
> renaming "op_ok" to "process_error" or something like that so
> it's clearer that it's doing some kind of checking on the error
> we just got back?
Ok. That /is/ a better suggestion.
> > +
> > + /* Check extent. */
> > + agf = XFS_BUF_TO_AGF(bs->sc->sa.agf_bp);
> > + eoag = be32_to_cpu(agf->agf_length);
> > + rec_end = (unsigned long long)irec.rm_startblock + irec.rm_blockcount;
> > +
> > + if (irec.rm_startblock >= mp->m_sb.sb_agblocks ||
> > + irec.rm_startblock >= eoag ||
> > + irec.rm_blockcount == 0 ||
> > + rec_end > mp->m_sb.sb_agblocks ||
> > + rec_end > eoag)
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
>
> Broken Record (tm).
Woo.
> > + /* Check flags. */
> > + non_inode = XFS_RMAP_NON_INODE_OWNER(irec.rm_owner);
> > + is_bmbt = irec.rm_flags & XFS_RMAP_BMBT_BLOCK;
> > + is_attr = irec.rm_flags & XFS_RMAP_ATTR_FORK;
> > + is_unwritten = irec.rm_flags & XFS_RMAP_UNWRITTEN;
> > +
> > + if (is_bmbt && irec.rm_offset != 0)
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +
> > + if (non_inode && irec.rm_offset != 0)
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +
> > + if (is_unwritten && (is_bmbt || non_inode || is_attr))
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +
> > + if (non_inode && (is_bmbt || is_unwritten || is_attr))
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +
> > + if (!non_inode) {
> > + xfs_agnumber_t agno = XFS_INO_TO_AGNO(mp, irec.rm_owner);
> > + xfs_agino_t agino = XFS_INO_TO_AGINO(mp, irec.rm_owner);
> > + xfs_agblock_t agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > +
> > + /* Owner inode within an AG? */
> > + if (agno >= mp->m_sb.sb_agcount ||
> > + agbno >= mp->m_sb.sb_agblocks)
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +
> > + /* Owner inode within the FS? */
> > + if (XFS_AGB_TO_DADDR(mp, agno, agbno) >=
> > + XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks))
> > + xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
>
> These two checks probably also should be libxfs functionality.
> We've got similar checks strewn all over the place (e.g. valid_bno()
> in xfs_db, verify_aginum/verify_inum/verify_agbno/verify_dfsbno
> in repair, etc.
>
> It would be good to get all these sorts of basic type checks
> centralised and used consistenly by all the code....
I just added that (currently code-factoring my way through the agi
scrubbers) so yes this should be easy to do for v12.
--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
