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.
*/
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
