On Mon, Apr 29, 2019 at 08:00:29AM -0400, Brian Foster wrote:
> On Sun, Apr 28, 2019 at 03:10:06PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Teach online scrub how to check the filesystem summary counters. We use
> > the incore delalloc block counter along with the incore AG headers to
> > compute expected values for fdblocks, icount, and ifree, and then check
> > that the percpu counter is within a certain threshold of the expected
> > value. This is done to avoid having to freeze or otherwise lock the
> > filesystem, which means that we're only checking that the counters are
> > fairly close, not that they're exactly correct.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > v2: move all the io generating calls into a warmup function
> > v3: tighten the aggregation loop and rework the threshold check function
> > ---
> > fs/xfs/Makefile | 1
> > fs/xfs/libxfs/xfs_fs.h | 3
> > fs/xfs/libxfs/xfs_types.c | 2
> > fs/xfs/libxfs/xfs_types.h | 2
> > fs/xfs/scrub/common.c | 9 +
> > fs/xfs/scrub/common.h | 2
> > fs/xfs/scrub/fscounters.c | 360 +++++++++++++++++++++++++++++++++++++++++++++
> > fs/xfs/scrub/health.c | 1
> > fs/xfs/scrub/scrub.c | 6 +
> > fs/xfs/scrub/scrub.h | 9 +
> > fs/xfs/scrub/trace.h | 63 ++++++++
> > 11 files changed, 455 insertions(+), 3 deletions(-)
> > create mode 100644 fs/xfs/scrub/fscounters.c
> >
> ...
> > diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
> > new file mode 100644
> > index 000000000000..aeb753dd0eaa
> > --- /dev/null
> > +++ b/fs/xfs/scrub/fscounters.c
> > @@ -0,0 +1,360 @@
> ...
> > +/*
> > + * Make sure the per-AG structure has been initialized from the on-disk header
> > + * contents and that the incore counters match the ondisk counters. Do this
> > + * from the setup function so that the inner summation loop runs as quickly as
> > + * possible.
> > + *
>
> I don't see this function matching incore counters against ondisk
> counters anywhere.
Oops, that was moved to the AG[FI] scrubbers. How about I change the
comment to:
/*
* Make sure the per-AG structure has been initialized from the on-disk
* header contents and trust that the incore counters match the ondisk
* counters. (The AGF and AGI scrubbers check them, and a normal
* xfs_scrub run checks the summary counters after checking all AG
* headers). Do this from the setup function so that the inner AG
* aggregation loop runs as quickly as possible.
*
* This function runs during the setup phase /before/ we start checking
* any metadata.
*/
?
>
> > + * This function runs during the setup phase /before/ we start checking any
> > + * metadata.
> > + */
> > +STATIC int
> > +xchk_fscount_warmup(
> > + struct xfs_scrub *sc)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + struct xfs_buf *agi_bp = NULL;
> > + struct xfs_buf *agf_bp = NULL;
> > + struct xfs_perag *pag = NULL;
> > + xfs_agnumber_t agno;
> > + int error = 0;
> > +
> > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > + pag = xfs_perag_get(mp, agno);
> > +
> > + if (pag->pagi_init && pag->pagf_init)
> > + goto next_loop_perag;
> > +
> > + /* Lock both AG headers. */
> > + error = xfs_ialloc_read_agi(mp, sc->tp, agno, &agi_bp);
> > + if (error)
> > + break;
> > + error = xfs_alloc_read_agf(mp, sc->tp, agno, 0, &agf_bp);
> > + if (error)
> > + break;
> > + error = -ENOMEM;
> > + if (!agf_bp || !agi_bp)
> > + break;
> > +
> > + /*
> > + * These are supposed to be initialized by the header read
> > + * function.
> > + */
> > + error = -EFSCORRUPTED;
> > + if (!pag->pagi_init || !pag->pagf_init)
> > + break;
> > +
> > + xfs_buf_relse(agf_bp);
> > + agf_bp = NULL;
> > + xfs_buf_relse(agi_bp);
> > + agi_bp = NULL;
> > +next_loop_perag:
> > + xfs_perag_put(pag);
> > + pag = NULL;
> > + error = 0;
> > +
> > + if (fatal_signal_pending(current))
> > + break;
> > + }
> > +
> > + if (agf_bp)
> > + xfs_buf_relse(agf_bp);
> > + if (agi_bp)
> > + xfs_buf_relse(agi_bp);
> > + if (pag)
> > + xfs_perag_put(pag);
> > + return error;
> > +}
> > +
> ...
> > +/*
> > + * Is the @counter reasonably close to the @expected value?
> > + *
> > + * We neither locked nor froze anything in the filesystem while aggregating the
> > + * per-AG data to compute the @expected value, which means that the counter
> > + * could have changed. We know the @old_value of the summation of the counter
> > + * before the aggregation, and we re-sum the counter now. If the expected
> > + * value falls between the two summations, we're ok.
> > + *
> > + * Otherwise, we /might/ have a problem. If the change in the summations is
> > + * more than we want to tolerate, the filesystem is probably busy and we should
> > + * just send back INCOMPLETE and see if userspace will try again.
> > + */
> > +static inline bool
> > +xchk_fscount_within_range(
> > + struct xfs_scrub *sc,
> > + const int64_t old_value,
> > + struct percpu_counter *counter,
> > + uint64_t expected)
> > +{
> > + int64_t min_value, max_value;
> > + int64_t curr_value = percpu_counter_sum(counter);
> > +
> > + trace_xchk_fscounters_within_range(sc->mp, expected, curr_value,
> > + old_value);
> > +
> > + /* Negative values are always wrong. */
> > + if (curr_value < 0)
> > + return false;
> > +
> > + /* Exact matches are always ok. */
> > + if (curr_value == expected)
> > + return true;
> > +
> > + min_value = min(old_value, curr_value);
> > + max_value = max(old_value, curr_value);
> > +
> > + /* Within the before-and-after range is ok. */
> > + if (expected >= min_value && expected <= max_value)
> > + return true;
> > +
>
> If the max/min variance is subject to restrictions, why do we allow the
> expected value to pass against those values before we check the variance
> below? Is the variance intended to only filter out false corruption
> reports?
For now, yes, the placement is deliberate to filter only false
corruption reports, because the only thing we can do for now is set the
OFLAG_INCOMPLETE. xfs_scrub reports that status but doesn't otherwise
do anything with that information...
> It seems a little strange to me to establish a variance rule
> like this where we don't trust them enough to indicate corruption, but
> would trust them enough to confirm lack of corruption (as opposed to
> telling the user "you might want to try again").
...because "INCOMPLETE" isn't specific enough to suggest to xfs_scrub
how it might improve the odds of a complete check when it tries again.
We ran the race on a busy system once and didn't win, so there's little
point in doing that all over again.
In a future online repair patchset I'll add the ability to freeze the
filesystem for certain fsck operations, which should solve the
variability problems; and add a new return value (EUSERS) that the
kernel will use to signal to xfs_scrub that it should try the scrub
again, but this time granting the kernel permission to freeze the fs
(IFLAG_FREEZE_OK). (Or xfs_scrub can decide that it doesn't care.)
At that point, I'll move the MIN_VARIANCE check above the min/max_value
check and have it return EUSERS so that userspace can be asked to call
back with freezing enabled. For now I aim only to avoid triggering
false corruption reports and warning about incomplete checks when scrub
can't really do much better.
Hmm, maybe that should be wrapped up in a comment...
/*
* If the difference between the two summations is too large, the fs
* might just be busy and so we'll mark the scrub incomplete. Return
* true here so that we don't mark the counter corrupt.
*
* XXX: In the future when userspace can grant scrub permission to
* quiesce the filesystem to solve the outsized variance problem, this
* check should be moved up and the return code changed to signal to
* userspace that we need quiesce permission.
*/
How about that?
--D
> Brian
>
> > + /*
> > + * If the difference between the two summations is too large, the fs
> > + * might just be busy and so we'll mark the scrub incomplete. Return
> > + * true here so that we don't mark the counter corrupt.
> > + */
> > + if (max_value - min_value >= XCHK_FSCOUNT_MIN_VARIANCE) {
> > + xchk_set_incomplete(sc);
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/* Check the superblock counters. */
> > +int
> > +xchk_fscounters(
> > + struct xfs_scrub *sc)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + struct xchk_fscounters *fsc = sc->buf;
> > + int64_t icount, ifree, fdblocks;
> > + int error;
> > +
> > + /* Snapshot the percpu counters. */
> > + icount = percpu_counter_sum(&mp->m_icount);
> > + ifree = percpu_counter_sum(&mp->m_ifree);
> > + fdblocks = percpu_counter_sum(&mp->m_fdblocks);
> > +
> > + /* No negative values, please! */
> > + if (icount < 0 || ifree < 0 || fdblocks < 0)
> > + xchk_set_corrupt(sc);
> > +
> > + /* See if icount is obviously wrong. */
> > + if (icount < fsc->icount_min || icount > fsc->icount_max)
> > + xchk_set_corrupt(sc);
> > +
> > + /* See if fdblocks is obviously wrong. */
> > + if (fdblocks > mp->m_sb.sb_dblocks)
> > + xchk_set_corrupt(sc);
> > +
> > + /*
> > + * If ifree exceeds icount by more than the minimum variance then
> > + * something's probably wrong with the counters.
> > + */
> > + if (ifree > icount && ifree - icount > XCHK_FSCOUNT_MIN_VARIANCE)
> > + xchk_set_corrupt(sc);
> > +
> > + /* Walk the incore AG headers to calculate the expected counters. */
> > + error = xchk_fscount_aggregate_agcounts(sc, fsc);
> > + if (!xchk_process_error(sc, 0, XFS_SB_BLOCK(mp), &error))
> > + return error;
> > + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_INCOMPLETE)
> > + return 0;
> > +
> > + /* Compare the in-core counters with whatever we counted. */
> > + if (!xchk_fscount_within_range(sc, icount, &mp->m_icount, fsc->icount))
> > + xchk_set_corrupt(sc);
> > +
> > + if (!xchk_fscount_within_range(sc, ifree, &mp->m_ifree, fsc->ifree))
> > + xchk_set_corrupt(sc);
> > +
> > + if (!xchk_fscount_within_range(sc, fdblocks, &mp->m_fdblocks,
> > + fsc->fdblocks))
> > + xchk_set_corrupt(sc);
> > +
> > + return 0;
> > +}
> > diff --git a/fs/xfs/scrub/health.c b/fs/xfs/scrub/health.c
> > index 16b536aa125e..23cf8e2f25db 100644
> > --- a/fs/xfs/scrub/health.c
> > +++ b/fs/xfs/scrub/health.c
> > @@ -109,6 +109,7 @@ static const struct xchk_health_map type_to_health_flag[XFS_SCRUB_TYPE_NR] = {
> > [XFS_SCRUB_TYPE_UQUOTA] = { XHG_FS, XFS_SICK_FS_UQUOTA },
> > [XFS_SCRUB_TYPE_GQUOTA] = { XHG_FS, XFS_SICK_FS_GQUOTA },
> > [XFS_SCRUB_TYPE_PQUOTA] = { XHG_FS, XFS_SICK_FS_PQUOTA },
> > + [XFS_SCRUB_TYPE_FSCOUNTERS] = { XHG_FS, XFS_SICK_FS_COUNTERS },
> > };
> >
> > /* Return the health status mask for this scrub type. */
> > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > index ce13c1c366db..f630389ee176 100644
> > --- a/fs/xfs/scrub/scrub.c
> > +++ b/fs/xfs/scrub/scrub.c
> > @@ -352,6 +352,12 @@ static const struct xchk_meta_ops meta_scrub_ops[] = {
> > .scrub = xchk_quota,
> > .repair = xrep_notsupported,
> > },
> > + [XFS_SCRUB_TYPE_FSCOUNTERS] = { /* fs summary counters */
> > + .type = ST_FS,
> > + .setup = xchk_setup_fscounters,
> > + .scrub = xchk_fscounters,
> > + .repair = xrep_notsupported,
> > + },
> > };
> >
> > /* This isn't a stable feature, warn once per day. */
> > diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> > index 01986ed364db..ad1ceb44a628 100644
> > --- a/fs/xfs/scrub/scrub.h
> > +++ b/fs/xfs/scrub/scrub.h
> > @@ -127,6 +127,7 @@ xchk_quota(struct xfs_scrub *sc)
> > return -ENOENT;
> > }
> > #endif
> > +int xchk_fscounters(struct xfs_scrub *sc);
> >
> > /* cross-referencing helpers */
> > void xchk_xref_is_used_space(struct xfs_scrub *sc, xfs_agblock_t agbno,
> > @@ -152,4 +153,12 @@ void xchk_xref_is_used_rt_space(struct xfs_scrub *sc, xfs_rtblock_t rtbno,
> > # define xchk_xref_is_used_rt_space(sc, rtbno, len) do { } while (0)
> > #endif
> >
> > +struct xchk_fscounters {
> > + uint64_t icount;
> > + uint64_t ifree;
> > + uint64_t fdblocks;
> > + unsigned long long icount_min;
> > + unsigned long long icount_max;
> > +};
> > +
> > #endif /* __XFS_SCRUB_SCRUB_H__ */
> > diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> > index 3c83e8b3b39c..3362bae28b46 100644
> > --- a/fs/xfs/scrub/trace.h
> > +++ b/fs/xfs/scrub/trace.h
> > @@ -50,6 +50,7 @@ TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_RTSUM);
> > TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_UQUOTA);
> > TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_GQUOTA);
> > TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_PQUOTA);
> > +TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_FSCOUNTERS);
> >
> > #define XFS_SCRUB_TYPE_STRINGS \
> > { XFS_SCRUB_TYPE_PROBE, "probe" }, \
> > @@ -75,7 +76,8 @@ TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_PQUOTA);
> > { XFS_SCRUB_TYPE_RTSUM, "rtsummary" }, \
> > { XFS_SCRUB_TYPE_UQUOTA, "usrquota" }, \
> > { XFS_SCRUB_TYPE_GQUOTA, "grpquota" }, \
> > - { XFS_SCRUB_TYPE_PQUOTA, "prjquota" }
> > + { XFS_SCRUB_TYPE_PQUOTA, "prjquota" }, \
> > + { XFS_SCRUB_TYPE_FSCOUNTERS, "fscounters" }
> >
> > DECLARE_EVENT_CLASS(xchk_class,
> > TP_PROTO(struct xfs_inode *ip, struct xfs_scrub_metadata *sm,
> > @@ -223,6 +225,7 @@ DEFINE_EVENT(xchk_block_error_class, name, \
> > void *ret_ip), \
> > TP_ARGS(sc, daddr, ret_ip))
> >
> > +DEFINE_SCRUB_BLOCK_ERROR_EVENT(xchk_fs_error);
> > DEFINE_SCRUB_BLOCK_ERROR_EVENT(xchk_block_error);
> > DEFINE_SCRUB_BLOCK_ERROR_EVENT(xchk_block_preen);
> >
> > @@ -590,6 +593,64 @@ TRACE_EVENT(xchk_iallocbt_check_cluster,
> > __entry->cluster_ino)
> > )
> >
> > +TRACE_EVENT(xchk_fscounters_calc,
> > + TP_PROTO(struct xfs_mount *mp, uint64_t icount, uint64_t ifree,
> > + uint64_t fdblocks, uint64_t delalloc),
> > + TP_ARGS(mp, icount, ifree, fdblocks, delalloc),
> > + TP_STRUCT__entry(
> > + __field(dev_t, dev)
> > + __field(int64_t, icount_sb)
> > + __field(uint64_t, icount_calculated)
> > + __field(int64_t, ifree_sb)
> > + __field(uint64_t, ifree_calculated)
> > + __field(int64_t, fdblocks_sb)
> > + __field(uint64_t, fdblocks_calculated)
> > + __field(uint64_t, delalloc)
> > + ),
> > + TP_fast_assign(
> > + __entry->dev = mp->m_super->s_dev;
> > + __entry->icount_sb = mp->m_sb.sb_icount;
> > + __entry->icount_calculated = icount;
> > + __entry->ifree_sb = mp->m_sb.sb_ifree;
> > + __entry->ifree_calculated = ifree;
> > + __entry->fdblocks_sb = mp->m_sb.sb_fdblocks;
> > + __entry->fdblocks_calculated = fdblocks;
> > + __entry->delalloc = delalloc;
> > + ),
> > + TP_printk("dev %d:%d icount %lld:%llu ifree %lld::%llu fdblocks %lld::%llu delalloc %llu",
> > + MAJOR(__entry->dev), MINOR(__entry->dev),
> > + __entry->icount_sb,
> > + __entry->icount_calculated,
> > + __entry->ifree_sb,
> > + __entry->ifree_calculated,
> > + __entry->fdblocks_sb,
> > + __entry->fdblocks_calculated,
> > + __entry->delalloc)
> > +)
> > +
> > +TRACE_EVENT(xchk_fscounters_within_range,
> > + TP_PROTO(struct xfs_mount *mp, uint64_t expected, int64_t curr_value,
> > + int64_t old_value),
> > + TP_ARGS(mp, expected, curr_value, old_value),
> > + TP_STRUCT__entry(
> > + __field(dev_t, dev)
> > + __field(uint64_t, expected)
> > + __field(int64_t, curr_value)
> > + __field(int64_t, old_value)
> > + ),
> > + TP_fast_assign(
> > + __entry->dev = mp->m_super->s_dev;
> > + __entry->expected = expected;
> > + __entry->curr_value = curr_value;
> > + __entry->old_value = old_value;
> > + ),
> > + TP_printk("dev %d:%d expected %llu curr_value %lld old_value %lld",
> > + MAJOR(__entry->dev), MINOR(__entry->dev),
> > + __entry->expected,
> > + __entry->curr_value,
> > + __entry->old_value)
> > +)
> > +
> > /* repair tracepoints */
> > #if IS_ENABLED(CONFIG_XFS_ONLINE_REPAIR)
> >
