Re: [PATCH 18/30] xfs: scrub inode btrees

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 18/30] xfs: scrub inode btrees
Date: Mon, 16 Oct 2017 15:16:22 -0700	[thread overview]
Message-ID: <20171016221622.GP4703@magnolia> (raw)
In-Reply-To: <20171016025559.GU3666@dastard>

On Mon, Oct 16, 2017 at 01:55:59PM +1100, Dave Chinner wrote:
> On Wed, Oct 11, 2017 at 06:42:41PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Check the records of the inode btrees to make sure that the values
> > make sense given the inode records themselves.
> 
> .....
> 
> I think maybe I missed this first time around...
> 
> > +/* Check a particular inode with ir_free. */
> > +STATIC int
> > +xfs_scrub_iallocbt_check_cluster_freemask(
> > +	struct xfs_scrub_btree		*bs,
> > +	xfs_ino_t			fsino,
> > +	xfs_agino_t			chunkino,
> > +	xfs_agino_t			clusterino,
> > +	struct xfs_inobt_rec_incore	*irec,
> > +	struct xfs_buf			*bp)
> > +{
> > +	struct xfs_dinode		*dip;
> > +	struct xfs_mount		*mp = bs->cur->bc_mp;
> > +	bool				freemask_ok;
> > +	bool				inuse;
> > +	int				error;
> > +
> > +	dip = xfs_buf_offset(bp, clusterino * mp->m_sb.sb_inodesize);
> > +	if (be16_to_cpu(dip->di_magic) != XFS_DINODE_MAGIC ||
> > +	    (dip->di_version >= 3 &&
> > +	     be64_to_cpu(dip->di_ino) != fsino + clusterino)) {
> > +		xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +		goto out;
> > +	}
> > +
> > +	freemask_ok = (irec->ir_free & XFS_INOBT_MASK(chunkino + clusterino));
> 
> Ok, so if the inode if free, the corresponding bit in the mask
> will be set....
> 
> > +	error = xfs_icache_inode_is_allocated(mp, bs->cur->bc_tp,
> > +			fsino + clusterino, &inuse);
> > +	if (error == -ENODATA) {
> > +		/* Not cached, just read the disk buffer */
> > +		freemask_ok ^= !!(dip->di_mode);
> 
> And this uses the lowest bit of the mask? How does that work?
> 
> /me spends 10 minutes trying to work out this function before he
> realises that freemask_ok is a boolean, so the initial freemask
> bit is collapsed down to a single bit.
> 
> Ok, that's definitely unexpected and not obvious from the code or
> comments. The name "freemask_ok" is misleading the way it's used.
> The first time it is set is means "inode is free", then after this
> operation it means "inode matches free mask"....

Ugh, sorry.  Thank you for complaining about this!

> 
> > +		if (!bs->sc->try_harder && !freemask_ok)
> > +			return -EDEADLOCK;
> > +	} else if (error < 0) {
> > +		/*
> > +		 * Inode is only half assembled, or there was an IO error,
> > +		 * or the verifier failed, so don't bother trying to check.
> > +		 * The inode scrubber can deal with this.
> > +		 */
> > +		freemask_ok = true;
> 
> And here it means "we didn't check the free mask"
> 
> > +	} else {
> > +		/* Inode is all there. */
> > +		freemask_ok ^= inuse;
> 
> And here is means "inode matches free mask" again....
> 
> > +	}
> > +	if (!freemask_ok)
> > +		xfs_scrub_btree_set_corrupt(bs->sc, bs->cur, 0);
> > +out:
> > +	return 0;
> 
> Can we rewrite this to be a little more obvious?
> 
> 	bool		inode_is_free = false;
> 	bool		freemask_ok;
> ....
> 	if (irec->ir_free & XFS_INOBT_MASK(chunkino + clusterino))
> 		inode_is_free = true;
> ....
> 	if (error == -ENODATA) {
> 		freemask_ok = inode_is_free ^ !!(dip->di_mode);
> ....
> 	else if (error < 0) {
> 		/*
> 		 * Inode is only half assembled, .....
> 		 */
> 		goto out;
> 	} else {
> 		freemask_ok = inode_is_free ^ inuse;
> 	}
> 
> That's a lot more obvious what the code is checking...

Done.

> 
> > +/* Scrub an inobt/finobt record. */
> > +STATIC int
> > +xfs_scrub_iallocbt_helper(
> 
> s/helper/rec/

Done.

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html