xfs
[Top] [All Lists]

Re: [PATCH 5/9] repair: detect CRC errors in AG headers

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/9] repair: detect CRC errors in AG headers
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Tue, 15 Apr 2014 15:40:29 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1397550301-31883-6-git-send-email-david@xxxxxxxxxxxxx>
References: <1397550301-31883-1-git-send-email-david@xxxxxxxxxxxxx> <1397550301-31883-6-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Apr 15, 2014 at 06:24:57PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> repair doesn't currently detect verifier errors in AG header
> blocks - apart from the primary superblock they are not detected.
> They are, fortunately, corrected in the important cases (AGF, AGI
> and AGFL) because these structures are rebuilt in phase 5, but if
> you run xfs_repair in checking mode it won't report them as bad.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  repair/scan.c | 66 
> ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 1744c32..6c43474 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -1207,28 +1207,31 @@ scan_ag(
>       void            *arg)
>  {
>       struct aghdr_cnts *agcnts = arg;
> -     xfs_agf_t       *agf;
> -     xfs_buf_t       *agfbuf;
> +     struct xfs_agf  *agf;
> +     struct xfs_buf  *agfbuf = NULL;
>       int             agf_dirty = 0;
> -     xfs_agi_t       *agi;
> -     xfs_buf_t       *agibuf;
> +     struct xfs_agi  *agi;
> +     struct xfs_buf  *agibuf = NULL;
>       int             agi_dirty = 0;
> -     xfs_sb_t        *sb;
> -     xfs_buf_t       *sbbuf;
> +     struct xfs_sb   *sb = NULL;
> +     struct xfs_buf  *sbbuf = NULL;
>       int             sb_dirty = 0;
>       int             status;
> +     char            *objname = NULL;
>  
>       sbbuf = libxfs_readbuf(mp->m_dev, XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
>                               XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);
>       if (!sbbuf)  {
> -             do_error(_("can't get root superblock for ag %d\n"), agno);
> -             return;
> +             objname = _("root superblock");
> +             goto out_free;
>       }
> +     if (sbbuf->b_error == EFSBADCRC || sbbuf->b_error == EFSCORRUPTED)
> +             sb_dirty = 1;
> +
>       sb = (xfs_sb_t *)calloc(BBSIZE, 1);
>       if (!sb) {
>               do_error(_("can't allocate memory for superblock\n"));
> -             libxfs_putbuf(sbbuf);
> -             return;
> +             goto out_free;
>       }
>       libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbbuf));
>  
> @@ -1236,23 +1239,22 @@ scan_ag(
>                       XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
>                       XFS_FSS_TO_BB(mp, 1), 0, &xfs_agf_buf_ops);
>       if (!agfbuf)  {
> -             do_error(_("can't read agf block for ag %d\n"), agno);
> -             libxfs_putbuf(sbbuf);
> -             free(sb);
> -             return;
> +             objname = _("agf block");
> +             goto out_free;
>       }
> +     if (agfbuf->b_error == EFSBADCRC || agfbuf->b_error == EFSCORRUPTED)
> +             agf_dirty = 1;
>       agf = XFS_BUF_TO_AGF(agfbuf);
>  
>       agibuf = libxfs_readbuf(mp->m_dev,
>                       XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp)),
>                       XFS_FSS_TO_BB(mp, 1), 0, &xfs_agi_buf_ops);
>       if (!agibuf)  {
> -             do_error(_("can't read agi block for ag %d\n"), agno);
> -             libxfs_putbuf(agfbuf);
> -             libxfs_putbuf(sbbuf);
> -             free(sb);
> -             return;
> +             objname = _("agi block");
> +             goto out_free;
>       }
> +     if (agibuf->b_error == EFSBADCRC || agibuf->b_error == EFSCORRUPTED)
> +             agi_dirty = 1;
>       agi = XFS_BUF_TO_AGI(agibuf);
>  
>       /* fix up bad ag headers */
> @@ -1277,7 +1279,7 @@ scan_ag(
>                       do_warn(_("would reset bad sb for ag %d\n"), agno);
>               }
>       }
> -     if (status & XR_AG_AGF)  {
> +     if (agf_dirty || status & XR_AG_AGF)  {
>               if (!no_modify) {
>                       do_warn(_("reset bad agf for ag %d\n"), agno);
>                       agf_dirty = 1;
> @@ -1285,7 +1287,7 @@ scan_ag(
>                       do_warn(_("would reset bad agf for ag %d\n"), agno);
>               }
>       }
> -     if (status & XR_AG_AGI)  {
> +     if (agi_dirty || status & XR_AG_AGI)  {
>               if (!no_modify) {
>                       do_warn(_("reset bad agi for ag %d\n"), agno);
>                       agi_dirty = 1;

There are a few asserts a bit further down this function that assume
*_dirty is set only when in !no_modify mode. E.g.:

        ASSERT(agi_dirty == 0 || (agi_dirty && !no_modify));

You'll probably want to remove those. Or...

> @@ -1295,15 +1297,9 @@ scan_ag(
>       }
>  
>       if (status && no_modify)  {
> -             libxfs_putbuf(agibuf);
> -             libxfs_putbuf(agfbuf);
> -             libxfs_putbuf(sbbuf);
> -             free(sb);
> -
>               do_warn(_("bad uncorrected agheader %d, skipping ag...\n"),
>                       agno);
> -
> -             return;
> +             goto out_free;
>       }

Would we want to skip the ag, as such, on a CRC error in no_modify mode?
If so, perhaps we could set the status variable on crc errors and
bitwise or the value returned from verify_set_agheader().

Brian

>  
>       scan_freelist(agf, agcnts);
> @@ -1341,6 +1337,20 @@ scan_ag(
>       print_inode_list(i);
>  #endif
>       return;
> +
> +out_free:
> +     if (sb)
> +             free(sb);
> +     if (agibuf)
> +             libxfs_putbuf(agibuf);
> +     if (agfbuf)
> +             libxfs_putbuf(agfbuf);
> +     if (sbbuf)
> +             libxfs_putbuf(sbbuf);
> +     if (objname)
> +             do_error(_("can't get %s for ag %d\n"), objname, agno);
> +     return;
> +
>  }
>  
>  #define SCAN_THREADS 32
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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