[PATCH v3] xfs: use ->b_state to fix buffer I/O accounting release race

[PATCH v3] xfs: use ->b_state to fix buffer I/O accounting release race

[Brian Foster]

We've had user reports of unmount hangs in xfs_wait_buftarg() that
analysis shows is due to btp->bt_io_count == -1. bt_io_count
represents the count of in-flight asynchronous buffers and thus
should always be >= 0. xfs_wait_buftarg() waits for this value to
stabilize to zero in order to ensure that all untracked (with
respect to the lru) buffers have completed I/O processing before
unmount proceeds to tear down in-core data structures.

The value of -1 implies an I/O accounting decrement race. Indeed,
the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
(where the buffer lock is no longer held) means that bp->b_flags can
be updated from an unsafe context. While a user-level reproducer is
currently not available, some intrusive hacks to run racing buffer
lookups/ioacct/releases from multiple threads was used to
successfully manufacture this problem.

Existing callers do not expect to acquire the buffer lock from
xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
this context. It turns out that we already have separate buffer
state bits and associated serialization for dealing with buffer LRU
state in the form of ->b_state and ->b_lock. Therefore, replace the
_XBF_IN_FLIGHT flag with a ->b_state variant, update the I/O
accounting wrappers appropriately and make sure they are used with
the correct locking. This ensures that buffer in-flight state can be
modified at buffer release time without racing with modifications
from a buffer lock holder.

Fixes: 9c7504aa72b6 ("xfs: track and serialize in-flight async buffers against unmount")
CC: stable@vger.kernel.org # v4.8+
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---

v3:
- Added Fixes/Reviewed-by tags and CC stable.
v2: http://www.spinics.net/lists/linux-xfs/msg07062.html
- Use ->b_state and ->b_lock instead of custom atomic counter.
v1: http://www.spinics.net/lists/linux-xfs/msg06995.html

 fs/xfs/xfs_buf.c | 38 ++++++++++++++++++++++++++------------
 fs/xfs/xfs_buf.h |  5 ++---
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ba036c1..4e19fda 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -97,12 +97,16 @@ static inline void
 xfs_buf_ioacct_inc(
 	struct xfs_buf	*bp)
 {
-	if (bp->b_flags & (XBF_NO_IOACCT|_XBF_IN_FLIGHT))
+	if (bp->b_flags & XBF_NO_IOACCT)
 		return;
 
 	ASSERT(bp->b_flags & XBF_ASYNC);
-	bp->b_flags |= _XBF_IN_FLIGHT;
-	percpu_counter_inc(&bp->b_target->bt_io_count);
+	spin_lock(&bp->b_lock);
+	if (!(bp->b_state & XFS_BSTATE_IN_FLIGHT)) {
+		bp->b_state |= XFS_BSTATE_IN_FLIGHT;
+		percpu_counter_inc(&bp->b_target->bt_io_count);
+	}
+	spin_unlock(&bp->b_lock);
 }
 
 /*
@@ -110,14 +114,24 @@ xfs_buf_ioacct_inc(
  * freed and unaccount from the buftarg.
  */
 static inline void
