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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
