[PATCH 0/2] xfs: don't drain buffer lru on freeze

[PATCH 0/2] xfs: don't drain buffer lru on freeze

[Brian Foster]

Hi all,

This series tweaks the xfs_log_quiesce() codepath to lift out the
explicit buffer target LRU draining and isolate it to the unmount path.
It's unnecessary to reclaim all buffers on filesystem freeze or
read-only remount, and this also causes such operations to stall if a
read heavy workload is running in parallel.

Patch 1 is a simple rename and patch 2 implements the functional change.
Thoughts, reviews, flames appreciated.

Brian

Brian Foster (2):
  xfs: rename xfs_wait_buftarg() to xfs_buftarg_drain()
  xfs: don't drain buffer lru on freeze and read-only remount

 fs/xfs/xfs_buf.c   | 30 ++++++++++++++++++++----------
 fs/xfs/xfs_buf.h   | 11 ++++++-----
 fs/xfs/xfs_log.c   |  8 +++++---
 fs/xfs/xfs_mount.c |  4 ++--
 fs/xfs/xfs_trace.h |  2 +-
 5 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.26.2

[PATCH 1/2] xfs: rename xfs_wait_buftarg() to xfs_buftarg_drain()

[Brian Foster]

xfs_wait_buftarg() is vaguely named and somewhat overloaded. Its
primary purpose is to reclaim all buffers from the provided buffer
target LRU. In preparation to refactor xfs_wait_buftarg() into
serialization and LRU draining components, rename the function and
associated helpers to something more descriptive. This patch has no
functional changes with the minor exception of renaming a
tracepoint.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_buf.c   | 12 ++++++------
 fs/xfs/xfs_buf.h   | 10 +++++-----
 fs/xfs/xfs_log.c   |  6 +++---
 fs/xfs/xfs_mount.c |  4 ++--
 fs/xfs/xfs_trace.h |  2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4e4cf91f4f9f..db918ed20c40 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -43,7 +43,7 @@ static kmem_zone_t *xfs_buf_zone;
  *	  pag_buf_lock
  *	    lru_lock
  *
- * xfs_buftarg_wait_rele
+ * xfs_buftarg_drain_rele
  *	lru_lock
  *	  b_lock (trylock due to inversion)
  *
