On Tue, Apr 20, 2021 at 07:08:55PM +0800, Gao Xiang wrote: > As Dave mentioned [1], "/me is now wondering why we even bother > with !lazy-count anymore. > > We've updated the agr btree block accounting unconditionally since > lazy-count was added, and scrub will always report a mismatch in > counts if they exist regardless of lazy-count. So why don't we just > start ignoring the on-disk value and always use lazy-count based > updates? " > > Therefore, turn on lazy sb counters if it's still disabled at the > mount time, or at remount_rw if fs was mounted as read-only. > xfs_initialize_perag_data() is reused here since no need to scan > agf/agi once more again. > > After this patch, we could get rid of this whole set of subtle > conditional behaviours in the codebase. > > [1] https://lore.kernel.org/r/20210417223201.GU63242@xxxxxxxxxxxxxxxxxxx > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx> > --- > Enabling lazysbcount is only addressed in this patch, I'll send > out a seperated following patch to cleanup all unused conditions > later. > > Also tr_sb is reused here since only agf is modified for each ag, > and before lazysbcount sb feature is enabled (m_update_sb = true), > agf_btreeblks field shouldn't matter for such AGs. > > fs/xfs/libxfs/xfs_format.h | 6 +++ > fs/xfs/libxfs/xfs_sb.c | 93 +++++++++++++++++++++++++++++++++++--- > fs/xfs/xfs_mount.c | 2 +- > fs/xfs/xfs_super.c | 5 ++ > 4 files changed, 98 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 76e2461b9e66..9081d7876d66 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -385,6 +385,12 @@ static inline bool xfs_sb_version_haslazysbcount(struct xfs_sb *sbp) > (sbp->sb_features2 & XFS_SB_VERSION2_LAZYSBCOUNTBIT)); > } > > +static inline void xfs_sb_version_addlazysbcount(struct xfs_sb *sbp) > +{ > + sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT; > + sbp->sb_features2 |= XFS_SB_VERSION2_LAZYSBCOUNTBIT; > +} > + > static inline bool xfs_sb_version_hasattr2(struct xfs_sb *sbp) > { > return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) || > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 423dada3f64c..6353e0d4cab1 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -18,6 +18,7 @@ > #include "xfs_trace.h" > #include "xfs_trans.h" > #include "xfs_buf_item.h" > +#include "xfs_btree.h" > #include "xfs_bmap_btree.h" > #include "xfs_alloc_btree.h" > #include "xfs_log.h" > @@ -841,6 +842,55 @@ xfs_sb_mount_common( > mp->m_ag_max_usable = xfs_alloc_ag_max_usable(mp); > } > > +static int > +xfs_fixup_agf_btreeblks( > + struct xfs_mount *mp, > + struct xfs_trans *tp, > + struct xfs_buf *agfbp, > + xfs_agnumber_t agno) > +{ > + struct xfs_btree_cur *cur; > + struct xfs_perag *pag = agfbp->b_pag; > + struct xfs_agf *agf = agfbp->b_addr; > + xfs_agblock_t btreeblks, blocks; > + int error; > + > + cur = xfs_allocbt_init_cursor(mp, tp, agfbp, agno, XFS_BTNUM_BNO); > + error = xfs_btree_count_blocks(cur, &blocks); > + if (error) > + goto err; > + xfs_btree_del_cursor(cur, error); > + btreeblks = blocks - 1; > + > + cur = xfs_allocbt_init_cursor(mp, tp, agfbp, agno, XFS_BTNUM_CNT); > + error = xfs_btree_count_blocks(cur, &blocks); > + if (error) > + goto err; > + xfs_btree_del_cursor(cur, error); > + btreeblks += blocks - 1; > + > + /* > + * although rmapbt doesn't exist in v4 fses, but it'd be better > + * to turn it as a generic helper. > + */ > + if (xfs_sb_version_hasrmapbt(&mp->m_sb)) { > + cur = xfs_rmapbt_init_cursor(mp, tp, agfbp, agno); > + error = xfs_btree_count_blocks(cur, &blocks); > + if (error) > + goto err; > + xfs_btree_del_cursor(cur, error); > + btreeblks += blocks - 1; > + } > + > + agf->agf_btreeblks = cpu_to_be32(btreeblks); > + pag->pagf_btreeblks = btreeblks; > + xfs_alloc_log_agf(tp, agfbp, XFS_AGF_BTREEBLKS); > + return 0; > +err: > + xfs_btree_del_cursor(cur, error); > + return error; > +} > + > /* > * xfs_initialize_perag_data > * > @@ -864,27 +914,51 @@ xfs_initialize_perag_data( > uint64_t btree = 0; > uint64_t fdblocks; > int error = 0; > + bool conv = !(mp->m_flags & XFS_MOUNT_RDONLY) && > + !xfs_sb_version_haslazysbcount(sbp); > + > + if (conv) > + xfs_warn(mp, "enabling lazy-counters..."); > > for (index = 0; index < agcount; index++) { > + struct xfs_trans *tp = NULL; > + struct xfs_buf *agfbp; > + > + if (conv) { > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, > + 0, 0, 0, &tp); > + if (error) > + return error; > + } > + > /* > - * read the agf, then the agi. This gets us > + * read the agi, then the agf. This gets us > * all the information we need and populates the > * per-ag structures for us. > */ > - error = xfs_alloc_pagf_init(mp, NULL, index, 0); > - if (error) > + error = xfs_ialloc_pagi_init(mp, tp, index); > + if (error) { > +err_out: > + if (tp) > + xfs_trans_cancel(tp); > return error; > + } > > - error = xfs_ialloc_pagi_init(mp, NULL, index); > + error = xfs_alloc_read_agf(mp, tp, index, 0, &agfbp); > if (error) > - return error; > - pag = xfs_perag_get(mp, index); > + goto err_out; > + pag = agfbp->b_pag; > ifree += pag->pagi_freecount; > ialloc += pag->pagi_count; > bfree += pag->pagf_freeblks; > bfreelst += pag->pagf_flcount; > + if (tp) { > + error = xfs_fixup_agf_btreeblks(mp, tp, agfbp, index); Lazysbcount upgrades should be done from a separate function, not mixed in with perag initialization. Also, why is it necessary to walk all the space btrees to set agf_btreeblks? > + xfs_trans_commit(tp); > + } else { > + xfs_buf_relse(agfbp); > + } > btree += pag->pagf_btreeblks; > - xfs_perag_put(pag); > } > fdblocks = bfree + bfreelst + btree; > > @@ -900,6 +974,11 @@ xfs_initialize_perag_data( > goto out; > } > > + if (conv) { > + xfs_sb_version_addlazysbcount(sbp); > + mp->m_update_sb = true; > + xfs_warn(mp, "lazy-counters has been enabled."); But we don't log the sb update? As far as the feature upgrade goes, is it necessary to bwrite the primary super to disk (and then log the change)[1] to prevent a truly ancient kernel that doesn't support lazysbcount from trying to recover the log and ending up with an unsupported feature set? [1] https://lore.kernel.org/linux-xfs/161723934343.3149451.16679733325094950568.stgit@magnolia/ > + } > /* Overwrite incore superblock counters with just-read data */ > spin_lock(&mp->m_sb_lock); > sbp->sb_ifree = ifree; > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index cb1e2c4702c3..b3b13acd45d6 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -626,7 +626,7 @@ xfs_check_summary_counts( > * superblock to be correct and we don't need to do anything here. > * Otherwise, recalculate the summary counters. > */ > - if ((!xfs_sb_version_haslazysbcount(&mp->m_sb) || > + if ((xfs_sb_version_haslazysbcount(&mp->m_sb) && Not clear why the logic here inverts? --D > XFS_LAST_UNMOUNT_WAS_CLEAN(mp)) && > !xfs_fs_has_sickness(mp, XFS_SICK_FS_COUNTERS)) > return 0; > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index a2dab05332ac..16197a890c15 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1678,6 +1678,11 @@ xfs_remount_rw( > } > > mp->m_flags &= ~XFS_MOUNT_RDONLY; > + if (!xfs_sb_version_haslazysbcount(sbp)) { > + error = xfs_initialize_perag_data(mp, sbp->sb_agcount); > + if (error) > + return error; > + } > > /* > * If this is the first remount to writeable state we might have some > -- > 2.27.0 >
