On Fri, Dec 15, 2017 at 09:03:26AM +1100, Dave Chinner wrote:
> On Wed, Dec 13, 2017 at 03:58:37PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Refactor the callers of verifiers to print the instruction address of a
> > failing check.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Just a quick comment about formatting as I browsed the patch...
>
> > @@ -567,13 +568,14 @@ xfs_agfl_read_verify(
> > if (!xfs_sb_version_hascrc(&mp->m_sb))
> > return;
> >
> > - if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF))
> > + if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF)) {
> > + fa = __this_address;
> > xfs_buf_ioerror(bp, -EFSBADCRC);
> > - else if (xfs_agfl_verify(bp))
> > + } else if ((fa = xfs_agfl_verify(bp)))
> > xfs_buf_ioerror(bp, -EFSCORRUPTED);
> >
> > if (bp->b_error)
> > - xfs_verifier_error(bp);
> > + xfs_verifier_error(bp, fa);
>
> We are really trying to get rid of assignments in if() statements,
> so I'd prefer we don't add a bunch of new ones. While I understand
> there's a lot of mechanical change in this patch, I'd prefer to see
> these end up as something more like:
>
> > - if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF))
> > + if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF)) {
> > + fa = __this_address;
> > xfs_buf_ioerror(bp, -EFSBADCRC);
> > - else if (xfs_agfl_verify(bp))
> > + } else if ((fa = xfs_agfl_verify(bp)))
> > xfs_buf_ioerror(bp, -EFSCORRUPTED);
>
> if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF)) {
> fa = __this_address;
> error = -EFSBADCRC;
> } else {
> fa = xfs_agfl_verify(bp);
> if (fa)
> error = -EFSCORRUPTED;
> }
>
> if (error) {
> xfs_buf_ioerror(bp, error);
> xfs_verifier_error(bp, fa);
> }
>
> .....
>
> > @@ -2459,16 +2462,18 @@ xfs_agf_read_verify(
> > struct xfs_buf *bp)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > + xfs_failaddr_t fa;
> >
> > if (xfs_sb_version_hascrc(&mp->m_sb) &&
> > - !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
> > + !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF)) {
> > + fa = __this_address;
> > xfs_buf_ioerror(bp, -EFSBADCRC);
> > - else if (XFS_TEST_ERROR(xfs_agf_verify(mp, bp), mp,
> > - XFS_ERRTAG_ALLOC_READ_AGF))
> > + } else if (XFS_TEST_ERROR((fa = xfs_agf_verify(mp, bp)), mp,
> > + XFS_ERRTAG_ALLOC_READ_AGF))
> > xfs_buf_ioerror(bp, -EFSCORRUPTED);
> >
> > if (bp->b_error)
> > - xfs_verifier_error(bp);
> > + xfs_verifier_error(bp, fa);
> > }
>
> Because this sort of thing is now getting towards being unreadable.
> With the way we keep adding to verifier checks, it's only going to
> get worse if we don't take steps to clean it up...
Ok.
> > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > index 4c9f35d..0bbbf0b 100644
> > --- a/fs/xfs/xfs_error.c
> > +++ b/fs/xfs/xfs_error.c
> > @@ -347,13 +347,15 @@ xfs_corruption_error(
> > */
> > void
> > xfs_verifier_error(
> > - struct xfs_buf *bp)
> > + struct xfs_buf *bp,
> > + xfs_failaddr_t fa)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> >
> > xfs_alert(mp, "Metadata %s detected at %pS, %s block 0x%llx",
> > bp->b_error == -EFSBADCRC ? "CRC error" : "corruption",
> > - __return_address, bp->b_ops->name, bp->b_bn);
> > + fa ? fa : __return_address, bp->b_ops->name,
> > + bp->b_bn);
> >
> > xfs_alert(mp, "Unmount and run xfs_repair");
>
> I'm also wondering if we should move the xfs_buf_ioerror() call
> inside this function, too, rather than coding multiple calls in the
> verifiers to set bp->b_error in each branch of the verifier that
> has an error...
What, something like:
if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF)) {
fa = __this_address;
error = -EFSBADCRC;
} else {
fa = xfs_agfl_verify(bp);
if (fa)
error = -EFSCORRUPTED;
}
if (error)
xfs_verifier_error(bp, fa, error);
???
--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
