Re: [PATCH 19/28] repair: scan sparse finobt records correctly

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 05, 2015 at 11:03:02AM +1000, Dave Chinner wrote:
> On Tue, Jun 02, 2015 at 02:41:52PM -0400, Brian Foster wrote:
> > The finobt scan performs similar checks as to the inobt scan, including
> > internal record consistency checks, consistency with inobt records,
> > inode block state, etc. Various parts of this mechanism also assume
> > fully allocated inode records and thus lead to false errors with sparse
> > records.
> > 
> > Update the finobt scan to detect and handle sparse inode records
> > correctly. As for the inobt, do not assume that blocks associated with
> > sparse regions are allocated for inodes and do not account sparse inodes
> > against the freecount. Additionally, verify that sparse state is
> > consistent with the in-core record and set up any new in-core records
> > that might have been missing from the inobt correctly.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> ....
> >  
> > +	/*
> > +	 * Mark sparse inodes as such in the in-core tree. Verify that sparse
> > +	 * inodes are free and that freecount is consistent with the free mask.
> > +	 */
> > +	nfree = 0;
> > +	for (j = 0; j < XFS_INODES_PER_CHUNK; j++) {
> > +		if (ino_issparse(rp, j)) {
> > +			if (!suspect && !XFS_INOBT_IS_FREE_DISK(rp, j)) {
> > +				do_warn(
> > +_("finobt ir_holemask/ir_free mismatch, inode chunk %d/%u, holemask 0x%x free 0x%llx\n"),
> > +					agno, ino,
> > +					be16_to_cpu(rp->ir_u.sp.ir_holemask),
> > +					be64_to_cpu(rp->ir_free));
> > +				suspect++;
> > +			}
> > +			if (!suspect && ino_rec)
> > +				set_inode_sparse(ino_rec, j);
> > +		} else if (XFS_INOBT_IS_FREE_DISK(rp, j)) {
> > +			/* freecount only tracks non-sparse inos */
> > +			nfree++;
> > +		}
> > +	}
> > +
> 
> This is the same checking code as used for the inobt. Can you factor
> these into a helper? I'll apply as is, so delta patch again. ;)
> 

There's actually quite a bit of duplication throughout the inobt and
finobt record scan functions. I noticed this when doing the finobt stuff
but there were enough differences that it didn't seem worth the effort
at the time.

Looking back at it now with the sparse stuff added and whatnot, it's
probably worth refactoring. The only difference between much of the
logic is the error messages that distinguish between the trees (inobt
vs. finobt). I've made a pass through this code to create a couple
helpers for these functions and allow the caller to identify the tree
based on a simple enum parameter.

I'll get the patches for this and the other cleanups posted once they
get through some testing...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs