From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/13] xfs: refactor verifier callers to print address of failing check
Date: Thu, 14 Dec 2017 16:04:09 -0800 [thread overview]
Message-ID: <20171215000409.GM13436@magnolia> (raw)
In-Reply-To: <20171214220326.GI5858@dastard>
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@oracle.com>
> >
> > Refactor the callers of verifiers to print the instruction address of a
> > failing check.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> 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@fromorbit.com
next prev parent reply other threads:[~2017-12-15 0:04 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-13 23:58 [PATCH 00/13] xfs: more and better verifiers Darrick J. Wong
2017-12-13 23:58 ` [PATCH 01/13] xfs: refactor long-format btree header verification routines Darrick J. Wong
2017-12-14 22:06 ` Dave Chinner
2017-12-15 0:12 ` Darrick J. Wong
2017-12-13 23:58 ` [PATCH 02/13] xfs: remove XFS_WANT_CORRUPTED_RETURN from dir3 data verifiers Darrick J. Wong
2017-12-19 3:50 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 03/13] xfs: have buffer verifier functions report failing address Darrick J. Wong
2017-12-19 4:12 ` Dave Chinner
2017-12-19 20:26 ` Darrick J. Wong
2017-12-13 23:58 ` [PATCH 04/13] xfs: refactor verifier callers to print address of failing check Darrick J. Wong
2017-12-14 22:03 ` Dave Chinner
2017-12-15 0:04 ` Darrick J. Wong [this message]
2017-12-15 3:09 ` Dave Chinner
2017-12-19 20:29 ` Darrick J. Wong
2017-12-13 23:58 ` [PATCH 05/13] xfs: verify dinode header first Darrick J. Wong
2017-12-19 4:13 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 06/13] xfs: move inode fork verifiers to xfs_dinode_verify Darrick J. Wong
2017-12-19 5:16 ` Dave Chinner
2017-12-19 20:34 ` Darrick J. Wong
2017-12-19 20:48 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 07/13] xfs: create structure verifier function for shortform xattrs Darrick J. Wong
2017-12-19 5:23 ` Dave Chinner
2017-12-19 20:41 ` Darrick J. Wong
2017-12-19 20:51 ` Dave Chinner
2017-12-19 21:04 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 08/13] xfs: create structure verifier function for short form symlinks Darrick J. Wong
2017-12-19 5:27 ` Dave Chinner
2017-12-19 20:45 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 09/13] xfs: refactor short form directory structure verifier function Darrick J. Wong
2017-12-19 5:45 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 10/13] xfs: provide a centralized method for verifying inline fork data Darrick J. Wong
2017-12-19 6:06 ` Dave Chinner
2017-12-19 20:50 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 11/13] xfs: fail out of xfs_attr3_leaf_lookup_int if it looks corrupt Darrick J. Wong
2017-12-19 6:13 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 12/13] xfs: create a new buf_ops pointer to verify structure metadata Darrick J. Wong
2017-12-19 6:22 ` Dave Chinner
2017-12-19 18:15 ` Darrick J. Wong
2017-12-19 20:53 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 13/13] xfs: scrub in-core metadata Darrick J. Wong
2017-12-19 6:23 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171215000409.GM13436@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox