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

[Dave Chinner <david@fromorbit.com>]

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