Re: [PATCH 3/4] xfs: force log and push AIL to clear pinned inodes when aborting mount

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Mar 08, 2021 at 10:01:23AM +1100, Dave Chinner wrote:
> On Sun, Mar 07, 2021 at 12:25:57PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If we allocate quota inodes in the process of mounting a filesystem but
> > then decide to abort the mount, it's possible that the quota inodes are
> > sitting around pinned by the log.  Now that inode reclaim relies on the
> > AIL to flush inodes, we have to force the log and push the AIL in
> > between releasing the quota inodes and kicking off reclaim to tear down
> > all the incore inodes.  Do this by extracting the bits we need from the
> > unmount path and reusing them.
> > 
> > This was originally found during a fuzz test of metadata directories
> > (xfs/1546), but the actual symptom was that reclaim hung up on the quota
> > inodes.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_mount.c |  100 ++++++++++++++++++++++++++++------------------------
> >  1 file changed, 54 insertions(+), 46 deletions(-)
> 
> Seems reasonable.
> 
> > 
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 52370d0a3f43..556ce373145f 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -634,6 +634,57 @@ xfs_check_summary_counts(
> >  	return xfs_initialize_perag_data(mp, mp->m_sb.sb_agcount);
> >  }
> >  
> > +/*
> > + * Force the log contents and checkpoint them into the filesystem, the reclaim
> > + * inodes in preparation to unmount.
> 
> "then reclaim"
> 
> Ignoring the typo, the comment doesn't add anything useful - you're
> saying what the function does, not why. I'd prefer you lift all the
> comments in the code up into the header, explaining why each step
> is needed/taken. Something like:
> 
> /*
>  * Flush and reclaim dirty inodes in preparation for unmount. Inodes and
>  * internal inode structures can be sitting in the CIL and AIL at this point, so
>  * we need to unpin them, write them back and/or reclaim them before unmount can
>  * proceed.
>  *
>  * An inode cluster that has been freed can have its buffer still pinned in
>  * memory because the transaction is still sitting in a iclog. The stale inodes
>  * on that buffer will be pinned to the buffer until the transaction hits the
>  * disk and the callbacks run. Pushing the AIL will skip the stale inodes and
>  * may never see the pinned buffer, so nothing will push out the iclog and unpin
>  * the buffer.
>  *
>  * Hence we need to force the log to unpin everything first. However, log forces
>  * don't wait for the discards they issue to complete, so we have to explicitly
>  * wait for them to complete here as well.
>  *
>  * Then we can tell the world we are unmounting so that error handling knows
>  * that the filesystem is going away and we should error out anything that we
>  * have been retrying in the background.  This will prevent never-ending retries
>  * in AIL pushing from hanging the unmount.
>  *
>  * Finally, we can push the AIL to clean all the remaining dirty objects, then
>  * reclaim the remaining inodes that are still in memory at this point in
>  * time.
>  */

Ok.  Seems good to me.

--D

> static void
> xfs_unmount_flush_inodes(
> 	struct xfs_mount	*mp)
> {
> 	xfs_log_force(mp, XFS_LOG_SYNC);
> 	xfs_extent_busy_wait_all(mp);
> 	flush_workqueue(xfs_discard_wq);
> 
> 	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
> 
> 	xfs_ail_push_all_sync(mp->m_ail);
> 	cancel_delayed_work_sync(&mp->m_reclaim_work);
> 	xfs_reclaim_inodes(mp);
> 	xfs_health_unmount(mp);
> }
> 
> Everything else looks fine.
> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com