On Tue, Apr 02, 2019 at 09:53:41AM -0400, Brian Foster wrote:
> On Tue, Apr 02, 2019 at 06:40:10AM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 02, 2019 at 09:24:45AM -0400, Brian Foster wrote:
> > > On Mon, Apr 01, 2019 at 10:10:28AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > >
> > > > If we know the filesystem metadata isn't healthy during unmount, we want
> > > > to encourage the administrator to run xfs_repair right away. We can't
> > > > do this if BAD_SUMMARY will cause an unclean log unmount to force
> > > > summary recalculation, so turn it off if the fs is bad.
> > > >
> > >
> > > Do you mean we don't want to suggest xfs_repair because we intentionally
> > > cause a dirty log and thus xfs_repair will require to zap it? If so, the
> > > wording above and the comment in xfs_health_unmount() could be a bit
> > > more specific on the reasoning.
> >
> > Sort of the opposite? We want to suggest xfs_repair, but we don't want
> > to leave the log dirty because that adds the additional step of running
> > xfs_repair -L to zap the log. I get the sense that we don't really want
> > to encourage admins to be cavalier about running that...?
> >
>
> Ok.. I guess what sounded funny to me is the suggestion that "we can't"
> leave a dirty log and suggest repair to the user. xfs_repair can clearly
> handle a dirty log, so it suggested to me that perhaps there was some
> other critical side effect I wasn't thinking of that we wanted to avoid
> as a result of needing to zap the log.
>
> To be clear, I don't feel strongly about it and still think the change
> is reasonable enough (it only matters when something else is broken in
> the fs after all). I just wanted to clarify the above, call out the
> potential tradeoff in the event that others might have further thoughts
> on it, and suggest the full reasoning eventually make it into the commit
> log for future reference.
Ahh, I see the confusion here. What if I reworked the comment:
/*
* We discovered uncorrected metadata problems at some point during this
* filesystem mount and have advised the administrator to run repair
* once the unmount completes.
*
* However, we must be careful -- when FSCOUNTERS are flagged unhealthy,
* the unmount procedure omits writing the clean unmount record to the
* log so that the next mount will run recovery and recompute the
* summary counters. In other words, we leave a dirty log to get the
* counters fixed.
*
* Unfortunately, xfs_repair cannot recover dirty logs, so if there were
* filesystem problems, FSCOUNTERS was flagged, and the administrator
* takes our advice to run xfs_repair, they'll have to zap the log
* before repairing structures. We don't really want to encourage this,
* so we mark the FSCOUNTERS healthy so that a subsequent repair run
* won't see a dirty log.
*/
Also the "if (sick)" check needs to mask off FSCOUNTERS; I'll fix that
too.
> > (Would be nice if we could just port log recovery to repair, but that's
> > a whole separate project...)
> >
>
> Or work around the whole "trigger summary recalc via log recovery"
> thing, but I guess that might not be trivial either..
Well I do have a prototype fscounters scrubber and repairer, so in the
future we might be able to avoid this dirty log dance.
> > > Also, what exactly is the side effect without this change in place? The
> > > user would have to zap the log from xfs_repair, but the somewhat
> > > artificial unclean unmount doesn't actually require log recovery to fix
> > > up the fs outside of the whole summary counter thing, right? IOW, would
> > > the user zapping the log actually lose anything besides the bad summary
> > > counter indication?
> >
> > The log checkpoints at unmount, right? So I think it's ok to zap the
> > log when its status is "cleanly unmounted but we didn't record the clean
> > unmount because we want to force summary recalculation at next mount".
> >
>
> That's what I would expect, but I haven't tested it. :)
Technically, I have, what with this obnoxious "hard shutdowns are a
normal part of our workflow" user case I've been wrangling with... the
clean umounts /do/ seem to leave a clean log.
--D
> Brian
>
> > > I ask just because even though we warn the user to
> > > run repair, that doesn't mean they'll actually do it and so it seems
> > > there is a bit of a tradeoff in that regard.
> >
> > Yeah, it's a pity we can't just run xfs_repair ourselves. :)
> >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > > ---
> > >
> > > BTW, I get the following compiler warning on this patch:
> > >
> > > In file included from fs/xfs/xfs_trace.h:12,
> > > from fs/xfs/xfs_health.c:19:
> > > fs/xfs/xfs_health.c: In function ‘xfs_health_unmount’:
> > > ./include/linux/tracepoint.h:195:6: warning: ‘sick’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > ((void(*)(proto))(it_func))(args); \
> > > ^
> > > fs/xfs/xfs_health.c:33:16: note: ‘sick’ was declared here
> > > unsigned int sick;
> >
> > <nod> Thanks, will fix that for v2.
> >
> > --D
> >
> > > Brian
> > >
> > > > fs/xfs/libxfs/xfs_health.h | 2 +
> > > > fs/xfs/xfs_health.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> > > > fs/xfs/xfs_mount.c | 2 +
> > > > fs/xfs/xfs_trace.h | 3 ++
> > > > 4 files changed, 66 insertions(+)
> > > >
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_health.h b/fs/xfs/libxfs/xfs_health.h
> > > > index 0d51bd2689ea..269b124dc1d7 100644
> > > > --- a/fs/xfs/libxfs/xfs_health.h
> > > > +++ b/fs/xfs/libxfs/xfs_health.h
> > > > @@ -148,6 +148,8 @@ void xfs_inode_mark_sick(struct xfs_inode *ip, unsigned int mask);
> > > > void xfs_inode_mark_healthy(struct xfs_inode *ip, unsigned int mask);
> > > > unsigned int xfs_inode_measure_sickness(struct xfs_inode *ip);
> > > >
> > > > +void xfs_health_unmount(struct xfs_mount *mp);
> > > > +
> > > > /* Now some helpers. */
> > > >
> > > > static inline bool
> > > > diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> > > > index e9d6859f7501..6e2da858c356 100644
> > > > --- a/fs/xfs/xfs_health.c
> > > > +++ b/fs/xfs/xfs_health.c
> > > > @@ -19,6 +19,65 @@
> > > > #include "xfs_trace.h"
> > > > #include "xfs_health.h"
> > > >
> > > > +/*
> > > > + * Warn about metadata corruption that we detected but haven't fixed, and
> > > > + * make sure we're not sitting on anything that would get in the way of
> > > > + * recovery.
> > > > + */
> > > > +void
> > > > +xfs_health_unmount(
> > > > + struct xfs_mount *mp)
> > > > +{
> > > > + struct xfs_perag *pag;
> > > > + xfs_agnumber_t agno;
> > > > + unsigned int sick;
> > > > + bool warn = false;
> > > > +
> > > > + if (XFS_FORCED_SHUTDOWN(mp))
> > > > + return;
> > > > +
> > > > + /* Measure AG corruption levels. */
> > > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > > > + pag = xfs_perag_get(mp, agno);
> > > > + spin_lock(&pag->pag_state_lock);
> > > > + if (pag->pag_sick) {
> > > > + trace_xfs_ag_unfixed_corruption(mp, agno, sick);
> > > > + warn = true;
> > > > + }
> > > > + spin_unlock(&pag->pag_state_lock);
> > > > + xfs_perag_put(pag);
> > > > + }
> > > > +
> > > > + /* Measure realtime volume corruption levels. */
> > > > + sick = xfs_rt_measure_sickness(mp);
> > > > + if (sick) {
> > > > + trace_xfs_rt_unfixed_corruption(mp, sick);
> > > > + warn = true;
> > > > + }
> > > > +
> > > > + /* Measure fs corruption and keep the sample around for the warning. */
> > > > + sick = xfs_fs_measure_sickness(mp);
> > > > + if (sick) {
> > > > + trace_xfs_fs_unfixed_corruption(mp, sick);
> > > > + warn = true;
> > > > + }
> > > > +
> > > > + if (warn) {
> > > > + xfs_warn(mp,
> > > > +"Uncorrected metadata errors detected; please run xfs_repair.");
> > > > +
> > > > + /*
> > > > + * If we have unhealthy metadata, we want the admin to run
> > > > + * xfs_repair after unmounting. They can't do that if the log
> > > > + * is written out without a clean unmount record (such as when
> > > > + * the summary counters are marked unhealthy to force
> > > > + * recalculation of the summary counters) so clear it.
> > > > + */
> > > > + if (sick & XFS_HEALTH_FS_COUNTERS)
> > > > + xfs_fs_mark_healthy(mp, XFS_HEALTH_FS_COUNTERS);
> > > > + }
> > > > +}
> > > > +
> > > > /* Mark unhealthy per-fs metadata. */
> > > > void
> > > > xfs_fs_mark_sick(
> > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > > index a43ca655a431..f0f73d598a0c 100644
> > > > --- a/fs/xfs/xfs_mount.c
> > > > +++ b/fs/xfs/xfs_mount.c
> > > > @@ -1075,6 +1075,7 @@ xfs_mountfs(
> > > > */
> > > > cancel_delayed_work_sync(&mp->m_reclaim_work);
> > > > xfs_reclaim_inodes(mp, SYNC_WAIT);
> > > > + xfs_health_unmount(mp);
> > > > out_log_dealloc:
> > > > mp->m_flags |= XFS_MOUNT_UNMOUNTING;
> > > > xfs_log_mount_cancel(mp);
> > > > @@ -1157,6 +1158,7 @@ xfs_unmountfs(
> > > > */
> > > > cancel_delayed_work_sync(&mp->m_reclaim_work);
> > > > xfs_reclaim_inodes(mp, SYNC_WAIT);
> > > > + xfs_health_unmount(mp);
> > > >
> > > > xfs_qm_unmount(mp);
> > > >
> > > > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > > > index f079841c7af6..2464ea351f83 100644
> > > > --- a/fs/xfs/xfs_trace.h
> > > > +++ b/fs/xfs/xfs_trace.h
> > > > @@ -3461,8 +3461,10 @@ DEFINE_EVENT(xfs_fs_corrupt_class, name, \
> > > > TP_ARGS(mp, flags))
> > > > DEFINE_FS_CORRUPT_EVENT(xfs_fs_mark_sick);
> > > > DEFINE_FS_CORRUPT_EVENT(xfs_fs_mark_healthy);
> > > > +DEFINE_FS_CORRUPT_EVENT(xfs_fs_unfixed_corruption);
> > > > DEFINE_FS_CORRUPT_EVENT(xfs_rt_mark_sick);
> > > > DEFINE_FS_CORRUPT_EVENT(xfs_rt_mark_healthy);
> > > > +DEFINE_FS_CORRUPT_EVENT(xfs_rt_unfixed_corruption);
> > > >
> > > > DECLARE_EVENT_CLASS(xfs_ag_corrupt_class,
> > > > TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, unsigned int flags),
> > > > @@ -3488,6 +3490,7 @@ DEFINE_EVENT(xfs_ag_corrupt_class, name, \
> > > > TP_ARGS(mp, agno, flags))
> > > > DEFINE_AG_CORRUPT_EVENT(xfs_ag_mark_sick);
> > > > DEFINE_AG_CORRUPT_EVENT(xfs_ag_mark_healthy);
> > > > +DEFINE_AG_CORRUPT_EVENT(xfs_ag_unfixed_corruption);
> > > >
> > > > DECLARE_EVENT_CLASS(xfs_inode_corrupt_class,
> > > > TP_PROTO(struct xfs_inode *ip, unsigned int flags),
> > > >
