misc buffer cache improvements

misc buffer cache improvements

[Christoph Hellwig]

Hi all,

a bunch of misc xfs_buf patches.  These came out of a little
project that needs a async read API (not actually included here),
and to get there a lot of lose ends had to be cleaned up, and a
lockless path for readaheads that doesn't kick off the read fell
out easily.

Diffstat:
 libxfs/xfs_btree_staging.c |    7 
 libxfs/xfs_dquot_buf.c     |    8 
 libxfs/xfs_ialloc.c        |    2 
 libxfs/xfs_inode_buf.c     |   15 -
 xfs_buf.c                  |  392 +++++++++++++++++++++++----------------------
 xfs_buf.h                  |    8 
 xfs_buf_item.c             |   10 -
 xfs_buf_item_recover.c     |    3 
 xfs_dquot_item_recover.c   |    1 
 xfs_fsops.c                |    2 
 xfs_inode.c                |    4 
 xfs_inode_item_recover.c   |    1 
 xfs_log_recover.c          |    5 
 xfs_trace.h                |    2 
 xfs_trans_buf.c            |    7 
 15 files changed, 239 insertions(+), 228 deletions(-)

[PATCH 01/12] xfs: don't get a pag reference in xfs_buf_get_map

[Christoph Hellwig]

As of commit 497560b9ef42 ("xfs: switch (back) to a per-buftarg buffer
hash"), buffer lookups don't require the perag structure.  Stop looking
it up in xfs_buf_get_map, and instead only find it when allocating a new
buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 44 ++++++++++----------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index e1465e950acc..54e091315d56 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -469,7 +469,6 @@ xfs_buf_lookup(
 static int
 xfs_buf_find_insert(
 	struct xfs_buftarg	*btp,
-	struct xfs_perag	*pag,
 	struct xfs_buf_map	*cmap,
 	struct xfs_buf_map	*map,
 	int			nmaps,
@@ -482,10 +481,13 @@ xfs_buf_find_insert(
 
 	error = xfs_buf_alloc(btp, map, nmaps, flags, &new_bp);
 	if (error)
-		goto out_drop_pag;
+		return error;
 
 	/* The new buffer keeps the perag reference until it is freed. */
-	new_bp->b_pag = pag;
+	if (!xfs_buftarg_is_mem(btp)) {
+		new_bp->b_pag = xfs_perag_get(btp->bt_mount,
+			xfs_daddr_to_agno(btp->bt_mount, cmap->bm_bn));
+	}
 
 retry:
 	rcu_read_lock();
@@ -520,25 +522,12 @@ xfs_buf_find_insert(
 	return 0;
 
 out_free_buf:
+	if (new_bp->b_pag)
+		xfs_perag_put(new_bp->b_pag);
 	xfs_buf_free(new_bp);
-out_drop_pag:
-	if (pag)
-		xfs_perag_put(pag);
 	return error;
 }
 
-static inline struct xfs_perag *
-xfs_buftarg_get_pag(
-	struct xfs_buftarg		*btp,
-	const struct xfs_buf_map	*map)
-{
-	struct xfs_mount		*mp = btp->bt_mount;
-
-	if (xfs_buftarg_is_mem(btp))
-		return NULL;
-	return xfs_perag_get(mp, xfs_daddr_to_agno(mp, map->bm_bn));
-}
-
 /*
  * Assembles a buffer covering the specified range. The code is optimised for
  * cache hits, as metadata intensive workloads will see 3 orders of magnitude
@@ -552,7 +541,6 @@ xfs_buf_get_map(
 	xfs_buf_flags_t		flags,
 	struct xfs_buf		**bpp)
 {
-	struct xfs_perag	*pag;
 	struct xfs_buf		*bp = NULL;
 	struct xfs_buf_map	cmap = { .bm_bn = map[0].bm_bn };
 	int			error;
@@ -567,28 +555,21 @@ xfs_buf_get_map(
 	if (error)
 		return error;
 
-	pag = xfs_buftarg_get_pag(btp, &cmap);
-
 	error = xfs_buf_lookup(btp, &cmap, flags, &bp);
 	if (error && error != -ENOENT)
-		goto out_put_perag;
+		return error;
 
 	/* cache hits always outnumber misses by at least 10:1 */
 	if (unlikely(!bp)) {
 		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
 
 		if (flags & XBF_INCORE)
-			goto out_put_perag;
-
-		/* xfs_buf_find_insert() consumes the perag reference. */
-		error = xfs_buf_find_insert(btp, pag, &cmap, map, nmaps,
-				flags, &bp);
+			return 0;
+		error = xfs_buf_find_insert(btp, &cmap, map, nmaps, flags, &bp);
 		if (error)
 			return error;
 	} else {
 		XFS_STATS_INC(btp->bt_mount, xb_get_locked);
-		if (pag)
-			xfs_perag_put(pag);
 	}
 
 	/*
@@ -602,11 +583,6 @@ xfs_buf_get_map(
 	trace_xfs_buf_get(bp, flags, _RET_IP_);
 	*bpp = bp;
 	return 0;
-
-out_put_perag:
-	if (pag)
-		xfs_perag_put(pag);
-	return error;
 }
 
 int
-- 
2.53.0

[PATCH 02/12] xfs: consolidate buffer locking in xfs_buf_get_map

[Christoph Hellwig]

Consolidate the code to lock the buffer based on the passed in flags
into xfs_buf_get_map instead of having two different sites for buffer
lookup vs insertation.  This requires initializing b_lock to unlocked on
allocation and doing an atomic for locking it for newly allocated buffers,
but greatly simplifies the logic.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c   | 62 ++++++++++++++++------------------------------
 fs/xfs/xfs_trace.h |  2 +-
 2 files changed, 23 insertions(+), 41 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 54e091315d56..e56d4b8b0771 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -282,15 +282,8 @@ xfs_buf_alloc(
 	 * specifically set by later operations on the buffer.
 	 */
 	flags &= ~(XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD);
-
-	/*
-	 * A new buffer is held and locked by the owner.  This ensures that the
-	 * buffer is owned by the caller and racing RCU lookups right after
-	 * inserting into the hash table are safe (and will have to wait for
-	 * the unlock to do anything non-trivial).
-	 */
 	lockref_init(&bp->b_lockref);
-	sema_init(&bp->b_sema, 0); /* held, no waiters */
+	sema_init(&bp->b_sema, 1); /* unlocked */
 	atomic_set(&bp->b_lru_ref, 1);
 	init_completion(&bp->b_iowait);
 	INIT_LIST_HEAD(&bp->b_lru);
@@ -433,33 +426,25 @@ xfs_buf_find_lock(
 	return 0;
 }
 
-static inline int
+static inline struct xfs_buf *
 xfs_buf_lookup(
 	struct xfs_buftarg	*btp,
-	struct xfs_buf_map	*map,
-	xfs_buf_flags_t		flags,
-	struct xfs_buf		**bpp)
+	struct xfs_buf_map	*map)
 {
 	struct xfs_buf          *bp;
-	int			error;
 
 	rcu_read_lock();
 	bp = rhashtable_lookup(&btp->bt_hash, map, xfs_buf_hash_params);
 	if (!bp || !lockref_get_not_dead(&bp->b_lockref)) {
 		rcu_read_unlock();
-		return -ENOENT;
+		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
+		return NULL;
 	}
 	rcu_read_unlock();
 
-	error = xfs_buf_find_lock(bp, flags);
-	if (error) {
-		xfs_buf_rele(bp);
-		return error;
-	}
-
-	trace_xfs_buf_find(bp, flags, _RET_IP_);
-	*bpp = bp;
-	return 0;
+	trace_xfs_buf_find(bp, _RET_IP_);
+	XFS_STATS_INC(btp->bt_mount, xb_get_locked);
+	return bp;
 }
 
 /*
@@ -509,11 +494,7 @@ xfs_buf_find_insert(
 			goto retry;
 		}
 		rcu_read_unlock();
-		error = xfs_buf_find_lock(bp, flags);
-		if (error)
-			xfs_buf_rele(bp);
-		else
-			*bpp = bp;
+		*bpp = bp;
 		goto out_free_buf;
 	}
 	rcu_read_unlock();
@@ -555,21 +536,20 @@ xfs_buf_get_map(
 	if (error)
 		return error;
 
-	error = xfs_buf_lookup(btp, &cmap, flags, &bp);
-	if (error && error != -ENOENT)
-		return error;
-
 	/* cache hits always outnumber misses by at least 10:1 */
+	bp = xfs_buf_lookup(btp, &cmap);
 	if (unlikely(!bp)) {
-		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
-
 		if (flags & XBF_INCORE)
-			return 0;
+			return -ENOENT;
 		error = xfs_buf_find_insert(btp, &cmap, map, nmaps, flags, &bp);
 		if (error)
 			return error;
-	} else {
-		XFS_STATS_INC(btp->bt_mount, xb_get_locked);
+	}
+
+	error = xfs_buf_find_lock(bp, flags);
+	if (error) {
+		xfs_buf_rele(bp);
+		return error;
 	}
 
 	/*
@@ -794,9 +774,11 @@ xfs_buf_get_uncached(
 	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
 
 	error = xfs_buf_alloc(target, &map, 1, 0, bpp);
-	if (!error)
-		trace_xfs_buf_get_uncached(*bpp, _RET_IP_);
-	return error;
+	if (error)
+		return error;
+	xfs_buf_lock(*bpp);
+	trace_xfs_buf_get_uncached(*bpp, _RET_IP_);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index aeb89ac53bf1..f333c938fbd9 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -792,6 +792,7 @@ DEFINE_BUF_EVENT(xfs_buf_backing_folio);
 DEFINE_BUF_EVENT(xfs_buf_backing_kmem);
 DEFINE_BUF_EVENT(xfs_buf_backing_vmalloc);
 DEFINE_BUF_EVENT(xfs_buf_backing_fallback);
+DEFINE_BUF_EVENT(xfs_buf_find);
 
 /* not really buffer traces, but the buf provides useful information */
 DEFINE_BUF_EVENT(xfs_btree_corrupt);
@@ -837,7 +838,6 @@ DECLARE_EVENT_CLASS(xfs_buf_flags_class,
 DEFINE_EVENT(xfs_buf_flags_class, name, \
 	TP_PROTO(struct xfs_buf *bp, unsigned flags, unsigned long caller_ip), \
 	TP_ARGS(bp, flags, caller_ip))
-DEFINE_BUF_FLAGS_EVENT(xfs_buf_find);
 DEFINE_BUF_FLAGS_EVENT(xfs_buf_get);
 DEFINE_BUF_FLAGS_EVENT(xfs_buf_read);
 DEFINE_BUF_FLAGS_EVENT(xfs_buf_readahead);
-- 
2.53.0

Re: [PATCH 02/12] xfs: consolidate buffer locking in xfs_buf_get_map

[Brian Foster]

On Wed, Jul 15, 2026 at 04:50:55PM +0200, Christoph Hellwig wrote:
> Consolidate the code to lock the buffer based on the passed in flags
> into xfs_buf_get_map instead of having two different sites for buffer
> lookup vs insertation.  This requires initializing b_lock to unlocked on
> allocation and doing an atomic for locking it for newly allocated buffers,
> but greatly simplifies the logic.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c   | 62 ++++++++++++++++------------------------------
>  fs/xfs/xfs_trace.h |  2 +-
>  2 files changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 54e091315d56..e56d4b8b0771 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -282,15 +282,8 @@ xfs_buf_alloc(
>  	 * specifically set by later operations on the buffer.
>  	 */
>  	flags &= ~(XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD);
> -
> -	/*
> -	 * A new buffer is held and locked by the owner.  This ensures that the
> -	 * buffer is owned by the caller and racing RCU lookups right after
> -	 * inserting into the hash table are safe (and will have to wait for
> -	 * the unlock to do anything non-trivial).
> -	 */
>  	lockref_init(&bp->b_lockref);
> -	sema_init(&bp->b_sema, 0); /* held, no waiters */
> +	sema_init(&bp->b_sema, 1); /* unlocked */

Maybe I'm missing something, but I thought the point of the locked init
was to ensure that the allocator owns the buffer and can't end up
blocking on the lock with a racing lookup. I.e., the buffer is locked
before it becomes available for external lookup. Do we plan to do that
another way, or are we just dropping that assumption?

Brian

>  	atomic_set(&bp->b_lru_ref, 1);
>  	init_completion(&bp->b_iowait);
>  	INIT_LIST_HEAD(&bp->b_lru);
> @@ -433,33 +426,25 @@ xfs_buf_find_lock(
>  	return 0;
>  }
>  
> -static inline int
> +static inline struct xfs_buf *
>  xfs_buf_lookup(
>  	struct xfs_buftarg	*btp,
> -	struct xfs_buf_map	*map,
> -	xfs_buf_flags_t		flags,
> -	struct xfs_buf		**bpp)
> +	struct xfs_buf_map	*map)
>  {
>  	struct xfs_buf          *bp;
> -	int			error;
>  
>  	rcu_read_lock();
>  	bp = rhashtable_lookup(&btp->bt_hash, map, xfs_buf_hash_params);
>  	if (!bp || !lockref_get_not_dead(&bp->b_lockref)) {
>  		rcu_read_unlock();
> -		return -ENOENT;
> +		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
> +		return NULL;
>  	}
>  	rcu_read_unlock();
>  
> -	error = xfs_buf_find_lock(bp, flags);
> -	if (error) {
> -		xfs_buf_rele(bp);
> -		return error;
> -	}
> -
> -	trace_xfs_buf_find(bp, flags, _RET_IP_);
> -	*bpp = bp;
> -	return 0;
> +	trace_xfs_buf_find(bp, _RET_IP_);
> +	XFS_STATS_INC(btp->bt_mount, xb_get_locked);
> +	return bp;
>  }
>  
>  /*
> @@ -509,11 +494,7 @@ xfs_buf_find_insert(
>  			goto retry;
>  		}
>  		rcu_read_unlock();
> -		error = xfs_buf_find_lock(bp, flags);
> -		if (error)
> -			xfs_buf_rele(bp);
> -		else
> -			*bpp = bp;
> +		*bpp = bp;
>  		goto out_free_buf;
>  	}
>  	rcu_read_unlock();
> @@ -555,21 +536,20 @@ xfs_buf_get_map(
>  	if (error)
>  		return error;
>  
> -	error = xfs_buf_lookup(btp, &cmap, flags, &bp);
> -	if (error && error != -ENOENT)
> -		return error;
> -
>  	/* cache hits always outnumber misses by at least 10:1 */
> +	bp = xfs_buf_lookup(btp, &cmap);
>  	if (unlikely(!bp)) {
> -		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
> -
>  		if (flags & XBF_INCORE)
> -			return 0;
> +			return -ENOENT;
>  		error = xfs_buf_find_insert(btp, &cmap, map, nmaps, flags, &bp);
>  		if (error)
>  			return error;
> -	} else {
> -		XFS_STATS_INC(btp->bt_mount, xb_get_locked);
> +	}
> +
> +	error = xfs_buf_find_lock(bp, flags);
> +	if (error) {
> +		xfs_buf_rele(bp);
> +		return error;
>  	}
>  
>  	/*
> @@ -794,9 +774,11 @@ xfs_buf_get_uncached(
>  	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
>  
>  	error = xfs_buf_alloc(target, &map, 1, 0, bpp);
> -	if (!error)
> -		trace_xfs_buf_get_uncached(*bpp, _RET_IP_);
> -	return error;
> +	if (error)
> +		return error;
> +	xfs_buf_lock(*bpp);
> +	trace_xfs_buf_get_uncached(*bpp, _RET_IP_);
> +	return 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index aeb89ac53bf1..f333c938fbd9 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -792,6 +792,7 @@ DEFINE_BUF_EVENT(xfs_buf_backing_folio);
>  DEFINE_BUF_EVENT(xfs_buf_backing_kmem);
>  DEFINE_BUF_EVENT(xfs_buf_backing_vmalloc);
>  DEFINE_BUF_EVENT(xfs_buf_backing_fallback);
> +DEFINE_BUF_EVENT(xfs_buf_find);
>  
>  /* not really buffer traces, but the buf provides useful information */
>  DEFINE_BUF_EVENT(xfs_btree_corrupt);
> @@ -837,7 +838,6 @@ DECLARE_EVENT_CLASS(xfs_buf_flags_class,
>  DEFINE_EVENT(xfs_buf_flags_class, name, \
>  	TP_PROTO(struct xfs_buf *bp, unsigned flags, unsigned long caller_ip), \
>  	TP_ARGS(bp, flags, caller_ip))
> -DEFINE_BUF_FLAGS_EVENT(xfs_buf_find);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_get);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_read);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_readahead);
> -- 
> 2.53.0
> 
> 

[PATCH 03/12] xfs: split out a lower-level xfs_buf_get_map helper from xfs_find_get_buf

[Christoph Hellwig]

xfs_buf_get_map is currently reused to implement xfs_buf_read_map and
xfs_buf_readahead_map.  This causes double accounting of buf_get stat
and leads to some ugly overload of the flags.

Split out a slightly lower-level xfs_find_get_buf helper and use that to
implement xfs_buf_get_map, xfs_buf_read_map and xfs_buf_readahead_map.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index e56d4b8b0771..2cf359b4c446 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -514,8 +514,8 @@ xfs_buf_find_insert(
  * cache hits, as metadata intensive workloads will see 3 orders of magnitude
  * more hits than misses.
  */
-int
-xfs_buf_get_map(
+static int
+xfs_find_get_buf(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map,
 	int			nmaps,
@@ -552,16 +552,33 @@ xfs_buf_get_map(
 		return error;
 	}
 
+	*bpp = bp;
+	return 0;
+}
+
+int
+xfs_buf_get_map(
+	struct xfs_buftarg	*btp,
+	struct xfs_buf_map	*map,
+	int			nmaps,
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp)
+{
+	int			error;
+
+	ASSERT(!(flags & ~(XBF_TRYLOCK | XBF_INCORE | XBF_LIVESCAN)));
+	ASSERT(!(flags & XBF_LIVESCAN) || (flags & XBF_INCORE));
+
 	/*
-	 * Clear b_error if this is a lookup from a caller that doesn't expect
-	 * valid data to be found in the buffer.
+	 * Zero the buffer and clear b_error as xfs_buf_get_map callers don't
+	 * expect valid data to be found in the buffer.
 	 */
-	if (!(flags & XBF_READ))
-		xfs_buf_ioerror(bp, 0);
-
+	error = xfs_find_get_buf(btp, map, nmaps, flags, bpp);
+	if (error)
+		return error;
 	XFS_STATS_INC(btp->bt_mount, xb_get);
-	trace_xfs_buf_get(bp, flags, _RET_IP_);
-	*bpp = bp;
+	trace_xfs_buf_get(*bpp, flags, _RET_IP_);
+	xfs_buf_ioerror(*bpp, 0);
 	return 0;
 }
 
@@ -625,12 +642,12 @@ xfs_buf_read_map(
 	struct xfs_buf		*bp;
 	int			error;
 
-	ASSERT(!(flags & (XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD)));
+	ASSERT(!(flags & ~XBF_TRYLOCK));
 
 	flags |= XBF_READ;
 	*bpp = NULL;
 
-	error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
+	error = xfs_find_get_buf(target, map, nmaps, flags, &bp);
 	if (error)
 		return error;
 
@@ -706,7 +723,7 @@ xfs_buf_readahead_map(
 	if (xfs_buftarg_is_mem(target))
 		return;
 
-	if (xfs_buf_get_map(target, map, nmaps, flags | XBF_TRYLOCK, &bp))
+	if (xfs_find_get_buf(target, map, nmaps, flags | XBF_TRYLOCK, &bp))
 		return;
 	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
 
-- 
2.53.0

[PATCH 04/12] xfs: remove spurious XBF_DONE clearing on readahead validation failure

[Christoph Hellwig]

Both callers of ->verify_read already do this, so don't duplicate the
flag manipulation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dquot_buf.c |  8 +++-----
 fs/xfs/libxfs/xfs_inode_buf.c | 15 ++++++++-------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index bbada0d3cc08..4736f66e2a9a 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -252,8 +252,8 @@ xfs_dquot_buf_read_verify(
 /*
  * readahead errors are silent and simply leave the buffer as !done so a real
  * read will then be run with the xfs_dquot_buf_ops verifier. See
- * xfs_inode_buf_verify() for why we use EIO and ~XBF_DONE here rather than
- * reporting the failure.
+ * xfs_inode_buf_verify() for why we use EIO here rather than reporting the
+ * failure.
  */
 static void
 xfs_dquot_buf_readahead_verify(
@@ -262,10 +262,8 @@ xfs_dquot_buf_readahead_verify(
 	struct xfs_mount	*mp = bp->b_mount;
 
 	if (!xfs_dquot_buf_verify_crc(mp, bp, true) ||
-	    xfs_dquot_buf_verify(mp, bp, true) != NULL) {
+	    xfs_dquot_buf_verify(mp, bp, true) != NULL)
 		xfs_buf_ioerror(bp, -EIO);
-		bp->b_flags &= ~XBF_DONE;
-	}
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 336ef843f2fe..e4c3f7b24e95 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -29,12 +29,14 @@
  * has not had the inode cores stamped into it. Hence for readahead, the buffer
  * may be potentially invalid.
  *
- * If the readahead buffer is invalid, we need to mark it with an error and
- * clear the DONE status of the buffer so that a followup read will re-read it
- * from disk. We don't report the error otherwise to avoid warnings during log
- * recovery and we don't get unnecessary panics on debug kernels. We use EIO here
- * because all we want to do is say readahead failed; there is no-one to report
- * the error to, so this will distinguish it from a non-ra verifier failure.
+ * If the readahead buffer is invalid, we need to mark it with an error so that a
+ * followup read will re-read it from disk.
+ *
+ * We don't report the error otherwise to avoid warnings during log recovery and
+ * we don't get unnecessary panics on debug kernels.  Use EIO here because all
+ * we want to do is say readahead failed; there is no-one to report the error
+ * to, so this will distinguish it from a non-ra verifier failure.
+ *
  * Changes to this readahead error behaviour also need to be reflected in
  * xfs_dquot_buf_readahead_verify().
  */
@@ -64,7 +66,6 @@ xfs_inode_buf_verify(
 		if (unlikely(!di_ok ||
 				XFS_TEST_ERROR(mp, XFS_ERRTAG_ITOBP_INOTOBP))) {
 			if (readahead) {
-				bp->b_flags &= ~XBF_DONE;
 				xfs_buf_ioerror(bp, -EIO);
 				return;
 			}
-- 
2.53.0

[PATCH 05/12] xfs: remove _XBF_LOGRECOVERY

[Christoph Hellwig]

Adding _XBF_LOGRECOVERY to every buffer write from log recovery is error
prone.  Instead key off the behavior on log recovery being active with
indirecting that through a flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c                |  5 ++---
 fs/xfs/xfs_buf.h                |  4 ----
 fs/xfs/xfs_buf_item.c           | 10 ++++++----
 fs/xfs/xfs_buf_item_recover.c   |  3 ---
 fs/xfs/xfs_dquot_item_recover.c |  1 -
 fs/xfs/xfs_inode_item_recover.c |  1 -
 fs/xfs/xfs_log_recover.c        |  5 ++---
 7 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2cf359b4c446..d0146d4fb3d3 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1017,7 +1017,7 @@ xfs_buf_ioend_handle_error(
 	 * We're not going to bother about retrying this during recovery.
 	 * One strike!
 	 */
-	if (bp->b_flags & _XBF_LOGRECOVERY) {
+	if (mp->m_log && xlog_in_recovery(mp->m_log)) {
 		xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
 		return false;
 	}
@@ -1124,8 +1124,7 @@ xfs_buf_ioend(
 			bp->b_iodone(bp);
 	}
 
-	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD |
-			 _XBF_LOGRECOVERY);
+	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
 	if (async)
 		xfs_buf_relse(bp);
 }
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 79cc9c3f0254..e440f97cf3e1 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -34,9 +34,6 @@ struct xfs_buf;
 #define XBF_STALE	 (1u << 6) /* buffer has been staled, do not find it */
 #define XBF_WRITE_FAIL	 (1u << 7) /* async writes have failed on this buffer */
 
-/* buffer type flags for write callbacks */
-#define _XBF_LOGRECOVERY (1u << 18)/* log recovery buffer */
-
 /* flags used only internally */
 #define _XBF_KMEM	 (1u << 21)/* backed by heap memory */
 #define _XBF_DELWRI_Q	 (1u << 22)/* buffer on a delwri queue */
@@ -61,7 +58,6 @@ typedef unsigned int xfs_buf_flags_t;
 	{ XBF_DONE,		"DONE" }, \
 	{ XBF_STALE,		"STALE" }, \
 	{ XBF_WRITE_FAIL,	"WRITE_FAIL" }, \
-	{ _XBF_LOGRECOVERY,	"LOG_RECOVERY" }, \
 	{ _XBF_KMEM,		"KMEM" }, \
 	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
 	/* The following interface flags should never be set */ \
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 1f055cd6732e..1a4ef34af8d5 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1066,6 +1066,8 @@ void
 xfs_buf_item_done(
 	struct xfs_buf		*bp)
 {
+	struct xfs_buf_log_item	*bip = bp->b_log_item;
+
 	/*
 	 * If we are forcibly shutting down, this may well be off the AIL
 	 * already. That's because we simulate the log-committed callbacks to
@@ -1078,8 +1080,8 @@ xfs_buf_item_done(
 	 * Note that log recovery writes might have buffer items that are not on
 	 * the AIL even when the file system is not shut down.
 	 */
-	xfs_trans_ail_delete(&bp->b_log_item->bli_item,
-			     (bp->b_flags & _XBF_LOGRECOVERY) ? 0 :
-			     SHUTDOWN_CORRUPT_INCORE);
-	xfs_buf_item_relse(bp->b_log_item);
+	xfs_trans_ail_delete(&bip->bli_item,
+			     xlog_in_recovery(bip->bli_item.li_log) ?
+			     0 : SHUTDOWN_CORRUPT_INCORE);
+	xfs_buf_item_relse(bip);
 }
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index 02b95b89d1b5..0a3ab33b0e74 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -448,7 +448,6 @@ xlog_recover_validate_buf_type(
 	if (bp->b_ops) {
 		struct xfs_buf_log_item	*bip;
 
-		bp->b_flags |= _XBF_LOGRECOVERY;
 		xfs_buf_item_init(bp, mp);
 		bip = bp->b_log_item;
 		bip->bli_item.li_lsn = current_lsn;
@@ -1100,7 +1099,6 @@ xlog_recover_buf_commit_pass2(
 			xfs_buf_lock(rtsb_bp);
 			xfs_buf_hold(rtsb_bp);
 			xfs_update_rtsb(rtsb_bp, bp);
-			rtsb_bp->b_flags |= _XBF_LOGRECOVERY;
 			xfs_buf_delwri_queue(rtsb_bp, buffer_list);
 			xfs_buf_relse(rtsb_bp);
 		}
@@ -1139,7 +1137,6 @@ xlog_recover_buf_commit_pass2(
 		error = xfs_bwrite(bp);
 	} else {
 		ASSERT(bp->b_mount == mp);
-		bp->b_flags |= _XBF_LOGRECOVERY;
 		xfs_buf_delwri_queue(bp, buffer_list);
 	}
 
diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c
index fe419b28de22..d4dfe1885666 100644
--- a/fs/xfs/xfs_dquot_item_recover.c
+++ b/fs/xfs/xfs_dquot_item_recover.c
@@ -168,7 +168,6 @@ xlog_recover_dquot_commit_pass2(
 
 	ASSERT(dq_f->qlf_size == 2);
 	ASSERT(bp->b_mount == mp);
-	bp->b_flags |= _XBF_LOGRECOVERY;
 	xfs_buf_delwri_queue(bp, buffer_list);
 
 out_release:
diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
index 169a8fe3bf0a..1d2319ad15c5 100644
--- a/fs/xfs/xfs_inode_item_recover.c
+++ b/fs/xfs/xfs_inode_item_recover.c
@@ -586,7 +586,6 @@ xlog_recover_inode_commit_pass2(
 	}
 
 	ASSERT(bp->b_mount == mp);
-	bp->b_flags |= _XBF_LOGRECOVERY;
 	xfs_buf_delwri_queue(bp, buffer_list);
 
 out_release:
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index fdb011e6ef60..e7e49529658b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3279,9 +3279,8 @@ xlog_do_recovery_pass(
 			 * checkpoints at this start LSN.
 			 *
 			 * Note: Shutting down the filesystem will result in the
-			 * delwri submission marking all the buffers stale,
-			 * completing them and cleaning up _XBF_LOGRECOVERY
-			 * state without doing any IO.
+			 * delwri submission marking all the buffers stale and
+			 * completing them without doing any IO.
 			 */
 			xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
 		}
-- 
2.53.0

[PATCH 06/12] xfs: hide b_flags manipulation from code outside of xfs_buf.c

[Christoph Hellwig]

Add helpers for the remaining buffer flags manipulation not done in the
core buffer cache code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_btree_staging.c |  7 +++----
 fs/xfs/libxfs/xfs_ialloc.c        |  2 +-
 fs/xfs/xfs_buf.c                  | 16 ++++++++++++++++
 fs/xfs/xfs_buf.h                  |  4 +++-
 fs/xfs/xfs_fsops.c                |  2 +-
 fs/xfs/xfs_inode.c                |  4 ++--
 fs/xfs/xfs_trans_buf.c            |  7 +++----
 7 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree_staging.c b/fs/xfs/libxfs/xfs_btree_staging.c
index c3c7ea54895a..7314dab4bcfb 100644
--- a/fs/xfs/libxfs/xfs_btree_staging.c
+++ b/fs/xfs/libxfs/xfs_btree_staging.c
@@ -248,11 +248,10 @@ xfs_btree_bload_drop_buf(
 		return 0;
 
 	/*
-	 * Mark this buffer XBF_DONE (i.e. uptodate) so that a subsequent
-	 * xfs_buf_read will not pointlessly reread the contents from the disk.
+	 * Mark this buffer uptodate so that a subsequent xfs_buf_read will
+	 * not pointlessly reread the contents from the disk.
 	 */
-	bp->b_flags |= XBF_DONE;
-
+	xfs_buf_set_uptodate(bp);
 	xfs_buf_delwri_queue_here(bp, buffers_list);
 	xfs_buf_relse(bp);
 	*bpp = NULL;
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index ffcdd1f691fd..58dac4d505ba 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -413,7 +413,7 @@ xfs_ialloc_inode_init(
 				xfs_trans_ordered_buf(tp, fbuf);
 			}
 		} else {
-			fbuf->b_flags |= XBF_DONE;
+			xfs_buf_set_uptodate(fbuf);
 			xfs_buf_delwri_queue(fbuf, buffer_list);
 			xfs_buf_relse(fbuf);
 		}
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index d0146d4fb3d3..33e904d838f4 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -55,6 +55,13 @@ static inline bool xfs_buf_is_uncached(struct xfs_buf *bp)
 	return bp->b_rhash_key == XFS_BUF_DADDR_NULL;
 }
 
+void
+xfs_buf_set_uptodate(
+	struct xfs_buf	*bp)
+{
+	bp->b_flags |= XBF_DONE;
+}
+
 /*
  * When we mark a buffer stale, we remove the buffer from the LRU and clear the
  * b_lru_ref count so that the buffer is freed immediately when the buffer
@@ -85,6 +92,15 @@ xfs_buf_stale(
 	spin_unlock(&bp->b_lockref.lock);
 }
 
+void
+xfs_buf_clear_stale(
+	struct xfs_buf	*bp)
+{
+	ASSERT(bp->b_flags & XBF_STALE);
+
+	bp->b_flags &= ~XBF_STALE;
+}
+
 static void
 xfs_buf_free_callback(
 	struct callback_head	*cb)
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index e440f97cf3e1..a4729253b56f 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -301,7 +301,9 @@ static inline void xfs_buf_zero(struct xfs_buf *bp, size_t boff, size_t bsize)
 	memset(bp->b_addr + boff, 0, bsize);
 }
 
-extern void xfs_buf_stale(struct xfs_buf *bp);
+void xfs_buf_set_uptodate(struct xfs_buf *bp);
+void xfs_buf_stale(struct xfs_buf *bp);
+void xfs_buf_clear_stale(struct xfs_buf *bp);
 
 /* Delayed Write Buffer Routines */
 extern void xfs_buf_delwri_cancel(struct list_head *);
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 67624a804a7f..21114bb6d4ff 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -501,7 +501,7 @@ xfs_do_force_shutdown(
 		return;
 	}
 	if (mp->m_sb_bp)
-		mp->m_sb_bp->b_flags |= XBF_DONE;
+		xfs_buf_set_uptodate(mp->m_sb_bp);
 
 	if (flags & SHUTDOWN_FORCE_UMOUNT)
 		xfs_alert(mp, "User initiated shutdown received.");
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 15279d22a894..030a7c8f2c12 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1753,7 +1753,7 @@ xfs_ifree_cluster(
 		 * attachment may occur in xfs_inode_item_precommit() after we
 		 * have marked this buffer stale.  If this buffer was not in
 		 * memory before xfs_ifree_cluster() started, it will not be
-		 * marked XBF_DONE and this will cause problems later in
+		 * marked uptodate and this will cause problems later in
 		 * xfs_inode_item_precommit() when we trip over a (stale, !done)
 		 * buffer to attached to the transaction.
 		 *
@@ -1766,7 +1766,7 @@ xfs_ifree_cluster(
 		 * fail. We can acheive this by adding a write verifier to the
 		 * buffer.
 		 */
-		bp->b_flags |= XBF_DONE;
+		xfs_buf_set_uptodate(bp);
 		bp->b_ops = &xfs_inode_buf_ops;
 
 		/*
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 7e17b93fe9ad..1e025848811a 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -140,7 +140,7 @@ xfs_trans_get_buf_map(
 		ASSERT(xfs_buf_islocked(bp));
 		if (xfs_is_shutdown(tp->t_mountp)) {
 			xfs_buf_stale(bp);
-			bp->b_flags |= XBF_DONE;
+			xfs_buf_set_uptodate(bp);
 		}
 
 		ASSERT(bp->b_transp == tp);
@@ -482,7 +482,7 @@ xfs_trans_dirty_buf(
 	 * item from the AIL and free it when the buffer is flushed
 	 * to disk.
 	 */
-	bp->b_flags |= XBF_DONE;
+	xfs_buf_set_uptodate(bp);
 
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 
@@ -494,8 +494,7 @@ xfs_trans_dirty_buf(
 	 */
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		bip->bli_flags &= ~XFS_BLI_STALE;
-		ASSERT(bp->b_flags & XBF_STALE);
-		bp->b_flags &= ~XBF_STALE;
+		xfs_buf_clear_stale(bp);
 		bip->__bli_format.blf_flags &= ~XFS_BLF_CANCEL;
 	}
 	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
-- 
2.53.0

[PATCH 07/12] xfs: use WRITE_ONCE to update b_flags

[Christoph Hellwig]

Prepare for limited lockless reading of flags by using WRITE_ONCE to
prevent the compiler from doing non-standard read-modify-write
operations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 86 ++++++++++++++++++++++++++++--------------------
 1 file changed, 51 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 33e904d838f4..2a0a19172847 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -55,11 +55,27 @@ static inline bool xfs_buf_is_uncached(struct xfs_buf *bp)
 	return bp->b_rhash_key == XFS_BUF_DADDR_NULL;
 }
 
+static inline void
+xfs_buf_set_flags(
+	struct xfs_buf	*bp,
+	unsigned int	flags)
+{
+	WRITE_ONCE(bp->b_flags, bp->b_flags | flags);
+}
+
+static inline void
+xfs_buf_clear_flags(
+	struct xfs_buf	*bp,
+	unsigned int	flags)
+{
+	WRITE_ONCE(bp->b_flags, bp->b_flags & ~flags);
+}
+
 void
 xfs_buf_set_uptodate(
 	struct xfs_buf	*bp)
 {
-	bp->b_flags |= XBF_DONE;
+	xfs_buf_set_flags(bp, XBF_DONE);
 }
 
 /*
@@ -76,14 +92,14 @@ xfs_buf_stale(
 {
 	ASSERT(xfs_buf_islocked(bp));
 
-	bp->b_flags |= XBF_STALE;
+	xfs_buf_set_flags(bp, XBF_STALE);
 
 	/*
 	 * Clear the delwri status so that a delwri queue walker will not
 	 * flush this buffer to disk now that it is stale. The delwri queue has
 	 * a reference to the buffer, so this is safe to do.
 	 */
-	bp->b_flags &= ~_XBF_DELWRI_Q;
+	xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
 
 	spin_lock(&bp->b_lockref.lock);
 	atomic_set(&bp->b_lru_ref, 0);
@@ -97,8 +113,7 @@ xfs_buf_clear_stale(
 	struct xfs_buf	*bp)
 {
 	ASSERT(bp->b_flags & XBF_STALE);
-
-	bp->b_flags &= ~XBF_STALE;
+	xfs_buf_clear_flags(bp, XBF_STALE);
 }
 
 static void
@@ -175,7 +190,7 @@ xfs_buf_alloc_kmem(
 		bp->b_addr = NULL;
 		return -ENOMEM;
 	}
-	bp->b_flags |= _XBF_KMEM;
+	xfs_buf_set_flags(bp, _XBF_KMEM);
 	trace_xfs_buf_backing_kmem(bp, _RET_IP_);
 	return 0;
 }
@@ -307,7 +322,7 @@ xfs_buf_alloc(
 	INIT_LIST_HEAD(&bp->b_li_list);
 	bp->b_target = target;
 	bp->b_mount = target->bt_mount;
-	bp->b_flags = flags;
+	WRITE_ONCE(bp->b_flags, flags);
 	bp->b_rhash_key = map[0].bm_bn;
 	bp->b_length = 0;
 	bp->b_map_count = nmaps;
@@ -436,7 +451,7 @@ xfs_buf_find_lock(
 			return -ENOENT;
 		}
 		ASSERT((bp->b_flags & _XBF_DELWRI_Q) == 0);
-		bp->b_flags &= _XBF_KMEM;
+		xfs_buf_clear_flags(bp, ~_XBF_KMEM);
 		bp->b_ops = NULL;
 	}
 	return 0;
@@ -604,8 +619,9 @@ _xfs_buf_read(
 {
 	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
 
-	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD | XBF_DONE);
-	bp->b_flags |= XBF_READ;
+	xfs_buf_clear_flags(bp, XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD |
+			XBF_DONE);
+	xfs_buf_set_flags(bp, XBF_READ);
 	xfs_buf_submit(bp);
 	return xfs_buf_iowait(bp);
 }
@@ -641,7 +657,7 @@ xfs_buf_reverify(
 	bp->b_ops = ops;
 	bp->b_ops->verify_read(bp);
 	if (bp->b_error)
-		bp->b_flags &= ~XBF_DONE;
+		xfs_buf_clear_flags(bp, XBF_DONE);
 	return bp->b_error;
 }
 
@@ -679,7 +695,7 @@ xfs_buf_read_map(
 		error = xfs_buf_reverify(bp, ops);
 
 		/* We do not want read in the flags */
-		bp->b_flags &= ~XBF_READ;
+		xfs_buf_clear_flags(bp, XBF_READ);
 		ASSERT(bp->b_ops != NULL || ops == NULL);
 	}
 
@@ -704,7 +720,7 @@ xfs_buf_read_map(
 		if (!xlog_is_shutdown(target->bt_mount->m_log))
 			xfs_buf_ioerror_alert(bp, fa);
 
-		bp->b_flags &= ~XBF_DONE;
+		xfs_buf_clear_flags(bp, XBF_DONE);
 		xfs_buf_stale(bp);
 		xfs_buf_relse(bp);
 
@@ -750,8 +766,8 @@ xfs_buf_readahead_map(
 	}
 	XFS_STATS_INC(target->bt_mount, xb_get_read);
 	bp->b_ops = ops;
-	bp->b_flags &= ~(XBF_WRITE | XBF_DONE);
-	bp->b_flags |= flags;
+	xfs_buf_clear_flags(bp, XBF_WRITE | XBF_DONE);
+	xfs_buf_set_flags(bp, flags);
 	percpu_counter_inc(&target->bt_readahead_count);
 	xfs_buf_submit(bp);
 }
@@ -783,7 +799,7 @@ xfs_buf_read_uncached(
 	ASSERT(bp->b_map_count == 1);
 	bp->b_rhash_key = XFS_BUF_DADDR_NULL;
 	bp->b_maps[0].bm_bn = daddr;
-	bp->b_flags |= XBF_READ;
+	xfs_buf_set_flags(bp, XBF_READ);
 	bp->b_ops = ops;
 
 	xfs_buf_submit(bp);
@@ -1077,14 +1093,14 @@ xfs_buf_ioend_handle_error(
 
 resubmit:
 	xfs_buf_ioerror(bp, 0);
-	bp->b_flags |= (XBF_DONE | XBF_WRITE_FAIL);
+	xfs_buf_set_flags(bp, XBF_DONE | XBF_WRITE_FAIL);
 	reinit_completion(&bp->b_iowait);
 	xfs_buf_submit(bp);
 	return true;
 out_stale:
 	xfs_buf_stale(bp);
-	bp->b_flags |= XBF_DONE;
-	bp->b_flags &= ~XBF_WRITE;
+	xfs_buf_set_flags(bp, XBF_DONE);
+	xfs_buf_clear_flags(bp, XBF_WRITE);
 	trace_xfs_buf_error_relse(bp, _RET_IP_);
 	return false;
 }
@@ -1109,7 +1125,7 @@ xfs_buf_ioend(
 		if (!bp->b_error && bp->b_ops)
 			bp->b_ops->verify_read(bp);
 		if (!bp->b_error)
-			bp->b_flags |= XBF_DONE;
+			xfs_buf_set_flags(bp, XBF_DONE);
 		if (bp->b_flags & XBF_READ_AHEAD)
 			percpu_counter_dec(&bp->b_target->bt_readahead_count);
 	} else {
@@ -1119,8 +1135,8 @@ xfs_buf_ioend(
 				return;
 			}
 		} else {
-			bp->b_flags &= ~XBF_WRITE_FAIL;
-			bp->b_flags |= XBF_DONE;
+			xfs_buf_clear_flags(bp, XBF_WRITE_FAIL);
+			xfs_buf_set_flags(bp, XBF_DONE);
 		}
 
 		/* clear the retry state */
@@ -1140,7 +1156,7 @@ xfs_buf_ioend(
 			bp->b_iodone(bp);
 	}
 
-	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
+	xfs_buf_clear_flags(bp, XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
 	if (async)
 		xfs_buf_relse(bp);
 }
@@ -1186,8 +1202,8 @@ xfs_buf_fail(
 {
 	ASSERT(xfs_buf_islocked(bp));
 
-	bp->b_flags |= XBF_ASYNC;
-	bp->b_flags &= ~XBF_DONE;
+	xfs_buf_set_flags(bp, XBF_ASYNC);
+	xfs_buf_clear_flags(bp, XBF_DONE);
 	xfs_buf_stale(bp);
 	xfs_buf_ioerror(bp, -EIO);
 	xfs_buf_ioend(bp);
@@ -1201,9 +1217,9 @@ xfs_bwrite(
 
 	ASSERT(xfs_buf_islocked(bp));
 
-	bp->b_flags |= XBF_WRITE;
-	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
-			 XBF_DONE);
+	xfs_buf_set_flags(bp, XBF_WRITE);
+	xfs_buf_clear_flags(bp, XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
+				XBF_DONE);
 
 	xfs_buf_submit(bp);
 	error = xfs_buf_iowait(bp);
@@ -1394,7 +1410,7 @@ xfs_buf_submit(
 	return;
 
 ioerror:
-	bp->b_flags &= ~XBF_DONE;
+	xfs_buf_clear_flags(bp, XBF_DONE);
 	xfs_buf_stale(bp);
 end_io:
 	if (bp->b_flags & XBF_ASYNC)
@@ -1805,7 +1821,7 @@ xfs_buf_delwri_cancel(
 		bp = list_first_entry(list, struct xfs_buf, b_list);
 
 		xfs_buf_lock(bp);
-		bp->b_flags &= ~_XBF_DELWRI_Q;
+		xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
 		xfs_buf_list_del(bp);
 		xfs_buf_relse(bp);
 	}
@@ -1850,7 +1866,7 @@ xfs_buf_delwri_queue(
 	 * might get readded to a delwri list after the synchronous writeout, in
 	 * which case we need just need to re-add the flag here.
 	 */
-	bp->b_flags |= _XBF_DELWRI_Q;
+	xfs_buf_set_flags(bp, _XBF_DELWRI_Q);
 	if (list_empty(&bp->b_list)) {
 		xfs_buf_hold(bp);
 		list_add_tail(&bp->b_list, list);
@@ -1927,8 +1943,8 @@ xfs_buf_delwri_submit_prep(
 	}
 
 	trace_xfs_buf_delwri_split(bp, _RET_IP_);
-	bp->b_flags &= ~_XBF_DELWRI_Q;
-	bp->b_flags |= XBF_WRITE;
+	xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
+	xfs_buf_set_flags(bp, XBF_WRITE);
 	return true;
 }
 
@@ -1969,7 +1985,7 @@ xfs_buf_delwri_submit_nowait(
 		}
 		if (!xfs_buf_delwri_submit_prep(bp))
 			continue;
-		bp->b_flags |= XBF_ASYNC;
+		xfs_buf_set_flags(bp, XBF_ASYNC);
 		xfs_buf_list_del(bp);
 		xfs_buf_submit(bp);
 	}
@@ -2002,7 +2018,7 @@ xfs_buf_delwri_submit(
 		xfs_buf_lock(bp);
 		if (!xfs_buf_delwri_submit_prep(bp))
 			continue;
-		bp->b_flags &= ~XBF_ASYNC;
+		xfs_buf_clear_flags(bp, XBF_ASYNC);
 		list_move_tail(&bp->b_list, &wait_list);
 		xfs_buf_submit(bp);
 	}
-- 
2.53.0

[PATCH 08/12] xfs: don't reverify buffers in xfs_buf_readahead_map

[Christoph Hellwig]

xfs_buf_read_map calls xfs_buf_reverify to ensure the verifier has run
for a buffer before the data can be used when an earlier readahead read
the data before the buf_ops were assigned.

There is no point in doing this in xfs_buf_readahead_map for a buffer
already in memory as a later xfs_buf_read will do the same and can
actually propagate the error to the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2a0a19172847..4430da3259f1 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -760,7 +760,6 @@ xfs_buf_readahead_map(
 	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
 
 	if (bp->b_flags & XBF_DONE) {
-		xfs_buf_reverify(bp, ops);
 		xfs_buf_relse(bp);
 		return;
 	}
-- 
2.53.0

[PATCH 09/12] xfs: use goto based error unwinding in xfs_buf_read_map

[Christoph Hellwig]

This keeps the I/O error handling contained at the end of the function
and removes the indentation for it.  It also allows to reorder the
comments so that they are closer to the logic that they describe.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4430da3259f1..21c21491d72e 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -699,6 +699,23 @@ xfs_buf_read_map(
 		ASSERT(bp->b_ops != NULL || ops == NULL);
 	}
 
+	if (error)
+		goto out_ioerror;
+
+	*bpp = bp;
+	return 0;
+
+out_ioerror:
+	/*
+	 * Check against log shutdown for error reporting because metadata
+	 * writeback may require a read first and we need to report errors in
+	 * metadata writeback until the log is shut down.  High level
+	 * transaction read functions already check against mount shutdown, so
+	 * we only need to be concerned about low level/ IO interactions here.
+	 */
+	if (!xlog_is_shutdown(target->bt_mount->m_log))
+		xfs_buf_ioerror_alert(bp, fa);
+
 	/*
 	 * If we've had a read error, then the contents of the buffer are
 	 * invalid and should not be used. To ensure that a followup read tries
@@ -708,30 +725,14 @@ xfs_buf_read_map(
 	 * future cache lookups will also treat it as an empty, uninitialised
 	 * buffer.
 	 */
-	if (error) {
-		/*
-		 * Check against log shutdown for error reporting because
-		 * metadata writeback may require a read first and we need to
-		 * report errors in metadata writeback until the log is shut
-		 * down. High level transaction read functions already check
-		 * against mount shutdown, anyway, so we only need to be
-		 * concerned about low level IO interactions here.
-		 */
-		if (!xlog_is_shutdown(target->bt_mount->m_log))
-			xfs_buf_ioerror_alert(bp, fa);
-
-		xfs_buf_clear_flags(bp, XBF_DONE);
-		xfs_buf_stale(bp);
-		xfs_buf_relse(bp);
-
-		/* bad CRC means corrupted metadata */
-		if (error == -EFSBADCRC)
-			error = -EFSCORRUPTED;
-		return error;
-	}
+	xfs_buf_clear_flags(bp, XBF_DONE);
+	xfs_buf_stale(bp);
+	xfs_buf_relse(bp);
 
-	*bpp = bp;
-	return 0;
+	/* bad CRC means corrupted metadata */
+	if (error == -EFSBADCRC)
+		return -EFSCORRUPTED;
+	return error;
 }
 
 /*
-- 
2.53.0

[PATCH 10/12] xfs: merge xfs_buf_reverify into xfs_buf_read_map

[Christoph Hellwig]

xfs_buf_read_map is the only caller of xfs_buf_reverify that is left.
Merge it into that so that the comments can be moved closer to the
logic, and redundant asserts can be removed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 79 ++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 43 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 21c21491d72e..be3b25ccbea6 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -626,41 +626,6 @@ _xfs_buf_read(
 	return xfs_buf_iowait(bp);
 }
 
-/*
- * Reverify a buffer found in cache without an attached ->b_ops.
- *
- * If the caller passed an ops structure and the buffer doesn't have ops
- * assigned, set the ops and use it to verify the contents. If verification
- * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
- * already in XBF_DONE state on entry.
- *
- * Under normal operations, every in-core buffer is verified on read I/O
- * completion. There are two scenarios that can lead to in-core buffers without
- * an assigned ->b_ops. The first is during log recovery of buffers on a V4
- * filesystem, though these buffers are purged at the end of recovery. The
- * other is online repair, which intentionally reads with a NULL buffer ops to
- * run several verifiers across an in-core buffer in order to establish buffer
- * type.  If repair can't establish that, the buffer will be left in memory
- * with NULL buffer ops.
- */
-static int
-xfs_buf_reverify(
-	struct xfs_buf		*bp,
-	const struct xfs_buf_ops *ops)
-{
-	ASSERT(bp->b_flags & XBF_DONE);
-	ASSERT(bp->b_error == 0);
-
-	if (!ops || bp->b_ops)
-		return 0;
-
-	bp->b_ops = ops;
-	bp->b_ops->verify_read(bp);
-	if (bp->b_error)
-		xfs_buf_clear_flags(bp, XBF_DONE);
-	return bp->b_error;
-}
-
 int
 xfs_buf_read_map(
 	struct xfs_buftarg	*target,
@@ -685,18 +650,46 @@ xfs_buf_read_map(
 
 	trace_xfs_buf_read(bp, flags, _RET_IP_);
 
-	if (!(bp->b_flags & XBF_DONE)) {
+	if (bp->b_flags & XBF_DONE) {
+		ASSERT(bp->b_error == 0);
+
+		/*
+		 * If the caller passed an ops structure and the buffer doesn't
+		 * have ops assigned yet, set the ops and use them to verify the
+		 * buffer contents.
+		 *
+		 * Under normal operations, every in-core buffer is verified on
+		 * read I/O completion, but there are two scenarios that can
+		 * lead to in-core buffers without an assigned ->b_ops:
+		 *
+		 *   1) During log recovery of buffers on a V4 filesystem.
+		 *	These buffers are purged at the end of recovery, though.
+		 *   2) Oonline repair intentionally reads with a NULL buffer
+		 *	ops to run several verifiers across an in-core buffer in
+		 *	order to establish buffer type.  If repair can't
+		 *	establish that, the buffer will be left in memory with
+		 *	NULL buffer ops.
+		 */
+		if (ops && !bp->b_ops) {
+			bp->b_ops = ops;
+			bp->b_ops->verify_read(bp);
+			/*
+			 * If verification failed, clear XBF_DONE as we assume
+			 * that buffers have no recorded errors when in XBF_DONE
+			 * state.
+			 */
+			error = bp->b_error;
+			if (error)
+				xfs_buf_clear_flags(bp, XBF_DONE);
+		}
+
+		/* We do not want read in the flags */
+		xfs_buf_clear_flags(bp, XBF_READ);
+	} else {
 		/* Initiate the buffer read and wait. */
 		XFS_STATS_INC(target->bt_mount, xb_get_read);
 		bp->b_ops = ops;
 		error = _xfs_buf_read(bp);
-	} else {
-		/* Buffer already read; all we need to do is check it. */
-		error = xfs_buf_reverify(bp, ops);
-
-		/* We do not want read in the flags */
-		xfs_buf_clear_flags(bp, XBF_READ);
-		ASSERT(bp->b_ops != NULL || ops == NULL);
 	}
 
 	if (error)
-- 
2.53.0

[PATCH 11/12] xfs: move buffer locking out of xfs_find_get_buf

[Christoph Hellwig]

To prepare for buffer loookups that don't lock the buffer, move the
call to xfs_buf_find_lock from xfs_find_get_buf to its callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index be3b25ccbea6..f5701f74061f 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -577,12 +577,6 @@ xfs_find_get_buf(
 			return error;
 	}
 
-	error = xfs_buf_find_lock(bp, flags);
-	if (error) {
-		xfs_buf_rele(bp);
-		return error;
-	}
-
 	*bpp = bp;
 	return 0;
 }
@@ -607,6 +601,12 @@ xfs_buf_get_map(
 	error = xfs_find_get_buf(btp, map, nmaps, flags, bpp);
 	if (error)
 		return error;
+
+	error = xfs_buf_find_lock(*bpp, flags);
+	if (error) {
+		xfs_buf_rele(*bpp);
+		return error;
+	}
 	XFS_STATS_INC(btp->bt_mount, xb_get);
 	trace_xfs_buf_get(*bpp, flags, _RET_IP_);
 	xfs_buf_ioerror(*bpp, 0);
@@ -647,6 +647,11 @@ xfs_buf_read_map(
 	error = xfs_find_get_buf(target, map, nmaps, flags, &bp);
 	if (error)
 		return error;
+	error = xfs_buf_find_lock(bp, flags);
+	if (error) {
+		xfs_buf_rele(bp);
+		return error;
+	}
 
 	trace_xfs_buf_read(bp, flags, _RET_IP_);
 
@@ -749,20 +754,26 @@ xfs_buf_readahead_map(
 	if (xfs_buftarg_is_mem(target))
 		return;
 
-	if (xfs_find_get_buf(target, map, nmaps, flags | XBF_TRYLOCK, &bp))
+	if (xfs_find_get_buf(target, map, nmaps, flags, &bp))
 		return;
+	if (xfs_buf_find_lock(bp, XBF_TRYLOCK))
+		goto out_rele;
+
 	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
+	if (bp->b_flags & XBF_DONE)
+		goto out_unlock;
 
-	if (bp->b_flags & XBF_DONE) {
-		xfs_buf_relse(bp);
-		return;
-	}
 	XFS_STATS_INC(target->bt_mount, xb_get_read);
 	bp->b_ops = ops;
 	xfs_buf_clear_flags(bp, XBF_WRITE | XBF_DONE);
 	xfs_buf_set_flags(bp, flags);
 	percpu_counter_inc(&target->bt_readahead_count);
 	xfs_buf_submit(bp);
+	return;
+out_unlock:
+	xfs_buf_unlock(bp);
+out_rele:
+	xfs_buf_rele(bp);
 }
 
 /*
-- 
2.53.0

[PATCH 12/12] xfs: add lockless xfs_buf_readahead_map fast path

[Christoph Hellwig]

Readahead currently always locks the buffer, which can cause contention
with actual users of the buffer.  Add a fast path without taking any
locks if the buffer is uptodate and not stale.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index f5701f74061f..2e498e9e05e9 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -756,13 +756,23 @@ xfs_buf_readahead_map(
 
 	if (xfs_find_get_buf(target, map, nmaps, flags, &bp))
 		return;
-	if (xfs_buf_find_lock(bp, XBF_TRYLOCK))
+
+	/*
+	 * Do a lockless fast path check for a valid uptodate buffer and avoid
+	 * locking entirely in this case.
+	 */
+	if ((READ_ONCE(bp->b_flags) & (XBF_DONE | XBF_STALE)) == XBF_DONE)
 		goto out_rele;
 
-	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
-	if (bp->b_flags & XBF_DONE)
+	/* Otherwise lock the buffer to stabilize the state */
+	if (!xfs_buf_trylock(bp))
+		goto out_rele;
+
+	/* Let the actual reader deal with stale buffers. */
+	if (bp->b_flags & (XBF_STALE | XBF_DONE))
 		goto out_unlock;
 
+	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
 	XFS_STATS_INC(target->bt_mount, xb_get_read);
 	bp->b_ops = ops;
 	xfs_buf_clear_flags(bp, XBF_WRITE | XBF_DONE);
-- 
2.53.0