xfs
[Top] [All Lists]

Re: [PATCH 9/9] xfs: modify verifiers to differentiate CRC from other er

To: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 9/9] xfs: modify verifiers to differentiate CRC from other errors
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 19 Feb 2014 09:01:37 -0500
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1392767549-25574-10-git-send-email-sandeen@xxxxxxxxxx>
References: <1392767549-25574-1-git-send-email-sandeen@xxxxxxxxxx> <1392767549-25574-10-git-send-email-sandeen@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
On 02/18/2014 06:52 PM, Eric Sandeen wrote:
> Modify all read & write verifiers to differentiate
> between CRC errors and other inconsistencies.
> 
> This sets the appropriate error number on bp->b_error,
> and then calls xfs_verifier_error() if something went
> wrong.  That function will issue the appropriate message
> to the user.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_alloc.c          |   37 +++++++++++++++++--------------------
>  fs/xfs/xfs_alloc_btree.c    |   15 ++++++++-------
>  fs/xfs/xfs_attr_leaf.c      |   14 ++++++++------
>  fs/xfs/xfs_attr_remote.c    |   15 ++++++---------
>  fs/xfs/xfs_bmap_btree.c     |   16 ++++++++--------
>  fs/xfs/xfs_da_btree.c       |   14 ++++++++------
>  fs/xfs/xfs_dir2_block.c     |   14 ++++++++------
>  fs/xfs/xfs_dir2_data.c      |   17 +++++++++--------
>  fs/xfs/xfs_dir2_leaf.c      |   14 ++++++++------
>  fs/xfs/xfs_dir2_node.c      |   14 ++++++++------
>  fs/xfs/xfs_dquot_buf.c      |   11 +++++++----
>  fs/xfs/xfs_ialloc.c         |   12 ++++++++----
>  fs/xfs/xfs_ialloc_btree.c   |   15 ++++++++-------
>  fs/xfs/xfs_inode_buf.c      |    3 +--
>  fs/xfs/xfs_sb.c             |   10 ++++------
>  fs/xfs/xfs_symlink_remote.c |   12 +++++++-----
>  16 files changed, 123 insertions(+), 110 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9c7cf3d..9a93601 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -474,7 +474,6 @@ xfs_agfl_read_verify(
>       struct xfs_buf  *bp)
>  {
>       struct xfs_mount *mp = bp->b_target->bt_mount;
> -     int             agfl_ok = 1;
>  
>       /*
>        * There is no verification of non-crc AGFLs because mkfs does not
> @@ -485,14 +484,13 @@ xfs_agfl_read_verify(
>       if (!xfs_sb_version_hascrc(&mp->m_sb))
>               return;
>  
> -     agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
> -
> -     agfl_ok = agfl_ok && xfs_agfl_verify(bp);
> -
> -     if (!agfl_ok) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 
> bp->b_addr);
> +     if (!xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc)))
> +             xfs_buf_ioerror(bp, EFSBADCRC);
> +     else if (!xfs_agfl_verify(bp))

Obviously you added the CRC_OFF directives earlier in the set. It looks
like this patch squashed a couple of them (XFS_AGF_CRC_OFF as well).

>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> -     }
> +
> +     if (bp->b_error)
> +             xfs_verifier_error(bp);
>  }
>  
...
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index 4657586..8aa720d 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -1573,13 +1573,17 @@ xfs_agi_read_verify(
>       if (xfs_sb_version_hascrc(&mp->m_sb))
>               agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
>  
> +     if (!agi_ok)
> +             xfs_buf_ioerror(bp, EFSBADCRC);
> +
>       agi_ok = agi_ok && xfs_agi_verify(bp);
>  
>       if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
> -                     XFS_RANDOM_IALLOC_READ_AGI))) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 
> bp->b_addr);
> +                     XFS_RANDOM_IALLOC_READ_AGI)))
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> -     }
> +
> +     if (bp->b_error)
> +             xfs_verifier_error(bp);
>  }

Any reason not to use the same if/else pattern here that the others are
now using (i.e., similar to xfs_agf_read_verify(), removing the need for
agi_ok)?

Brian

>  
>  static void
> @@ -1590,8 +1594,8 @@ xfs_agi_write_verify(
>       struct xfs_buf_log_item *bip = bp->b_fspriv;
>  
>       if (!xfs_agi_verify(bp)) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 
> bp->b_addr);
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> +             xfs_verifier_error(bp);
>               return;
>       }
>  
> diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
> index 0028c50..7e309b1 100644
> --- a/fs/xfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/xfs_ialloc_btree.c
> @@ -243,12 +243,14 @@ static void
>  xfs_inobt_read_verify(
>       struct xfs_buf  *bp)
>  {
> -     if (!(xfs_btree_sblock_verify_crc(bp) &&
> -           xfs_inobt_verify(bp))) {
> -             trace_xfs_btree_corrupt(bp, _RET_IP_);
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -                                  bp->b_target->bt_mount, bp->b_addr);
> +     if (!xfs_btree_sblock_verify_crc(bp))
> +             xfs_buf_ioerror(bp, EFSBADCRC);
> +     else if (!xfs_inobt_verify(bp))
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> +
> +     if (bp->b_error) {
> +             trace_xfs_btree_corrupt(bp, _RET_IP_);
> +             xfs_verifier_error(bp);
>       }
>  }
>  
> @@ -258,9 +260,8 @@ xfs_inobt_write_verify(
>  {
>       if (!xfs_inobt_verify(bp)) {
>               trace_xfs_btree_corrupt(bp, _RET_IP_);
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -                                  bp->b_target->bt_mount, bp->b_addr);
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> +             xfs_verifier_error(bp);
>               return;
>       }
>       xfs_btree_sblock_calc_crc(bp);
> diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
> index 606b43a..24e9939 100644
> --- a/fs/xfs/xfs_inode_buf.c
> +++ b/fs/xfs/xfs_inode_buf.c
> @@ -102,8 +102,7 @@ xfs_inode_buf_verify(
>                       }
>  
>                       xfs_buf_ioerror(bp, EFSCORRUPTED);
> -                     XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
> -                                          mp, dip);
> +                     xfs_verifier_error(bp);
>  #ifdef DEBUG
>                       xfs_alert(mp,
>                               "bad inode magic/vsn daddr %lld #%d (magic=%x)",
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 818359f..b134aa8 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -614,7 +614,7 @@ xfs_sb_read_verify(
>                       /* Only fail bad secondaries on a known V5 filesystem */
>                       if (bp->b_bn == XFS_SB_DADDR ||
>                           xfs_sb_version_hascrc(&mp->m_sb)) {
> -                             error = EFSCORRUPTED;
> +                             error = EFSBADCRC;
>                               goto out_error;
>                       }
>               }
> @@ -623,10 +623,9 @@ xfs_sb_read_verify(
>  
>  out_error:
>       if (error) {
> -             if (error == EFSCORRUPTED)
> -                     XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -                                          mp, bp->b_addr);
>               xfs_buf_ioerror(bp, error);
> +             if (error == EFSCORRUPTED || error == EFSBADCRC)
> +                     xfs_verifier_error(bp);
>       }
>  }
>  
> @@ -661,9 +660,8 @@ xfs_sb_write_verify(
>  
>       error = xfs_sb_verify(bp, false);
>       if (error) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -                                  mp, bp->b_addr);
>               xfs_buf_ioerror(bp, error);
> +             xfs_verifier_error(bp);
>               return;
>       }
>  
> diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
> index defa09f..9b32052 100644
> --- a/fs/xfs/xfs_symlink_remote.c
> +++ b/fs/xfs/xfs_symlink_remote.c
> @@ -133,11 +133,13 @@ xfs_symlink_read_verify(
>       if (!xfs_sb_version_hascrc(&mp->m_sb))
>               return;
>  
> -     if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
> -         !xfs_symlink_verify(bp)) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 
> bp->b_addr);
> +     if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF))
> +             xfs_buf_ioerror(bp, EFSBADCRC);
> +     else if (!xfs_symlink_verify(bp))
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> -     }
> +
> +     if (bp->b_error)
> +             xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -152,8 +154,8 @@ xfs_symlink_write_verify(
>               return;
>  
>       if (!xfs_symlink_verify(bp)) {
> -             XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 
> bp->b_addr);
>               xfs_buf_ioerror(bp, EFSCORRUPTED);
> +             xfs_verifier_error(bp);
>               return;
>       }
>  
> 

<Prev in Thread] Current Thread [Next in Thread>