On Wed, May 20, 2020 at 08:23:10AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> It's a global atomic counter, and we are hitting it at a rate of
> half a million transactions a second, so it's bouncing the counter
> cacheline all over the place on large machines. We don't actually
> need it anymore - it used to be required because the VFS freeze code
> could not track/prevent filesystem transactions that were running,
> but that problem no longer exists.
>
> Hence to remove the counter, we simply have to ensure that nothing
> calls xfs_sync_sb() while we are trying to quiesce the filesytem.
> That only happens if the log worker is still running when we call
> xfs_quiesce_attr(). The log worker is cancelled at the end of
> xfs_quiesce_attr() by calling xfs_log_quiesce(), so just call it
> early here and then we can remove the counter altogether.
>
> Concurrent create, 50 million inodes, identical 16p/16GB virtual
> machines on different physical hosts. Machine A has twice the CPU
> cores per socket of machine B:
>
> unpatched patched
> machine A: 3m16s 2m00s
> machine B: 4m04s 4m05s
>
> Create rates:
> unpatched patched
> machine A: 282k+/-31k 468k+/-21k
> machine B: 231k+/-8k 233k+/-11k
>
> Concurrent rm of same 50 million inodes:
>
> unpatched patched
> machine A: 6m42s 2m33s
> machine B: 4m47s 4m47s
>
> The transaction rate on the fast machine went from just under
> 300k/sec to 700k/sec, which indicates just how much of a bottleneck
> this atomic counter was.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_mount.h | 1 -
> fs/xfs/xfs_super.c | 17 +++++------------
> fs/xfs/xfs_trans.c | 27 +++++++++++----------------
> 3 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index c1f92c1847bb2..3725d25ad97e8 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -176,7 +176,6 @@ typedef struct xfs_mount {
> uint64_t m_resblks; /* total reserved blocks */
> uint64_t m_resblks_avail;/* available reserved blocks */
> uint64_t m_resblks_save; /* reserved blks @ remount,ro */
> - atomic_t m_active_trans; /* number trans frozen */
> struct delayed_work m_reclaim_work; /* background inode reclaim */
> struct delayed_work m_eofblocks_work; /* background eof blocks
> trimming */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index aae469f73efeb..fa58cb07c8fdf 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -874,8 +874,10 @@ xfs_restore_resvblks(struct xfs_mount *mp)
> * there is no log replay required to write the inodes to disk - this is the
> * primary difference between a sync and a quiesce.
> *
> + * We cancel log work early here to ensure all transactions the log worker may
> + * run have finished before we clean up and log the superblock and write an
> + * unmount record. The unfreeze process is responsible for restarting the log
> + * worker correctly.
> */
> void
> xfs_quiesce_attr(
> @@ -883,9 +885,7 @@ xfs_quiesce_attr(
> {
> int error = 0;
>
> - /* wait for all modifications to complete */
> - while (atomic_read(&mp->m_active_trans) > 0)
> - delay(100);
> + cancel_delayed_work_sync(&mp->m_log->l_work);
Shouldn't the cancel_delayed_work_sync for l_work in xfs_log_quiesce
be removed now given that we've already cancelled it here?
