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

Dave Chinner
2015-06-05T01:12:15Z

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@redhat.com>

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@fromorbit.com