@@ -88,7 +88,7 @@ xfs_buf_vmap_len(
  * because the corresponding decrement is deferred to buffer release. Buffers
  * can undergo I/O multiple times in a hold-release cycle and per buffer I/O
  * tracking adds unnecessary overhead. This is used for sychronization purposes
- * with unmount (see xfs_wait_buftarg()), so all we really need is a count of
+ * with unmount (see xfs_buftarg_drain()), so all we really need is a count of
  * in-flight buffers.
  *
  * Buffers that are never released (e.g., superblock, iclog buffers) must set
@@ -1786,7 +1786,7 @@ __xfs_buf_mark_corrupt(
  * while freeing all the buffers only held by the LRU.
  */
 static enum lru_status
-xfs_buftarg_wait_rele(
+xfs_buftarg_drain_rele(
 	struct list_head	*item,
 	struct list_lru_one	*lru,
 	spinlock_t		*lru_lock,
@@ -1798,7 +1798,7 @@ xfs_buftarg_wait_rele(
 
 	if (atomic_read(&bp->b_hold) > 1) {
 		/* need to wait, so skip it this pass */
-		trace_xfs_buf_wait_buftarg(bp, _RET_IP_);
+		trace_xfs_buf_drain_buftarg(bp, _RET_IP_);
 		return LRU_SKIP;
 	}
 	if (!spin_trylock(&bp->b_lock))
@@ -1816,7 +1816,7 @@ xfs_buftarg_wait_rele(
 }
 
 void
-xfs_wait_buftarg(
+xfs_buftarg_drain(
 	struct xfs_buftarg	*btp)
 {
 	LIST_HEAD(dispose);
@@ -1841,7 +1841,7 @@ xfs_wait_buftarg(
 
 	/* loop until there is nothing left on the lru list. */
 	while (list_lru_count(&btp->bt_lru)) {
-		list_lru_walk(&btp->bt_lru, xfs_buftarg_wait_rele,
+		list_lru_walk(&btp->bt_lru, xfs_buftarg_drain_rele,
 			      &dispose, LONG_MAX);
 
 		while (!list_empty(&dispose)) {
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index bfd2907e7bc4..ea32369f8f77 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -152,7 +152,7 @@ typedef struct xfs_buf {
 	struct list_head	b_list;
 	struct xfs_perag	*b_pag;		/* contains rbtree root */
 	struct xfs_mount	*b_mount;
-	xfs_buftarg_t		*b_target;	/* buffer target (device) */
+	struct xfs_buftarg	*b_target;	/* buffer target (device) */
 	void			*b_addr;	/* virtual address of buffer */
 	struct work_struct	b_ioend_work;
 	struct completion	b_iowait;	/* queue for I/O waiters */
@@ -344,11 +344,11 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 /*
  *	Handling of buftargs.
  */
-extern xfs_buftarg_t *xfs_alloc_buftarg(struct xfs_mount *,
-			struct block_device *, struct dax_device *);
+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_wait_buftarg(xfs_buftarg_t *);
-extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
+extern void xfs_buftarg_drain(struct xfs_buftarg *);
+extern int xfs_setsize_buftarg(struct xfs_buftarg *, unsigned int);
 
 #define xfs_getsize_buftarg(buftarg)	block_size((buftarg)->bt_bdev)
 #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index fa2d05e65ff1..5ad4d5e78019 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -741,7 +741,7 @@ xfs_log_mount_finish(
 		xfs_log_force(mp, XFS_LOG_SYNC);
 		xfs_ail_push_all_sync(mp->m_ail);
 	}
-	xfs_wait_buftarg(mp->m_ddev_targp);
+	xfs_buftarg_drain(mp->m_ddev_targp);
 
 	if (readonly)
 		mp->m_flags |= XFS_MOUNT_RDONLY;
@@ -936,13 +936,13 @@ xfs_log_quiesce(
 
 	/*
 	 * The superblock buffer is uncached and while xfs_ail_push_all_sync()
-	 * will push it, xfs_wait_buftarg() will not wait for it. Further,
+	 * will push it, xfs_buftarg_drain() 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_wait_buftarg(mp->m_ddev_targp);
+	xfs_buftarg_drain(mp->m_ddev_targp);
 	xfs_buf_lock(mp->m_sb_bp);
 	xfs_buf_unlock(mp->m_sb_bp);
 
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 7110507a2b6b..29a553f0877d 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1023,8 +1023,8 @@ xfs_mountfs(
 	xfs_log_mount_cancel(mp);
  out_fail_wait:
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
-		xfs_wait_buftarg(mp->m_logdev_targp);
-	xfs_wait_buftarg(mp->m_ddev_targp);
+		xfs_buftarg_drain(mp->m_logdev_targp);
+	xfs_buftarg_drain(mp->m_ddev_targp);
  out_free_perag:
 	xfs_free_perag(mp);
  out_free_dir:
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 86951652d3ed..7b4d8a5f2a49 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -340,7 +340,7 @@ DEFINE_BUF_EVENT(xfs_buf_get_uncached);
 DEFINE_BUF_EVENT(xfs_buf_item_relse);
 DEFINE_BUF_EVENT(xfs_buf_iodone_async);
 DEFINE_BUF_EVENT(xfs_buf_error_relse);
-DEFINE_BUF_EVENT(xfs_buf_wait_buftarg);
+DEFINE_BUF_EVENT(xfs_buf_drain_buftarg);
 DEFINE_BUF_EVENT(xfs_trans_read_buf_shut);
 
 /* not really buffer traces, but the buf provides useful information */
-- 
2.26.2

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

[Brian Foster]

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>
---
 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

Re: [PATCH 1/2] xfs: rename xfs_wait_buftarg() to xfs_buftarg_drain()

[Chandan Babu R]

On Thu, 10 Dec 2020 09:46:06 -0500, Brian Foster wrote:
> xfs_wait_buftarg() is vaguely named and somewhat overloaded. Its
> primary purpose is to reclaim all buffers from the provided buffer
> target LRU. In preparation to refactor xfs_wait_buftarg() into
> serialization and LRU draining components, rename the function and
> associated helpers to something more descriptive. This patch has no
> functional changes with the minor exception of renaming a
> tracepoint.
>

The changes look good to me.

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

> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_buf.c   | 12 ++++++------
>  fs/xfs/xfs_buf.h   | 10 +++++-----
>  fs/xfs/xfs_log.c   |  6 +++---
>  fs/xfs/xfs_mount.c |  4 ++--
>  fs/xfs/xfs_trace.h |  2 +-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4e4cf91f4f9f..db918ed20c40 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -43,7 +43,7 @@ static kmem_zone_t *xfs_buf_zone;
>   *	  pag_buf_lock
>   *	    lru_lock
>   *
> - * xfs_buftarg_wait_rele
> + * xfs_buftarg_drain_rele
>   *	lru_lock
>   *	  b_lock (trylock due to inversion)
>   *
> @@ -88,7 +88,7 @@ xfs_buf_vmap_len(
>   * because the corresponding decrement is deferred to buffer release. Buffers
>   * can undergo I/O multiple times in a hold-release cycle and per buffer I/O
>   * tracking adds unnecessary overhead. This is used for sychronization purposes
> - * with unmount (see xfs_wait_buftarg()), so all we really need is a count of
> + * with unmount (see xfs_buftarg_drain()), so all we really need is a count of
>   * in-flight buffers.
>   *
>   * Buffers that are never released (e.g., superblock, iclog buffers) must set
> @@ -1786,7 +1786,7 @@ __xfs_buf_mark_corrupt(
>   * while freeing all the buffers only held by the LRU.
>   */
>  static enum lru_status
> -xfs_buftarg_wait_rele(
> +xfs_buftarg_drain_rele(
>  	struct list_head	*item,
>  	struct list_lru_one	*lru,
>  	spinlock_t		*lru_lock,
> @@ -1798,7 +1798,7 @@ xfs_buftarg_wait_rele(
>  
>  	if (atomic_read(&bp->b_hold) > 1) {
>  		/* need to wait, so skip it this pass */
> -		trace_xfs_buf_wait_buftarg(bp, _RET_IP_);
> +		trace_xfs_buf_drain_buftarg(bp, _RET_IP_);
>  		return LRU_SKIP;
>  	}
>  	if (!spin_trylock(&bp->b_lock))
> @@ -1816,7 +1816,7 @@ xfs_buftarg_wait_rele(
>  }
>  
>  void
> -xfs_wait_buftarg(
> +xfs_buftarg_drain(
>  	struct xfs_buftarg	*btp)
>  {
>  	LIST_HEAD(dispose);
> @@ -1841,7 +1841,7 @@ xfs_wait_buftarg(
>  
>  	/* loop until there is nothing left on the lru list. */
>  	while (list_lru_count(&btp->bt_lru)) {
> -		list_lru_walk(&btp->bt_lru, xfs_buftarg_wait_rele,
> +		list_lru_walk(&btp->bt_lru, xfs_buftarg_drain_rele,
>  			      &dispose, LONG_MAX);
>  
>  		while (!list_empty(&dispose)) {
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index bfd2907e7bc4..ea32369f8f77 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -152,7 +152,7 @@ typedef struct xfs_buf {
>  	struct list_head	b_list;
>  	struct xfs_perag	*b_pag;		/* contains rbtree root */
>  	struct xfs_mount	*b_mount;
> -	xfs_buftarg_t		*b_target;	/* buffer target (device) */
> +	struct xfs_buftarg	*b_target;	/* buffer target (device) */
>  	void			*b_addr;	/* virtual address of buffer */
>  	struct work_struct	b_ioend_work;
>  	struct completion	b_iowait;	/* queue for I/O waiters */
> @@ -344,11 +344,11 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  /*
>   *	Handling of buftargs.
>   */
> -extern xfs_buftarg_t *xfs_alloc_buftarg(struct xfs_mount *,
> -			struct block_device *, struct dax_device *);
> +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_wait_buftarg(xfs_buftarg_t *);
> -extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> +extern void xfs_buftarg_drain(struct xfs_buftarg *);
> +extern int xfs_setsize_buftarg(struct xfs_buftarg *, unsigned int);
>  
>  #define xfs_getsize_buftarg(buftarg)	block_size((buftarg)->bt_bdev)
>  #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index fa2d05e65ff1..5ad4d5e78019 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -741,7 +741,7 @@ xfs_log_mount_finish(
>  		xfs_log_force(mp, XFS_LOG_SYNC);
>  		xfs_ail_push_all_sync(mp->m_ail);
>  	}
> -	xfs_wait_buftarg(mp->m_ddev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>  
>  	if (readonly)
>  		mp->m_flags |= XFS_MOUNT_RDONLY;
> @@ -936,13 +936,13 @@ xfs_log_quiesce(
>  
>  	/*
>  	 * The superblock buffer is uncached and while xfs_ail_push_all_sync()
> -	 * will push it, xfs_wait_buftarg() will not wait for it. Further,
> +	 * will push it, xfs_buftarg_drain() 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_wait_buftarg(mp->m_ddev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>  	xfs_buf_lock(mp->m_sb_bp);
>  	xfs_buf_unlock(mp->m_sb_bp);
>  
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 7110507a2b6b..29a553f0877d 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1023,8 +1023,8 @@ xfs_mountfs(
>  	xfs_log_mount_cancel(mp);
>   out_fail_wait:
>  	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> -		xfs_wait_buftarg(mp->m_logdev_targp);
> -	xfs_wait_buftarg(mp->m_ddev_targp);
> +		xfs_buftarg_drain(mp->m_logdev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>   out_free_perag:
>  	xfs_free_perag(mp);
>   out_free_dir:
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 86951652d3ed..7b4d8a5f2a49 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -340,7 +340,7 @@ DEFINE_BUF_EVENT(xfs_buf_get_uncached);
>  DEFINE_BUF_EVENT(xfs_buf_item_relse);
>  DEFINE_BUF_EVENT(xfs_buf_iodone_async);
>  DEFINE_BUF_EVENT(xfs_buf_error_relse);
> -DEFINE_BUF_EVENT(xfs_buf_wait_buftarg);
> +DEFINE_BUF_EVENT(xfs_buf_drain_buftarg);
>  DEFINE_BUF_EVENT(xfs_trans_read_buf_shut);
>  
>  /* not really buffer traces, but the buf provides useful information */
> 


-- 
chandan

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

[Chandan Babu R]

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,

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

> 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

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

[Brian Foster]

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
> 
> 
> 

Re: [PATCH 1/2] xfs: rename xfs_wait_buftarg() to xfs_buftarg_drain()

[Darrick J. Wong]

On Thu, Dec 10, 2020 at 09:46:06AM -0500, Brian Foster wrote:
> xfs_wait_buftarg() is vaguely named and somewhat overloaded. Its
> primary purpose is to reclaim all buffers from the provided buffer
> target LRU. In preparation to refactor xfs_wait_buftarg() into
> serialization and LRU draining components, rename the function and
> associated helpers to something more descriptive. This patch has no
> functional changes with the minor exception of renaming a
> tracepoint.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Hmmm, I guess my RVB never made it to the list...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c   | 12 ++++++------
>  fs/xfs/xfs_buf.h   | 10 +++++-----
>  fs/xfs/xfs_log.c   |  6 +++---
>  fs/xfs/xfs_mount.c |  4 ++--
>  fs/xfs/xfs_trace.h |  2 +-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4e4cf91f4f9f..db918ed20c40 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -43,7 +43,7 @@ static kmem_zone_t *xfs_buf_zone;
>   *	  pag_buf_lock
>   *	    lru_lock
>   *
> - * xfs_buftarg_wait_rele
> + * xfs_buftarg_drain_rele
>   *	lru_lock
>   *	  b_lock (trylock due to inversion)
>   *
> @@ -88,7 +88,7 @@ xfs_buf_vmap_len(
>   * because the corresponding decrement is deferred to buffer release. Buffers
>   * can undergo I/O multiple times in a hold-release cycle and per buffer I/O
>   * tracking adds unnecessary overhead. This is used for sychronization purposes
> - * with unmount (see xfs_wait_buftarg()), so all we really need is a count of
> + * with unmount (see xfs_buftarg_drain()), so all we really need is a count of
>   * in-flight buffers.
>   *
>   * Buffers that are never released (e.g., superblock, iclog buffers) must set
> @@ -1786,7 +1786,7 @@ __xfs_buf_mark_corrupt(
>   * while freeing all the buffers only held by the LRU.
>   */
>  static enum lru_status
> -xfs_buftarg_wait_rele(
> +xfs_buftarg_drain_rele(
>  	struct list_head	*item,
>  	struct list_lru_one	*lru,
>  	spinlock_t		*lru_lock,
> @@ -1798,7 +1798,7 @@ xfs_buftarg_wait_rele(
>  
>  	if (atomic_read(&bp->b_hold) > 1) {
>  		/* need to wait, so skip it this pass */
> -		trace_xfs_buf_wait_buftarg(bp, _RET_IP_);
> +		trace_xfs_buf_drain_buftarg(bp, _RET_IP_);
>  		return LRU_SKIP;
>  	}
>  	if (!spin_trylock(&bp->b_lock))
> @@ -1816,7 +1816,7 @@ xfs_buftarg_wait_rele(
>  }
>  
>  void
> -xfs_wait_buftarg(
> +xfs_buftarg_drain(
>  	struct xfs_buftarg	*btp)
>  {
>  	LIST_HEAD(dispose);
> @@ -1841,7 +1841,7 @@ xfs_wait_buftarg(
>  
>  	/* loop until there is nothing left on the lru list. */
>  	while (list_lru_count(&btp->bt_lru)) {
> -		list_lru_walk(&btp->bt_lru, xfs_buftarg_wait_rele,
> +		list_lru_walk(&btp->bt_lru, xfs_buftarg_drain_rele,
>  			      &dispose, LONG_MAX);
>  
>  		while (!list_empty(&dispose)) {
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index bfd2907e7bc4..ea32369f8f77 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -152,7 +152,7 @@ typedef struct xfs_buf {
>  	struct list_head	b_list;
>  	struct xfs_perag	*b_pag;		/* contains rbtree root */
>  	struct xfs_mount	*b_mount;
> -	xfs_buftarg_t		*b_target;	/* buffer target (device) */
> +	struct xfs_buftarg	*b_target;	/* buffer target (device) */
>  	void			*b_addr;	/* virtual address of buffer */
>  	struct work_struct	b_ioend_work;
>  	struct completion	b_iowait;	/* queue for I/O waiters */
> @@ -344,11 +344,11 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  /*
>   *	Handling of buftargs.
>   */
> -extern xfs_buftarg_t *xfs_alloc_buftarg(struct xfs_mount *,
> -			struct block_device *, struct dax_device *);
> +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_wait_buftarg(xfs_buftarg_t *);
> -extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> +extern void xfs_buftarg_drain(struct xfs_buftarg *);
> +extern int xfs_setsize_buftarg(struct xfs_buftarg *, unsigned int);
>  
>  #define xfs_getsize_buftarg(buftarg)	block_size((buftarg)->bt_bdev)
>  #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index fa2d05e65ff1..5ad4d5e78019 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -741,7 +741,7 @@ xfs_log_mount_finish(
>  		xfs_log_force(mp, XFS_LOG_SYNC);
>  		xfs_ail_push_all_sync(mp->m_ail);
>  	}
> -	xfs_wait_buftarg(mp->m_ddev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>  
>  	if (readonly)
>  		mp->m_flags |= XFS_MOUNT_RDONLY;
> @@ -936,13 +936,13 @@ xfs_log_quiesce(
>  
>  	/*
>  	 * The superblock buffer is uncached and while xfs_ail_push_all_sync()
> -	 * will push it, xfs_wait_buftarg() will not wait for it. Further,
> +	 * will push it, xfs_buftarg_drain() 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_wait_buftarg(mp->m_ddev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>  	xfs_buf_lock(mp->m_sb_bp);
>  	xfs_buf_unlock(mp->m_sb_bp);
>  
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 7110507a2b6b..29a553f0877d 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1023,8 +1023,8 @@ xfs_mountfs(
>  	xfs_log_mount_cancel(mp);
>   out_fail_wait:
>  	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> -		xfs_wait_buftarg(mp->m_logdev_targp);
> -	xfs_wait_buftarg(mp->m_ddev_targp);
> +		xfs_buftarg_drain(mp->m_logdev_targp);
> +	xfs_buftarg_drain(mp->m_ddev_targp);
>   out_free_perag:
>  	xfs_free_perag(mp);
>   out_free_dir:
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 86951652d3ed..7b4d8a5f2a49 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -340,7 +340,7 @@ DEFINE_BUF_EVENT(xfs_buf_get_uncached);
>  DEFINE_BUF_EVENT(xfs_buf_item_relse);
>  DEFINE_BUF_EVENT(xfs_buf_iodone_async);
>  DEFINE_BUF_EVENT(xfs_buf_error_relse);
> -DEFINE_BUF_EVENT(xfs_buf_wait_buftarg);
> +DEFINE_BUF_EVENT(xfs_buf_drain_buftarg);
>  DEFINE_BUF_EVENT(xfs_trans_read_buf_shut);
>  
>  /* not really buffer traces, but the buf provides useful information */
> -- 
> 2.26.2
> 

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

[Darrick J. Wong]

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
>