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.
>
> [...]
> > +/*
> > + * Figure out how many blocks to reserve for an AG repair. We calculate the
> > + * worst case estimate for the number of blocks we'd need to rebuild one of
> > + * any type of per-AG btree.
> > + */
> > +xfs_extlen_t
> > +xfs_repair_calc_ag_resblks(
> > + struct xfs_scrub_context *sc)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + struct xfs_scrub_metadata *sm = sc->sm;
> > + struct xfs_perag *pag;
> > + struct xfs_buf *bp;
> > + xfs_agino_t icount = 0;
> > + xfs_extlen_t aglen = 0;
> > + xfs_extlen_t usedlen;
> > + xfs_extlen_t freelen;
> > + xfs_extlen_t bnobt_sz;
> > + xfs_extlen_t inobt_sz;
> > + xfs_extlen_t rmapbt_sz;
> > + xfs_extlen_t refcbt_sz;
> > + int error;
> > +
> > + if (!(sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
> > + return 0;
> > +
> > + /* Use in-core counters if possible. */
> > + pag = xfs_perag_get(mp, sm->sm_agno);
> > + if (pag->pagi_init)
> > + icount = pag->pagi_count;
> > + xfs_perag_put(pag);
>
> Don't drop the pag here, do it ....
> > +
> > + /*
> > + * Otherwise try to get the actual counters from disk; if not, make
> > + * some worst case assumptions.
> > + */
> > + if (icount == 0) {
> > + error = xfs_ialloc_read_agi(mp, NULL, sm->sm_agno, &bp);
> > + if (error) {
> > + icount = mp->m_sb.sb_agblocks / mp->m_sb.sb_inopblock;
> > + } else {
> > + pag = xfs_perag_get(mp, sm->sm_agno);
> > + icount = pag->pagi_count;
> > + xfs_perag_put(pag);
> > + xfs_buf_relse(bp);
> > + }
> > + }
> > +
> > + /* Now grab the block counters from the AGF. */
> > + error = xfs_alloc_read_agf(mp, NULL, sm->sm_agno, 0, &bp);
> > + if (error) {
> > + aglen = mp->m_sb.sb_agblocks;
> > + freelen = aglen;
> > + usedlen = aglen;
> > + } else {
> > + pag = xfs_perag_get(mp, sm->sm_agno);
> > + aglen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_length);
> > + freelen = pag->pagf_freeblks;
> > + usedlen = aglen - freelen;
> > + xfs_perag_put(pag);
> > + xfs_buf_relse(bp);
> > + }
>
> .... here, so we don't have to do repeated lookups.
Ok, fixed.
> > + trace_xfs_repair_calc_ag_resblks(mp, sm->sm_agno, icount, aglen,
> > + 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;
}
--D
>
> Otherwise looks ok.
>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> --
> 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
--
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
