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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
