From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:49612 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072AbdLSU0U (ORCPT ); Tue, 19 Dec 2017 15:26:20 -0500 Date: Tue, 19 Dec 2017 12:26:15 -0800 From: "Darrick J. Wong" Subject: Re: [PATCH 03/13] xfs: have buffer verifier functions report failing address Message-ID: <20171219202615.GD11969@magnolia> References: <151320949282.30654.14805160700975182459.stgit@magnolia> <151320951104.30654.17605262300441313090.stgit@magnolia> <20171219041238.GM4094@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171219041238.GM4094@dastard> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Dave Chinner Cc: linux-xfs@vger.kernel.org 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 > > > > 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@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html