Re: [PATCH 3/4] xfs: drain the buffer LRU on mount

[Brian Foster <bfoster@redhat.com>]

On Mon, Oct 23, 2017 at 09:39:40AM -0700, Darrick J. Wong wrote:
> On Mon, Oct 23, 2017 at 10:46:45AM -0400, Brian Foster wrote:
> > Log recovery of v4 filesystems does not use buffer verifiers because
> > log recovery historically can result in transient buffer corruption
> > when target buffers might be ahead of the log after a crash. v5
> > filesystems work around this problem with metadata LSN ordering.
> > 
> > While the log recovery behavior is necessary on v4 supers, it
> > currently can result in leaving buffers around in the LRU without
> > verifiers attached for a significant amount of time. This can lead
> > to use of unverified buffers while the filesystem is in active use,
> > long after recovery has completed.
> > 
> > To address this problem and provide a more consistent clean,
> > post-mount buffer cache state, update the log mount sequence to
> > unconditionally drain all buffers from the LRU as a final step.
> > 
> > Reported-by: Darrick Wong <darrick.wong@oracle.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_log.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index dc95a49..e282fd8 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -744,6 +744,7 @@ xfs_log_mount_finish(
> >  {
> >  	int	error = 0;
> >  	bool	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);
> > +	bool	recovered = mp->m_log->l_flags & XLOG_RECOVERY_NEEDED;
> >  
> >  	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
> >  		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
> > @@ -780,6 +781,18 @@ xfs_log_mount_finish(
> >  	mp->m_super->s_flags &= ~MS_ACTIVE;
> >  	evict_inodes(mp->m_super);
> >  
> > +	/*
> > +	 * Drain the buffer LRU after log recovery. This is required for v4
> > +	 * filesystems to avoid leaving around buffers with NULL verifier ops,
> > +	 * but we do it unconditionally to make sure we're always in a clean
> > +	 * cache state after mount.
> > +	 */
> > +	if (recovered) {
> 
> if (recovered && !error) { ?
> 
> I observed that running xfs/376 on an rmap filesystem fails when it
> tries to fuzz the high bit of u3.bmx[0].startoff.  That triggers an
> incorrect freeing of what is now a post-eof extent.  The corresponding
> rmap free operation fails after the RUI has been logged and shuts down
> the filesystem, so a subsequent log recovery attempt also fails when it
> tries to remove an rmap that doesn't exist.  If we then try to force the
> log we end up deadlocked somehwere... though if we /don't/ then memory
> gets corrupted and the kernel blows up anyway. :(
> 

Interesting... do you have a stack trace? I'm curious why forcing the
log would hang here and not in the subsequent log force in
xfs_log_unmount() -> xfs_log_quiesce().

Brian

> --D
> 
> > +		xfs_log_force(mp, XFS_LOG_SYNC);
> > +		xfs_ail_push_all_sync(mp->m_ail);
> > +	}
> > +	xfs_wait_buftarg(mp->m_ddev_targp);
> > +
> >  	if (readonly)
> >  		mp->m_flags |= XFS_MOUNT_RDONLY;
> >  
> > -- 
> > 2.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html