Re: [PATCH 01/22] xfs: add helpers to deal with transaction allocation and rolling

[Dave Chinner <david@fromorbit.com>]

On Wed, May 16, 2018 at 09:46:45AM -0700, Darrick J. Wong wrote:
> On Wed, May 16, 2018 at 04:51:03PM +1000, Dave Chinner wrote:
> > On Tue, May 15, 2018 at 03:33:45PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > For repairs, we need to reserve at least as many blocks as we think
> > > we're going to need to rebuild the data structure, and we're going to
> > > need some helpers to roll transactions while maintaining locks on the AG
> > > headers so that other threads cannot wander into the middle of a repair.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/scrub/bmap.c   |    2 -
> > >  fs/xfs/scrub/common.c |   21 ++++++-
> > >  fs/xfs/scrub/common.h |    2 -
> > >  fs/xfs/scrub/inode.c  |    4 +
> > >  fs/xfs/scrub/repair.c |  152 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/xfs/scrub/repair.h |   12 ++++
> > >  6 files changed, 186 insertions(+), 7 deletions(-)
> > 
> > mostly looks good.
> > 
> > [...]
> > > +			freelen, usedlen);
> > > +
> > > +	/*
> > > +	 * Figure out how many blocks we'd need worst case to rebuild
> > > +	 * each type of btree.  Note that we can only rebuild the
> > > +	 * bnobt/cntbt or inobt/finobt as pairs.
> > > +	 */
> > > +	bnobt_sz = 2 * xfs_allocbt_calc_size(mp, freelen);
> > > +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +		inobt_sz = xfs_iallocbt_calc_size(mp, icount /
> > > +				XFS_INODES_PER_HOLEMASK_BIT);
> > > +	else
> > > +		inobt_sz = xfs_iallocbt_calc_size(mp, icount /
> > > +				XFS_INODES_PER_CHUNK);
> > > +	if (xfs_sb_version_hasfinobt(&mp->m_sb))
> > > +		inobt_sz *= 2;
> > > +	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> > > +		rmapbt_sz = xfs_rmapbt_calc_size(mp, aglen);
> > > +		refcbt_sz = xfs_refcountbt_calc_size(mp, usedlen);
> > > +	} else {
> > > +		rmapbt_sz = xfs_rmapbt_calc_size(mp, usedlen);
> > > +		refcbt_sz = 0;
> > > +	}
> > > +	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
> > > +		rmapbt_sz = 0;
> > 
> > This looks kinda whacky. reflink and rmapbt are different features,
> > maybe a comment to explain why the rmapbt size calc is done all
> > back to front?
> 
> Yeah.  Replace that whole section with:
> 
> 	if (xfs_sb_version_hasreflink(&mp->m_sb))
> 		refcbt_sz = xfs_refcountbt_calc_size(mp, usedlen);
> 	else
> 		refcbt_sz = 0;
> 	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
> 		/*
> 		 * Guess how many blocks we need to rebuild the rmapbt.
> 		 * For non-reflink filesystems we can't have more
> 		 * records than used blocks.  However, with reflink it's
> 		 * possible to have more than one rmap record per AG
> 		 * block.  We don't know how many rmaps there could be
> 		 * in the AG, so we start off with what we hope is an
> 		 * generous over-estimation.
> 		 */
> 		if (xfs_sb_version_hasreflink(&mp->m_sb))
> 			rmapbt_sz = xfs_rmapbt_calc_size(mp,
> 					(unsigned long long)aglen * 2);
> 		else
> 			rmapbt_sz = xfs_rmapbt_calc_size(mp, usedlen);
> 	} else {
> 		rmapbt_sz = 0;
> 	}

Yup, that's much better :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com