xfs
[Top] [All Lists]

Re: [PATCH 21/28] repair: process sparse inode records correctly

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 21/28] repair: process sparse inode records correctly
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 5 Jun 2015 11:12:10 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1433270521-62026-22-git-send-email-bfoster@xxxxxxxxxx>
References: <1433270521-62026-1-git-send-email-bfoster@xxxxxxxxxx> <1433270521-62026-22-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Jun 02, 2015 at 02:41:54PM -0400, Brian Foster wrote:
> The inode processing phases of xfs_repair (3 and 4) validate the actual
> inodes referred to by the previously scanned inode btrees. The physical
> inodes are read from disk and internally validated in various ways. The
> inode block state is also verified and corrected if necessary.
> 
> Sparse inodes are not physically allocated and the associated blocks may
> be allocated to any other area of the fs (file data, internal use,
> etc.). Attempts to validate these blocks as inode blocks produce noisy
> corruption errors.
> 
> Update the inode processing mechanism to handle sparse inode records
> correctly. Since sparse inodes do not exist, the general approach here
> is to simply skip validation of sparse inodes. Update
> process_inode_chunk() to skip reads of sparse clusters and set the buf
> pointer of associated clusters to NULL. Update the rest of the function
> to only verify non-NULL cluster buffers. Also, skip the inode block
> state checks for blocks in sparse inode clusters.
> 
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>

Code looks good, but in looking at this, another helper is in order:

> @@ -736,35 +757,41 @@ process_inode_chunk(
>       /*
>        * mark block as an inode block in the incore bitmap
>        */
> -     pthread_mutex_lock(&ag_locks[agno].lock);
> -     state = get_bmap(agno, agbno);
> -     switch (state) {
> -     case XR_E_INO:  /* already marked */
> -             break;
> -     case XR_E_UNKNOWN:
> -     case XR_E_FREE:
> -     case XR_E_FREE1:
> -             set_bmap(agno, agbno, XR_E_INO);
> -             break;
> -     case XR_E_BAD_STATE:
> -             do_error(_("bad state in block map %d\n"), state);
> -             break;
> -     default:
> -             set_bmap(agno, agbno, XR_E_MULT);
> -             do_warn(_("inode block %" PRIu64 " multiply claimed, state was 
> %d\n"),
> -                     XFS_AGB_TO_FSB(mp, agno, agbno), state);
> -             break;
> +     if (!is_inode_sparse(ino_rec, irec_offset)) {
> +             pthread_mutex_lock(&ag_locks[agno].lock);
> +             state = get_bmap(agno, agbno);
> +             switch (state) {
> +             case XR_E_INO:  /* already marked */
> +                     break;
> +             case XR_E_UNKNOWN:
> +             case XR_E_FREE:
> +             case XR_E_FREE1:
> +                     set_bmap(agno, agbno, XR_E_INO);
> +                     break;
> +             case XR_E_BAD_STATE:
> +                     do_error(_("bad state in block map %d\n"), state);
> +                     break;
> +             default:
> +                     set_bmap(agno, agbno, XR_E_MULT);
> +                     do_warn(
> +             _("inode block %" PRIu64 " multiply claimed, state was %d\n"),
> +                             XFS_AGB_TO_FSB(mp, agno, agbno), state);
> +                     break;
> +             }
> +             pthread_mutex_unlock(&ag_locks[agno].lock);
>       }
> -     pthread_mutex_unlock(&ag_locks[agno].lock);

This state update code is repeated and has an indentical
modification later in the patch, so can you factor
it into a helper again? (delta patch!)

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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