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

[Brian Foster <bfoster@redhat.com>]

On Fri, Dec 11, 2020 at 02:58:51PM +0530, Chandan Babu R wrote:
> On Thu, 10 Dec 2020 09:46:07 -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.
> >
> 
> One change that the patch causes is that xfs_log_unmount_write() is now
> invoked while xfs_buf cache is still populated (though none of the xfs_bufs
> would be undergoing I/O). However, I don't see this causing any erroneous
> behaviour. Hence,
> 

Yeah.. that is intentional. The buffer LRU drain/reclaim is basically
now more of a memory cleanup operation as opposed to part of the log and
I/O quiescing process, the latter of which is required to complete
before we write out the unmount record.

> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
> 

Thanks for the review.

Brian

> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  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);
> > 
> 
> 
> -- 
> chandan
> 
> 
>