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@xxxxxxxxx>
>
Thanks for the review.
Brian
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > 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
>
>
>