-xfs_buf_ioacct_dec(
+__xfs_buf_ioacct_dec(
 	struct xfs_buf	*bp)
 {
-	if (!(bp->b_flags & _XBF_IN_FLIGHT))
-		return;
+	ASSERT(spin_is_locked(&bp->b_lock));
 
-	bp->b_flags &= ~_XBF_IN_FLIGHT;
-	percpu_counter_dec(&bp->b_target->bt_io_count);
+	if (bp->b_state & XFS_BSTATE_IN_FLIGHT) {
+		bp->b_state &= ~XFS_BSTATE_IN_FLIGHT;
+		percpu_counter_dec(&bp->b_target->bt_io_count);
+	}
+}
+
+static inline void
+xfs_buf_ioacct_dec(
+	struct xfs_buf	*bp)
+{
+	spin_lock(&bp->b_lock);
+	__xfs_buf_ioacct_dec(bp);
+	spin_unlock(&bp->b_lock);
 }
 
 /*
@@ -149,9 +163,9 @@ xfs_buf_stale(
 	 * unaccounted (released to LRU) before that occurs. Drop in-flight
 	 * status now to preserve accounting consistency.
 	 */
-	xfs_buf_ioacct_dec(bp);
-
 	spin_lock(&bp->b_lock);
+	__xfs_buf_ioacct_dec(bp);
+
 	atomic_set(&bp->b_lru_ref, 0);
 	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&
 	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
@@ -979,12 +993,12 @@ xfs_buf_rele(
 		 * ensures the decrement occurs only once per-buf.
 		 */
 		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
-			xfs_buf_ioacct_dec(bp);
+			__xfs_buf_ioacct_dec(bp);
 		goto out_unlock;
 	}
 
 	/* the last reference has been dropped ... */
-	xfs_buf_ioacct_dec(bp);
+	__xfs_buf_ioacct_dec(bp);
 	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {
 		/*
 		 * If the buffer is added to the LRU take a new reference to the
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 8d1d44f..1508121 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -63,7 +63,6 @@ typedef enum {
 #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
 #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
 #define _XBF_COMPOUND	 (1 << 23)/* compound buffer */
-#define _XBF_IN_FLIGHT	 (1 << 25) /* I/O in flight, for accounting purposes */
 
 typedef unsigned int xfs_buf_flags_t;
 
@@ -84,14 +83,14 @@ typedef unsigned int xfs_buf_flags_t;
 	{ _XBF_PAGES,		"PAGES" }, \
 	{ _XBF_KMEM,		"KMEM" }, \
 	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
-	{ _XBF_COMPOUND,	"COMPOUND" }, \
-	{ _XBF_IN_FLIGHT,	"IN_FLIGHT" }
+	{ _XBF_COMPOUND,	"COMPOUND" }
 
 
 /*
  * Internal state flags.
  */
 #define XFS_BSTATE_DISPOSE	 (1 << 0)	/* buffer being discarded */
+#define XFS_BSTATE_IN_FLIGHT	 (1 << 1)	/* I/O in flight */
 
 /*
  * The xfs_buftarg contains 2 notions of "sector size" -
-- 
2.7.5

Re: [PATCH v3] xfs: use ->b_state to fix buffer I/O accounting release race

[Brian Foster]

Oops, fixed stable CC.

On Tue, May 30, 2017 at 09:22:53AM -0400, Brian Foster wrote:
> We've had user reports of unmount hangs in xfs_wait_buftarg() that
> analysis shows is due to btp->bt_io_count == -1. bt_io_count
> represents the count of in-flight asynchronous buffers and thus
> should always be >= 0. xfs_wait_buftarg() waits for this value to
> stabilize to zero in order to ensure that all untracked (with
> respect to the lru) buffers have completed I/O processing before
> unmount proceeds to tear down in-core data structures.
> 
> The value of -1 implies an I/O accounting decrement race. Indeed,
> the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
> (where the buffer lock is no longer held) means that bp->b_flags can
> be updated from an unsafe context. While a user-level reproducer is
> currently not available, some intrusive hacks to run racing buffer
> lookups/ioacct/releases from multiple threads was used to
> successfully manufacture this problem.
> 
> Existing callers do not expect to acquire the buffer lock from
> xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
> this context. It turns out that we already have separate buffer
> state bits and associated serialization for dealing with buffer LRU
> state in the form of ->b_state and ->b_lock. Therefore, replace the
> _XBF_IN_FLIGHT flag with a ->b_state variant, update the I/O
> accounting wrappers appropriately and make sure they are used with
> the correct locking. This ensures that buffer in-flight state can be
> modified at buffer release time without racing with modifications
> from a buffer lock holder.
> 
> Fixes: 9c7504aa72b6 ("xfs: track and serialize in-flight async buffers against unmount")
> CC: stable@vger.kernel.org # v4.8+
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> v3:
> - Added Fixes/Reviewed-by tags and CC stable.
> v2: http://www.spinics.net/lists/linux-xfs/msg07062.html
> - Use ->b_state and ->b_lock instead of custom atomic counter.
> v1: http://www.spinics.net/lists/linux-xfs/msg06995.html
> 
>  fs/xfs/xfs_buf.c | 38 ++++++++++++++++++++++++++------------
>  fs/xfs/xfs_buf.h |  5 ++---
>  2 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ba036c1..4e19fda 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -97,12 +97,16 @@ static inline void
>  xfs_buf_ioacct_inc(
>  	struct xfs_buf	*bp)
>  {
> -	if (bp->b_flags & (XBF_NO_IOACCT|_XBF_IN_FLIGHT))
> +	if (bp->b_flags & XBF_NO_IOACCT)
>  		return;
>  
>  	ASSERT(bp->b_flags & XBF_ASYNC);
> -	bp->b_flags |= _XBF_IN_FLIGHT;
> -	percpu_counter_inc(&bp->b_target->bt_io_count);
> +	spin_lock(&bp->b_lock);
> +	if (!(bp->b_state & XFS_BSTATE_IN_FLIGHT)) {
> +		bp->b_state |= XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_inc(&bp->b_target->bt_io_count);
> +	}
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -110,14 +114,24 @@ xfs_buf_ioacct_inc(
>   * freed and unaccount from the buftarg.
>   */
>  static inline void
> -xfs_buf_ioacct_dec(
> +__xfs_buf_ioacct_dec(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(bp->b_flags & _XBF_IN_FLIGHT))
> -		return;
> +	ASSERT(spin_is_locked(&bp->b_lock));
>  
> -	bp->b_flags &= ~_XBF_IN_FLIGHT;
> -	percpu_counter_dec(&bp->b_target->bt_io_count);
> +	if (bp->b_state & XFS_BSTATE_IN_FLIGHT) {
> +		bp->b_state &= ~XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_dec(&bp->b_target->bt_io_count);
> +	}
> +}
> +
> +static inline void
> +xfs_buf_ioacct_dec(
> +	struct xfs_buf	*bp)
> +{
> +	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -149,9 +163,9 @@ xfs_buf_stale(
>  	 * unaccounted (released to LRU) before that occurs. Drop in-flight
>  	 * status now to preserve accounting consistency.
>  	 */
> -	xfs_buf_ioacct_dec(bp);
> -
>  	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +
>  	atomic_set(&bp->b_lru_ref, 0);
>  	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&
>  	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
> @@ -979,12 +993,12 @@ xfs_buf_rele(
>  		 * ensures the decrement occurs only once per-buf.
>  		 */
>  		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
> -			xfs_buf_ioacct_dec(bp);
> +			__xfs_buf_ioacct_dec(bp);
>  		goto out_unlock;
>  	}
>  
>  	/* the last reference has been dropped ... */
> -	xfs_buf_ioacct_dec(bp);
> +	__xfs_buf_ioacct_dec(bp);
>  	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {
>  		/*
>  		 * If the buffer is added to the LRU take a new reference to the
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 8d1d44f..1508121 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -63,7 +63,6 @@ typedef enum {
>  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
>  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
>  #define _XBF_COMPOUND	 (1 << 23)/* compound buffer */
> -#define _XBF_IN_FLIGHT	 (1 << 25) /* I/O in flight, for accounting purposes */
>  
>  typedef unsigned int xfs_buf_flags_t;
>  
> @@ -84,14 +83,14 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ _XBF_PAGES,		"PAGES" }, \
>  	{ _XBF_KMEM,		"KMEM" }, \
>  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> -	{ _XBF_COMPOUND,	"COMPOUND" }, \
> -	{ _XBF_IN_FLIGHT,	"IN_FLIGHT" }
> +	{ _XBF_COMPOUND,	"COMPOUND" }
>  
>  
>  /*
>   * Internal state flags.
>   */
>  #define XFS_BSTATE_DISPOSE	 (1 << 0)	/* buffer being discarded */
> +#define XFS_BSTATE_IN_FLIGHT	 (1 << 1)	/* I/O in flight */
>  
>  /*
>   * The xfs_buftarg contains 2 notions of "sector size" -
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] xfs: use ->b_state to fix buffer I/O accounting release race

[Nikolay Borisov]

On 30.05.2017 16:22, Brian Foster wrote:
> We've had user reports of unmount hangs in xfs_wait_buftarg() that
> analysis shows is due to btp->bt_io_count == -1. bt_io_count
> represents the count of in-flight asynchronous buffers and thus
> should always be >= 0. xfs_wait_buftarg() waits for this value to
> stabilize to zero in order to ensure that all untracked (with
> respect to the lru) buffers have completed I/O processing before
> unmount proceeds to tear down in-core data structures.
> 
> The value of -1 implies an I/O accounting decrement race. Indeed,
> the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
> (where the buffer lock is no longer held) means that bp->b_flags can
> be updated from an unsafe context. While a user-level reproducer is
> currently not available, some intrusive hacks to run racing buffer
> lookups/ioacct/releases from multiple threads was used to
> successfully manufacture this problem.
> 
> Existing callers do not expect to acquire the buffer lock from
> xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
> this context. It turns out that we already have separate buffer
> state bits and associated serialization for dealing with buffer LRU
> state in the form of ->b_state and ->b_lock. Therefore, replace the
> _XBF_IN_FLIGHT flag with a ->b_state variant, update the I/O
> accounting wrappers appropriately and make sure they are used with
> the correct locking. This ensures that buffer in-flight state can be
> modified at buffer release time without racing with modifications
> from a buffer lock holder.
> 
> Fixes: 9c7504aa72b6 ("xfs: track and serialize in-flight async buffers against unmount")
> CC: stable@vger.kernel.org # v4.8+
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Our QA guys managed to reliably reproduce this on an HP GEN8 box,
running test_dbench-4_0_async-scenario-run (I don't know if it's some
internal flavor of dbench). With this patch applied the issue is gone.
You can add:

Tested-by: Libor Pechacek <lpechacek@suse.com>

> ---
> 
> v3:
> - Added Fixes/Reviewed-by tags and CC stable.
> v2: http://www.spinics.net/lists/linux-xfs/msg07062.html
> - Use ->b_state and ->b_lock instead of custom atomic counter.
> v1: http://www.spinics.net/lists/linux-xfs/msg06995.html
> 
>  fs/xfs/xfs_buf.c | 38 ++++++++++++++++++++++++++------------
>  fs/xfs/xfs_buf.h |  5 ++---
>  2 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ba036c1..4e19fda 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -97,12 +97,16 @@ static inline void
>  xfs_buf_ioacct_inc(
>  	struct xfs_buf	*bp)
>  {
> -	if (bp->b_flags & (XBF_NO_IOACCT|_XBF_IN_FLIGHT))
> +	if (bp->b_flags & XBF_NO_IOACCT)
>  		return;
>  
>  	ASSERT(bp->b_flags & XBF_ASYNC);
> -	bp->b_flags |= _XBF_IN_FLIGHT;
> -	percpu_counter_inc(&bp->b_target->bt_io_count);
> +	spin_lock(&bp->b_lock);
> +	if (!(bp->b_state & XFS_BSTATE_IN_FLIGHT)) {
> +		bp->b_state |= XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_inc(&bp->b_target->bt_io_count);
> +	}
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -110,14 +114,24 @@ xfs_buf_ioacct_inc(
>   * freed and unaccount from the buftarg.
>   */
>  static inline void
> -xfs_buf_ioacct_dec(
> +__xfs_buf_ioacct_dec(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(bp->b_flags & _XBF_IN_FLIGHT))
> -		return;
> +	ASSERT(spin_is_locked(&bp->b_lock));
>  
> -	bp->b_flags &= ~_XBF_IN_FLIGHT;
> -	percpu_counter_dec(&bp->b_target->bt_io_count);
> +	if (bp->b_state & XFS_BSTATE_IN_FLIGHT) {
> +		bp->b_state &= ~XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_dec(&bp->b_target->bt_io_count);
> +	}
> +}
> +
> +static inline void
> +xfs_buf_ioacct_dec(
> +	struct xfs_buf	*bp)
> +{
> +	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -149,9 +163,9 @@ xfs_buf_stale(
>  	 * unaccounted (released to LRU) before that occurs. Drop in-flight
>  	 * status now to preserve accounting consistency.
>  	 */
> -	xfs_buf_ioacct_dec(bp);
> -
>  	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +
>  	atomic_set(&bp->b_lru_ref, 0);
>  	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&
>  	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
> @@ -979,12 +993,12 @@ xfs_buf_rele(
>  		 * ensures the decrement occurs only once per-buf.
>  		 */
>  		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
> -			xfs_buf_ioacct_dec(bp);
> +			__xfs_buf_ioacct_dec(bp);
>  		goto out_unlock;
>  	}
>  
>  	/* the last reference has been dropped ... */
> -	xfs_buf_ioacct_dec(bp);
> +	__xfs_buf_ioacct_dec(bp);
>  	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {
>  		/*
>  		 * If the buffer is added to the LRU take a new reference to the
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 8d1d44f..1508121 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -63,7 +63,6 @@ typedef enum {
>  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
>  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
>  #define _XBF_COMPOUND	 (1 << 23)/* compound buffer */
> -#define _XBF_IN_FLIGHT	 (1 << 25) /* I/O in flight, for accounting purposes */
>  
>  typedef unsigned int xfs_buf_flags_t;
>  
> @@ -84,14 +83,14 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ _XBF_PAGES,		"PAGES" }, \
>  	{ _XBF_KMEM,		"KMEM" }, \
>  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> -	{ _XBF_COMPOUND,	"COMPOUND" }, \
> -	{ _XBF_IN_FLIGHT,	"IN_FLIGHT" }
> +	{ _XBF_COMPOUND,	"COMPOUND" }
>  
>  
>  /*
>   * Internal state flags.
>   */
>  #define XFS_BSTATE_DISPOSE	 (1 << 0)	/* buffer being discarded */
> +#define XFS_BSTATE_IN_FLIGHT	 (1 << 1)	/* I/O in flight */
>  
>  /*
>   * The xfs_buftarg contains 2 notions of "sector size" -
> 

Re: [PATCH v3] xfs: use ->b_state to fix buffer I/O accounting release race

[Darrick J. Wong]

On Tue, May 30, 2017 at 09:22:53AM -0400, Brian Foster wrote:
> We've had user reports of unmount hangs in xfs_wait_buftarg() that
> analysis shows is due to btp->bt_io_count == -1. bt_io_count
> represents the count of in-flight asynchronous buffers and thus
> should always be >= 0. xfs_wait_buftarg() waits for this value to
> stabilize to zero in order to ensure that all untracked (with
> respect to the lru) buffers have completed I/O processing before
> unmount proceeds to tear down in-core data structures.
> 
> The value of -1 implies an I/O accounting decrement race. Indeed,
> the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
> (where the buffer lock is no longer held) means that bp->b_flags can
> be updated from an unsafe context. While a user-level reproducer is
> currently not available, some intrusive hacks to run racing buffer
> lookups/ioacct/releases from multiple threads was used to
> successfully manufacture this problem.
> 
> Existing callers do not expect to acquire the buffer lock from
> xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
> this context. It turns out that we already have separate buffer
> state bits and associated serialization for dealing with buffer LRU
> state in the form of ->b_state and ->b_lock. Therefore, replace the
> _XBF_IN_FLIGHT flag with a ->b_state variant, update the I/O
> accounting wrappers appropriately and make sure they are used with
> the correct locking. This ensures that buffer in-flight state can be
> modified at buffer release time without racing with modifications
> from a buffer lock holder.
> 
> Fixes: 9c7504aa72b6 ("xfs: track and serialize in-flight async buffers against unmount")
> CC: stable@vger.kernel.org # v4.8+
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Looks good enough to test,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> v3:
> - Added Fixes/Reviewed-by tags and CC stable.
> v2: http://www.spinics.net/lists/linux-xfs/msg07062.html
> - Use ->b_state and ->b_lock instead of custom atomic counter.
> v1: http://www.spinics.net/lists/linux-xfs/msg06995.html
> 
>  fs/xfs/xfs_buf.c | 38 ++++++++++++++++++++++++++------------
>  fs/xfs/xfs_buf.h |  5 ++---
>  2 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ba036c1..4e19fda 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -97,12 +97,16 @@ static inline void
>  xfs_buf_ioacct_inc(
>  	struct xfs_buf	*bp)
>  {
> -	if (bp->b_flags & (XBF_NO_IOACCT|_XBF_IN_FLIGHT))
> +	if (bp->b_flags & XBF_NO_IOACCT)
>  		return;
>  
>  	ASSERT(bp->b_flags & XBF_ASYNC);
> -	bp->b_flags |= _XBF_IN_FLIGHT;
> -	percpu_counter_inc(&bp->b_target->bt_io_count);
> +	spin_lock(&bp->b_lock);
> +	if (!(bp->b_state & XFS_BSTATE_IN_FLIGHT)) {
> +		bp->b_state |= XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_inc(&bp->b_target->bt_io_count);
> +	}
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -110,14 +114,24 @@ xfs_buf_ioacct_inc(
>   * freed and unaccount from the buftarg.
>   */
>  static inline void
> -xfs_buf_ioacct_dec(
> +__xfs_buf_ioacct_dec(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(bp->b_flags & _XBF_IN_FLIGHT))
> -		return;
> +	ASSERT(spin_is_locked(&bp->b_lock));
>  
> -	bp->b_flags &= ~_XBF_IN_FLIGHT;
> -	percpu_counter_dec(&bp->b_target->bt_io_count);
> +	if (bp->b_state & XFS_BSTATE_IN_FLIGHT) {
> +		bp->b_state &= ~XFS_BSTATE_IN_FLIGHT;
> +		percpu_counter_dec(&bp->b_target->bt_io_count);
> +	}
> +}
> +
> +static inline void
> +xfs_buf_ioacct_dec(
> +	struct xfs_buf	*bp)
> +{
> +	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +	spin_unlock(&bp->b_lock);
>  }
>  
>  /*
> @@ -149,9 +163,9 @@ xfs_buf_stale(
>  	 * unaccounted (released to LRU) before that occurs. Drop in-flight
>  	 * status now to preserve accounting consistency.
>  	 */
> -	xfs_buf_ioacct_dec(bp);
> -
>  	spin_lock(&bp->b_lock);
> +	__xfs_buf_ioacct_dec(bp);
> +
>  	atomic_set(&bp->b_lru_ref, 0);
>  	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&
>  	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
> @@ -979,12 +993,12 @@ xfs_buf_rele(
>  		 * ensures the decrement occurs only once per-buf.
>  		 */
>  		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
> -			xfs_buf_ioacct_dec(bp);
> +			__xfs_buf_ioacct_dec(bp);
>  		goto out_unlock;
>  	}
>  
>  	/* the last reference has been dropped ... */
> -	xfs_buf_ioacct_dec(bp);
> +	__xfs_buf_ioacct_dec(bp);
>  	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {
>  		/*
>  		 * If the buffer is added to the LRU take a new reference to the
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 8d1d44f..1508121 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -63,7 +63,6 @@ typedef enum {
>  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
>  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
>  #define _XBF_COMPOUND	 (1 << 23)/* compound buffer */
> -#define _XBF_IN_FLIGHT	 (1 << 25) /* I/O in flight, for accounting purposes */
>  
>  typedef unsigned int xfs_buf_flags_t;
>  
> @@ -84,14 +83,14 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ _XBF_PAGES,		"PAGES" }, \
>  	{ _XBF_KMEM,		"KMEM" }, \
>  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> -	{ _XBF_COMPOUND,	"COMPOUND" }, \
> -	{ _XBF_IN_FLIGHT,	"IN_FLIGHT" }
> +	{ _XBF_COMPOUND,	"COMPOUND" }
>  
>  
>  /*
>   * Internal state flags.
>   */
>  #define XFS_BSTATE_DISPOSE	 (1 << 0)	/* buffer being discarded */
> +#define XFS_BSTATE_IN_FLIGHT	 (1 << 1)	/* I/O in flight */
>  
>  /*
>   * The xfs_buftarg contains 2 notions of "sector size" -
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html