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...
> 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...
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
