Re: [PATCH 2/2] xfs: don't drain buffer lru on freeze and read-only remount

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

On Thu, Dec 10, 2020 at 09:46:07AM -0500, Brian Foster wrote:
> xfs_buftarg_drain() is called from xfs_log_quiesce() to ensure the
> buffer cache is reclaimed during unmount. xfs_log_quiesce() is also
> called from xfs_quiesce_attr(), however, which means that cache
> state is completely drained for filesystem freeze and read-only
> remount. While technically harmless, this is unnecessarily
> heavyweight. Both freeze and read-only mounts allow reads and thus
> allow population of the buffer cache. Therefore, the transitional
> sequence in either case really only needs to quiesce outstanding
> writes to return the filesystem in a generally read-only state.
> 
> Additionally, some users have reported that attempts to freeze a
> filesystem concurrent with a read-heavy workload causes the freeze
> process to stall for a significant amount of time. This occurs
> because, as mentioned above, the read workload repopulates the
> buffer LRU while the freeze task attempts to drain it.
> 
> To improve this situation, replace the drain in xfs_log_quiesce()
> with a buffer I/O quiesce and lift the drain into the unmount path.
> This removes buffer LRU reclaim from freeze and read-only [re]mount,
> but ensures the LRU is still drained before the filesystem unmounts.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 20 +++++++++++++++-----
>  fs/xfs/xfs_buf.h |  1 +
>  fs/xfs/xfs_log.c |  6 ++++--
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index db918ed20c40..d3fce3129f6e 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1815,14 +1815,13 @@ xfs_buftarg_drain_rele(
>  	return LRU_REMOVED;
>  }
>  
> +/*
> + * Wait for outstanding I/O on the buftarg to complete.
> + */
>  void
> -xfs_buftarg_drain(
> +xfs_buftarg_wait(
>  	struct xfs_buftarg	*btp)
>  {
> -	LIST_HEAD(dispose);
> -	int			loop = 0;
> -	bool			write_fail = false;
> -
>  	/*
>  	 * First wait on the buftarg I/O count for all in-flight buffers to be
>  	 * released. This is critical as new buffers do not make the LRU until
> @@ -1838,6 +1837,17 @@ xfs_buftarg_drain(
>  	while (percpu_counter_sum(&btp->bt_io_count))
>  		delay(100);
>  	flush_workqueue(btp->bt_mount->m_buf_workqueue);
> +}
> +
> +void
> +xfs_buftarg_drain(
> +	struct xfs_buftarg	*btp)
> +{
> +	LIST_HEAD(dispose);
> +	int			loop = 0;
> +	bool			write_fail = false;
> +
> +	xfs_buftarg_wait(btp);
>  
>  	/* loop until there is nothing left on the lru list. */
>  	while (list_lru_count(&btp->bt_lru)) {
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index ea32369f8f77..96c6b478e26e 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -347,6 +347,7 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  extern struct xfs_buftarg *xfs_alloc_buftarg(struct xfs_mount *,
>  		struct block_device *, struct dax_device *);
>  extern void xfs_free_buftarg(struct xfs_buftarg *);
> +extern void xfs_buftarg_wait(struct xfs_buftarg *);
>  extern void xfs_buftarg_drain(struct xfs_buftarg *);
>  extern int xfs_setsize_buftarg(struct xfs_buftarg *, unsigned int);
>  
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5ad4d5e78019..46ea4017fcec 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -936,13 +936,13 @@ xfs_log_quiesce(
>  
>  	/*
>  	 * The superblock buffer is uncached and while xfs_ail_push_all_sync()
> -	 * will push it, xfs_buftarg_drain() will not wait for it. Further,
> +	 * will push it, xfs_buftarg_wait() will not wait for it. Further,
>  	 * xfs_buf_iowait() cannot be used because it was pushed with the
>  	 * XBF_ASYNC flag set, so we need to use a lock/unlock pair to wait for
>  	 * the IO to complete.
>  	 */
>  	xfs_ail_push_all_sync(mp->m_ail);
> -	xfs_buftarg_drain(mp->m_ddev_targp);
> +	xfs_buftarg_wait(mp->m_ddev_targp);
>  	xfs_buf_lock(mp->m_sb_bp);
>  	xfs_buf_unlock(mp->m_sb_bp);
>  
> @@ -962,6 +962,8 @@ xfs_log_unmount(
>  {
>  	xfs_log_quiesce(mp);
>  
> +	xfs_buftarg_drain(mp->m_ddev_targp);
> +
>  	xfs_trans_ail_destroy(mp);
>  
>  	xfs_sysfs_del(&mp->m_log->l_kobj);
> -- 
> 2.26.2
>