On Tue, Dec 19, 2017 at 03:12:38PM +1100, Dave Chinner wrote:
> On Wed, Dec 13, 2017 at 03:58:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Modify each function that checks the contents of a metadata buffer to
> > return the instruction address of the failing test so that we can report
> > more precise failure errors to the log.
>
>
> > @@ -331,29 +332,29 @@ xfs_allocbt_verify(
> > level = be16_to_cpu(block->bb_level);
> > switch (block->bb_magic) {
> > case cpu_to_be32(XFS_ABTB_CRC_MAGIC):
> > - if (!xfs_btree_sblock_v5hdr_verify(bp))
> > - return false;
> > + if ((fa = xfs_btree_sblock_v5hdr_verify(bp)))
> > + return fa;
>
> I see a few of these.
>
> fa = xfs_btree_sblock_v5hdr_verify(bp);
> if (fa)
> return fa;
Fixed.
> > @@ -4609,7 +4609,7 @@ xfs_btree_sblock_v5hdr_verify(
> > * @bp: buffer containing the btree block
> > * @max_recs: maximum records allowed in this btree node
> > */
> > -bool
> > +xfs_failaddr_t
> > xfs_btree_sblock_verify(
> > struct xfs_buf *bp,
> > unsigned int max_recs)
> > @@ -4619,19 +4619,19 @@ xfs_btree_sblock_verify(
> >
> > /* numrecs verification */
> > if (be16_to_cpu(block->bb_numrecs) > max_recs)
> > - return false;
> > + return __this_address;
> >
> > /* sibling pointer verification */
> > if (!block->bb_u.s.bb_leftsib ||
> > (be32_to_cpu(block->bb_u.s.bb_leftsib) >= mp->m_sb.sb_agblocks &&
> > block->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK)))
> > - return false;
> > + return __this_address;
> > if (!block->bb_u.s.bb_rightsib ||
> > (be32_to_cpu(block->bb_u.s.bb_rightsib) >= mp->m_sb.sb_agblocks &&
> > block->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)))
> > - return false;
> > + return __this_address;
>
> Out if scope for this patch, but like th e lblock verifying, I think
> these are candidates for xfs_verify_agbno()?
Yep. Added another patch to fix this.
> > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index 27297a6..87565b6 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -73,13 +73,13 @@ xfs_dir3_leaf1_check(
> > } else if (leafhdr.magic != XFS_DIR2_LEAF1_MAGIC)
> > return false;
> >
> > - return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf);
> > + return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf) == NULL;
> > }
>
> Hmmmm - shouldn't we make xfs_dir3_leaf_check() dump the failaddr
> so we know what debug check failed? This would improve the debug
> failure reporting in this code, and seeing as we now have the info it
> doesn't really make sense to throw it away without reporting it...
Fixed.
> > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > index 682e2bf..00ded0a 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -76,13 +76,13 @@ xfs_dir3_leafn_check(
> > } else if (leafhdr.magic != XFS_DIR2_LEAFN_MAGIC)
> > return false;
> >
> > - return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf);
> > + return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf) == NULL;
> > }
> > #else
> > #define xfs_dir3_leaf_check(dp, bp)
> > #endif
>
> Same here.
Fixed.
> > @@ -93,21 +93,21 @@ xfs_dir3_free_verify(
> > struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
> >
> > if (hdr3->magic != cpu_to_be32(XFS_DIR3_FREE_MAGIC))
> > - return false;
> > + return __this_address;
> > if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> > - return false;
> > + return __this_address;
> > if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> > - return false;
> > + return __this_address;
> > if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> > - return false;
> > + return __this_address;
> > } else {
> > if (hdr->magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC))
> > - return false;
> > + return __this_address;
> > }
>
> /me wonders if there is opportunity for factoring this header check
> seeing as it's repeated a few times, just with different magic
> numbers...
>
> > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > index 45c68d0..c441efb 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > @@ -41,7 +41,7 @@ extern int xfs_dir2_leaf_to_block(struct xfs_da_args *args,
> > #ifdef DEBUG
> > #define xfs_dir3_data_check(dp, bp) \
> > do { \
> > - if (!__xfs_dir3_data_check((dp), (bp))) { \
> > + if (__xfs_dir3_data_check((dp), (bp))) { \
> > XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, \
> > (bp)->b_target->bt_mount, (bp)->b_addr); \
> > } \
>
> This should capture the failaddr and dump it as part of the
> corruption error, I think. Otherwise we lose what part of the
> structure was considered corrupt from the error report.
Ok.
> > @@ -507,7 +507,7 @@ xfs_iread(
> > return error;
> >
> > /* even unallocated inodes are verified */
> > - if (!xfs_dinode_verify(mp, ip->i_ino, dip)) {
> > + if (xfs_dinode_verify(mp, ip->i_ino, dip)) {
> > xfs_alert(mp, "%s: validation failed for inode %lld",
> > __func__, ip->i_ino);
>
> Capture the failaddr in the error message?
Ok.
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> > index a9c97a3..2317511 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.h
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> > @@ -82,7 +82,7 @@ void xfs_inobp_check(struct xfs_mount *, struct xfs_buf *);
> > #define xfs_inobp_check(mp, bp)
> > #endif /* DEBUG */
> >
> > -bool xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> > - struct xfs_dinode *dip);
> > +void *xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> > + struct xfs_dinode *dip);
>
> xfs_failaddr_t?
Fixed.
--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
