[PATCH 0/2] xfs: optimise XBF_TRYLOCK buffer lookups

[PATCH 0/2] xfs: optimise XBF_TRYLOCK buffer lookups

[Dave Chinner]

Hi folks,

This is a cleaned up version of my original patch to avoid
unnecessary buffer lookups on locked buffers when XBF_TRYLOCK is
set. That patch can be found here:

https://marc.info/?l=linux-xfs&m=151994388115521&w=2

Version 2:
- split out changes to xfs_incore interface to make _xfs_buf_find
  static and not externally visible
- fixed indenting issues in error handling switch statement

Cheers,

Dave.

[PATCH 1/2] xfs: make xfs_buf_incore out of line

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Move xfs_buf_incore out of line and make it the only way to look up
a buffer in the buffer cache from outside the buffer cache. Convert
the external users of _xfs_buf_find() to xfs_buf_incore() and make
_xfs_buf_find() static.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf.c | 21 ++++++++++++++++-----
 fs/xfs/xfs_buf.h | 17 +++--------------
 fs/xfs/xfs_qm.c  |  5 ++---
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 55661cbdb51b..2ca8e2c7fbc4 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -549,17 +549,17 @@ xfs_buf_hash_destroy(
 }
 
 /*
- *	Look up, and creates if absent, a lockable buffer for
- *	a given range of an inode.  The buffer is returned
- *	locked.	No I/O is implied by this call.
+ * Look up (and insert if absent), a lockable buffer for a given
+ * range of an inode.  The buffer is returned locked. No I/O is
+ * implied by this call.
  */
-xfs_buf_t *
+static struct xfs_buf *
 _xfs_buf_find(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map,
 	int			nmaps,
 	xfs_buf_flags_t		flags,
-	xfs_buf_t		*new_bp)
+	struct xfs_buf		*new_bp)
 {
 	struct xfs_perag	*pag;
 	xfs_buf_t		*bp;
@@ -649,6 +649,17 @@ _xfs_buf_find(
 	return bp;
 }
 
+struct xfs_buf *
+xfs_incore(
+	struct xfs_buftarg	*target,
+	xfs_daddr_t		blkno,
+	size_t			numblks,
+	xfs_buf_flags_t		flags)
+{
+	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	return _xfs_buf_find(target, &map, 1, flags, NULL);
+}
+
 /*
  * Assembles a buffer covering the specified range. The code is optimised for
  * cache hits, as metadata intensive workloads will see 3 orders of magnitude
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index edced162a674..e8947c1adac0 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -218,20 +218,9 @@ typedef struct xfs_buf {
 } xfs_buf_t;
 
 /* Finding and Reading Buffers */
-struct xfs_buf *_xfs_buf_find(struct xfs_buftarg *target,
-			      struct xfs_buf_map *map, int nmaps,
-			      xfs_buf_flags_t flags, struct xfs_buf *new_bp);
-
-static inline struct xfs_buf *
-xfs_incore(
-	struct xfs_buftarg	*target,
-	xfs_daddr_t		blkno,
-	size_t			numblks,
-	xfs_buf_flags_t		flags)
-{
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
-}
+struct xfs_buf *xfs_incore(struct xfs_buftarg *target,
+			   xfs_daddr_t blkno, size_t numblks,
+			   xfs_buf_flags_t flags);
 
 struct xfs_buf *_xfs_buf_alloc(struct xfs_buftarg *target,
 			       struct xfs_buf_map *map, int nmaps,
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ec39ae274c78..aff4924f637f 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1247,9 +1247,8 @@ xfs_qm_flush_one(
 	 */
 	if (!xfs_dqflock_nowait(dqp)) {
 		/* buf is pinned in-core by delwri list */
-		DEFINE_SINGLE_BUF_MAP(map, dqp->q_blkno,
-				      mp->m_quotainfo->qi_dqchunklen);
-		bp = _xfs_buf_find(mp->m_ddev_targp, &map, 1, 0, NULL);
+		bp = xfs_incore(mp->m_ddev_targp, dqp->q_blkno,
+				mp->m_quotainfo->qi_dqchunklen, 0);
 		if (!bp) {
 			error = -EINVAL;
 			goto out_unlock;
-- 
2.16.1

Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line

[Christoph Hellwig]

On Wed, Apr 18, 2018 at 10:21:10AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move xfs_buf_incore out of line and make it the only way to look up
> a buffer in the buffer cache from outside the buffer cache. Convert
> the external users of _xfs_buf_find() to xfs_buf_incore() and make
> _xfs_buf_find() static.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line

[Carlos Maiolino]

On Wed, Apr 18, 2018 at 10:21:10AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move xfs_buf_incore out of line and make it the only way to look up
> a buffer in the buffer cache from outside the buffer cache. Convert
> the external users of _xfs_buf_find() to xfs_buf_incore() and make
> _xfs_buf_find() static.
> 

Looks good, but shouldn't it be xfs_incore() instead of xfs_buf_incore() in the
Subject/description?

Otherwise

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_buf.c | 21 ++++++++++++++++-----
>  fs/xfs/xfs_buf.h | 17 +++--------------
>  fs/xfs/xfs_qm.c  |  5 ++---
>  3 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 55661cbdb51b..2ca8e2c7fbc4 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -549,17 +549,17 @@ xfs_buf_hash_destroy(
>  }
>  
>  /*
> - *	Look up, and creates if absent, a lockable buffer for
> - *	a given range of an inode.  The buffer is returned
> - *	locked.	No I/O is implied by this call.
> + * Look up (and insert if absent), a lockable buffer for a given
> + * range of an inode.  The buffer is returned locked. No I/O is
> + * implied by this call.
>   */
> -xfs_buf_t *
> +static struct xfs_buf *
>  _xfs_buf_find(
>  	struct xfs_buftarg	*btp,
>  	struct xfs_buf_map	*map,
>  	int			nmaps,
>  	xfs_buf_flags_t		flags,
> -	xfs_buf_t		*new_bp)
> +	struct xfs_buf		*new_bp)
>  {
>  	struct xfs_perag	*pag;
>  	xfs_buf_t		*bp;
> @@ -649,6 +649,17 @@ _xfs_buf_find(
>  	return bp;
>  }
>  
> +struct xfs_buf *
> +xfs_incore(
> +	struct xfs_buftarg	*target,
> +	xfs_daddr_t		blkno,
> +	size_t			numblks,
> +	xfs_buf_flags_t		flags)
> +{
> +	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	return _xfs_buf_find(target, &map, 1, flags, NULL);
> +}
> +
>  /*
>   * Assembles a buffer covering the specified range. The code is optimised for
>   * cache hits, as metadata intensive workloads will see 3 orders of magnitude
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index edced162a674..e8947c1adac0 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -218,20 +218,9 @@ typedef struct xfs_buf {
>  } xfs_buf_t;
>  
>  /* Finding and Reading Buffers */
> -struct xfs_buf *_xfs_buf_find(struct xfs_buftarg *target,
> -			      struct xfs_buf_map *map, int nmaps,
> -			      xfs_buf_flags_t flags, struct xfs_buf *new_bp);
> -
> -static inline struct xfs_buf *
> -xfs_incore(
> -	struct xfs_buftarg	*target,
> -	xfs_daddr_t		blkno,
> -	size_t			numblks,
> -	xfs_buf_flags_t		flags)
> -{
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -	return _xfs_buf_find(target, &map, 1, flags, NULL);
> -}
> +struct xfs_buf *xfs_incore(struct xfs_buftarg *target,
> +			   xfs_daddr_t blkno, size_t numblks,
> +			   xfs_buf_flags_t flags);
>  
>  struct xfs_buf *_xfs_buf_alloc(struct xfs_buftarg *target,
>  			       struct xfs_buf_map *map, int nmaps,
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ec39ae274c78..aff4924f637f 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1247,9 +1247,8 @@ xfs_qm_flush_one(
>  	 */
>  	if (!xfs_dqflock_nowait(dqp)) {
>  		/* buf is pinned in-core by delwri list */
> -		DEFINE_SINGLE_BUF_MAP(map, dqp->q_blkno,
> -				      mp->m_quotainfo->qi_dqchunklen);
> -		bp = _xfs_buf_find(mp->m_ddev_targp, &map, 1, 0, NULL);
> +		bp = xfs_incore(mp->m_ddev_targp, dqp->q_blkno,
> +				mp->m_quotainfo->qi_dqchunklen, 0);
>  		if (!bp) {
>  			error = -EINVAL;
>  			goto out_unlock;
> -- 
> 2.16.1
> 
> --
> 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

-- 
Carlos

Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line

[Darrick J. Wong]

On Wed, Apr 18, 2018 at 01:46:58PM +0200, Carlos Maiolino wrote:
> On Wed, Apr 18, 2018 at 10:21:10AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Move xfs_buf_incore out of line and make it the only way to look up
> > a buffer in the buffer cache from outside the buffer cache. Convert
> > the external users of _xfs_buf_find() to xfs_buf_incore() and make
> > _xfs_buf_find() static.
> > 
> 
> Looks good, but shouldn't it be xfs_incore() instead of xfs_buf_incore() in the
> Subject/description?

I wonder if this function really should be named xfs_buf_incore because
xfs maintains a lot of incore state besides buffers, but as far as this
patch goes I'll fix the commit message on the way in.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> Otherwise
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_buf.c | 21 ++++++++++++++++-----
> >  fs/xfs/xfs_buf.h | 17 +++--------------
> >  fs/xfs/xfs_qm.c  |  5 ++---
> >  3 files changed, 21 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 55661cbdb51b..2ca8e2c7fbc4 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -549,17 +549,17 @@ xfs_buf_hash_destroy(
> >  }
> >  
> >  /*
> > - *	Look up, and creates if absent, a lockable buffer for
> > - *	a given range of an inode.  The buffer is returned
> > - *	locked.	No I/O is implied by this call.
> > + * Look up (and insert if absent), a lockable buffer for a given
> > + * range of an inode.  The buffer is returned locked. No I/O is
> > + * implied by this call.
> >   */
> > -xfs_buf_t *
> > +static struct xfs_buf *
> >  _xfs_buf_find(
> >  	struct xfs_buftarg	*btp,
> >  	struct xfs_buf_map	*map,
> >  	int			nmaps,
> >  	xfs_buf_flags_t		flags,
> > -	xfs_buf_t		*new_bp)
> > +	struct xfs_buf		*new_bp)
> >  {
> >  	struct xfs_perag	*pag;
> >  	xfs_buf_t		*bp;
> > @@ -649,6 +649,17 @@ _xfs_buf_find(
> >  	return bp;
> >  }
> >  
> > +struct xfs_buf *
> > +xfs_incore(
> > +	struct xfs_buftarg	*target,
> > +	xfs_daddr_t		blkno,
> > +	size_t			numblks,
> > +	xfs_buf_flags_t		flags)
> > +{
> > +	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > +	return _xfs_buf_find(target, &map, 1, flags, NULL);
> > +}
> > +
> >  /*
> >   * Assembles a buffer covering the specified range. The code is optimised for
> >   * cache hits, as metadata intensive workloads will see 3 orders of magnitude
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index edced162a674..e8947c1adac0 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -218,20 +218,9 @@ typedef struct xfs_buf {
> >  } xfs_buf_t;
> >  
> >  /* Finding and Reading Buffers */
> > -struct xfs_buf *_xfs_buf_find(struct xfs_buftarg *target,
> > -			      struct xfs_buf_map *map, int nmaps,
> > -			      xfs_buf_flags_t flags, struct xfs_buf *new_bp);
> > -
> > -static inline struct xfs_buf *
> > -xfs_incore(
> > -	struct xfs_buftarg	*target,
> > -	xfs_daddr_t		blkno,
> > -	size_t			numblks,
> > -	xfs_buf_flags_t		flags)
> > -{
> > -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > -	return _xfs_buf_find(target, &map, 1, flags, NULL);
> > -}
> > +struct xfs_buf *xfs_incore(struct xfs_buftarg *target,
> > +			   xfs_daddr_t blkno, size_t numblks,
> > +			   xfs_buf_flags_t flags);
> >  
> >  struct xfs_buf *_xfs_buf_alloc(struct xfs_buftarg *target,
> >  			       struct xfs_buf_map *map, int nmaps,
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index ec39ae274c78..aff4924f637f 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -1247,9 +1247,8 @@ xfs_qm_flush_one(
> >  	 */
> >  	if (!xfs_dqflock_nowait(dqp)) {
> >  		/* buf is pinned in-core by delwri list */
> > -		DEFINE_SINGLE_BUF_MAP(map, dqp->q_blkno,
> > -				      mp->m_quotainfo->qi_dqchunklen);
> > -		bp = _xfs_buf_find(mp->m_ddev_targp, &map, 1, 0, NULL);
> > +		bp = xfs_incore(mp->m_ddev_targp, dqp->q_blkno,
> > +				mp->m_quotainfo->qi_dqchunklen, 0);
> >  		if (!bp) {
> >  			error = -EINVAL;
> >  			goto out_unlock;
> > -- 
> > 2.16.1
> > 
> > --
> > 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
> 
> -- 
> Carlos
> --
> 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 1/2] xfs: make xfs_buf_incore out of line

[Dave Chinner]

On Wed, Apr 18, 2018 at 08:24:42AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 18, 2018 at 01:46:58PM +0200, Carlos Maiolino wrote:
> > On Wed, Apr 18, 2018 at 10:21:10AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Move xfs_buf_incore out of line and make it the only way to look up
> > > a buffer in the buffer cache from outside the buffer cache. Convert
> > > the external users of _xfs_buf_find() to xfs_buf_incore() and make
> > > _xfs_buf_find() static.
> > > 
> > 
> > Looks good, but shouldn't it be xfs_incore() instead of xfs_buf_incore() in the
> > Subject/description?
> 
> I wonder if this function really should be named xfs_buf_incore because
> xfs maintains a lot of incore state besides buffers, but as far as this
> patch goes I'll fix the commit message on the way in.

I think I intended to rename it xfs_buf_incore() in this patch,
but got distracted between writing the commit message (I almost
always do that first) and then making the change. ANd so I forgot
to s/xfs_incore/xfs_buf_incore.

Do you want me to resend?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line

[Darrick J. Wong]

On Thu, Apr 19, 2018 at 09:34:19AM +1000, Dave Chinner wrote:
> On Wed, Apr 18, 2018 at 08:24:42AM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 18, 2018 at 01:46:58PM +0200, Carlos Maiolino wrote:
> > > On Wed, Apr 18, 2018 at 10:21:10AM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Move xfs_buf_incore out of line and make it the only way to look up
> > > > a buffer in the buffer cache from outside the buffer cache. Convert
> > > > the external users of _xfs_buf_find() to xfs_buf_incore() and make
> > > > _xfs_buf_find() static.
> > > > 
> > > 
> > > Looks good, but shouldn't it be xfs_incore() instead of xfs_buf_incore() in the
> > > Subject/description?
> > 
> > I wonder if this function really should be named xfs_buf_incore because
> > xfs maintains a lot of incore state besides buffers, but as far as this
> > patch goes I'll fix the commit message on the way in.
> 
> I think I intended to rename it xfs_buf_incore() in this patch,
> but got distracted between writing the commit message (I almost
> always do that first) and then making the change. ANd so I forgot
> to s/xfs_incore/xfs_buf_incore.
> 
> Do you want me to resend?

Nah, I'll just s/xfs_incore/xfs_buf_incore/ what I already have.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

[PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When looking at an event trace recently, I noticed that non-blocking
buffer lookup attempts would fail on cached locked buffers and then
run the slow cache-miss path. This means we are doing an xfs_buf
allocation, lookup and free unnecessarily every time we avoid
blocking on a locked buffer.

Fix this by changing _xfs_buf_find() to return an error status to
the caller to indicate that we failed the lock attempt rather than
just returning a NULL. This allows the higher level code to
discriminate between a cache miss and an cache hit that we failed to
lock.

This also allows us to return a -EFSCORRUPTED state if we are asked
to look up a block number outside the range of the filesystem in
_xfs_buf_find(), which moves us one step closer to being able to
handle such errors in a more graceful manner at the higher levels.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2ca8e2c7fbc4..41b386c26582 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -549,17 +549,31 @@ xfs_buf_hash_destroy(
 }
 
 /*
- * Look up (and insert if absent), a lockable buffer for a given
- * range of an inode.  The buffer is returned locked. No I/O is
- * implied by this call.
+ * Look up a buffer in the buffer cache and return it referenced and locked
+ * in @found_bp.
+ *
+ * If @new_bp is supplied and we have a lookup miss, insert @new_bp into the
+ * cache.
+ *
+ * If XBF_TRYLOCK is set in @flags, only try to lock the buffer and return
+ * -EAGAIN if we fail to lock it.
+ *
+ * Return values are:
+ *	-EFSCORRUPTED if have been supplied with an invalid address
+ *	-EAGAIN on trylock failure
+ *	-ENOENT if we fail to find a match and @new_bp was NULL
+ *	0, with @found_bp:
+ *		- @new_bp if we inserted it into the cache
+ *		- the buffer we found and locked.
  */
-static struct xfs_buf *
-_xfs_buf_find(
+static int
+xfs_buf_find(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map,
 	int			nmaps,
 	xfs_buf_flags_t		flags,
-	struct xfs_buf		*new_bp)
+	struct xfs_buf		*new_bp,
+	struct xfs_buf		**found_bp)
 {
 	struct xfs_perag	*pag;
 	xfs_buf_t		*bp;
@@ -567,6 +581,8 @@ _xfs_buf_find(
 	xfs_daddr_t		eofs;
 	int			i;
 
+	*found_bp = NULL;
+
 	for (i = 0; i < nmaps; i++)
 		cmap.bm_len += map[i].bm_len;
 
@@ -580,16 +596,11 @@ _xfs_buf_find(
 	 */
 	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
 	if (cmap.bm_bn < 0 || cmap.bm_bn >= eofs) {
-		/*
-		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
-		 * but none of the higher level infrastructure supports
-		 * returning a specific error on buffer lookup failures.
-		 */
 		xfs_alert(btp->bt_mount,
 			  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
 			  __func__, cmap.bm_bn, eofs);
 		WARN_ON(1);
-		return NULL;
+		return -EFSCORRUPTED;
 	}
 
 	pag = xfs_perag_get(btp->bt_mount,
@@ -604,19 +615,20 @@ _xfs_buf_find(
 	}
 
 	/* No match found */
-	if (new_bp) {
-		/* the buffer keeps the perag reference until it is freed */
-		new_bp->b_pag = pag;
-		rhashtable_insert_fast(&pag->pag_buf_hash,
-				       &new_bp->b_rhash_head,
-				       xfs_buf_hash_params);
-		spin_unlock(&pag->pag_buf_lock);
-	} else {
+	if (!new_bp) {
 		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
 		spin_unlock(&pag->pag_buf_lock);
 		xfs_perag_put(pag);
+		return -ENOENT;
 	}
-	return new_bp;
+
+	/* the buffer keeps the perag reference until it is freed */
+	new_bp->b_pag = pag;
+	rhashtable_insert_fast(&pag->pag_buf_hash, &new_bp->b_rhash_head,
+			       xfs_buf_hash_params);
+	spin_unlock(&pag->pag_buf_lock);
+	*found_bp = new_bp;
+	return 0;
 
 found:
 	spin_unlock(&pag->pag_buf_lock);
@@ -626,7 +638,7 @@ _xfs_buf_find(
 		if (flags & XBF_TRYLOCK) {
 			xfs_buf_rele(bp);
 			XFS_STATS_INC(btp->bt_mount, xb_busy_locked);
-			return NULL;
+			return -EAGAIN;
 		}
 		xfs_buf_lock(bp);
 		XFS_STATS_INC(btp->bt_mount, xb_get_locked_waited);
@@ -646,7 +658,8 @@ _xfs_buf_find(
 
 	trace_xfs_buf_find(bp, flags, _RET_IP_);
 	XFS_STATS_INC(btp->bt_mount, xb_get_locked);
-	return bp;
+	*found_bp = bp;
+	return 0;
 }
 
 struct xfs_buf *
@@ -656,8 +669,14 @@ xfs_incore(
 	size_t			numblks,
 	xfs_buf_flags_t		flags)
 {
+	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
+
+	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -676,9 +695,27 @@ xfs_buf_get_map(
 	struct xfs_buf		*new_bp;
 	int			error = 0;
 
-	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
-	if (likely(bp))
+	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
+
+	switch (error) {
+	case 0:
+		/* cache hit */
 		goto found;
+	case -EAGAIN:
+		/* cache hit, trylock failure, caller handles failure */
+		ASSERT(flags & XBF_TRYLOCK);
+		return NULL;
+	case -ENOENT:
+		/* cache miss, go for insert */
+		break;
+	case -EFSCORRUPTED:
+	default:
+		/*
+		 * None of the higher layers understand failure types
+		 * yet, so return NULL to signal a fatal lookup error.
+		 */
+		return NULL;
+	}
 
 	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
 	if (unlikely(!new_bp))
@@ -690,8 +727,8 @@ xfs_buf_get_map(
 		return NULL;
 	}
 
-	bp = _xfs_buf_find(target, map, nmaps, flags, new_bp);
-	if (!bp) {
+	error = xfs_buf_find(target, map, nmaps, flags, new_bp, &bp);
+	if (error) {
 		xfs_buf_free(new_bp);
 		return NULL;
 	}
-- 
2.16.1

Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

[Carlos Maiolino]

On Wed, Apr 18, 2018 at 10:21:11AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When looking at an event trace recently, I noticed that non-blocking
> buffer lookup attempts would fail on cached locked buffers and then
> run the slow cache-miss path. This means we are doing an xfs_buf
> allocation, lookup and free unnecessarily every time we avoid
> blocking on a locked buffer.
> 
> Fix this by changing _xfs_buf_find() to return an error status to
> the caller to indicate that we failed the lock attempt rather than
> just returning a NULL. This allows the higher level code to
> discriminate between a cache miss and an cache hit that we failed to
> lock.
> 
> This also allows us to return a -EFSCORRUPTED state if we are asked
> to look up a block number outside the range of the filesystem in
> _xfs_buf_find(), which moves us one step closer to being able to
> handle such errors in a more graceful manner at the higher levels.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>

Looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2ca8e2c7fbc4..41b386c26582 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -549,17 +549,31 @@ xfs_buf_hash_destroy(
>  }
>  
>  /*
> - * Look up (and insert if absent), a lockable buffer for a given
> - * range of an inode.  The buffer is returned locked. No I/O is
> - * implied by this call.
> + * Look up a buffer in the buffer cache and return it referenced and locked
> + * in @found_bp.
> + *
> + * If @new_bp is supplied and we have a lookup miss, insert @new_bp into the
> + * cache.
> + *
> + * If XBF_TRYLOCK is set in @flags, only try to lock the buffer and return
> + * -EAGAIN if we fail to lock it.
> + *
> + * Return values are:
> + *	-EFSCORRUPTED if have been supplied with an invalid address
> + *	-EAGAIN on trylock failure
> + *	-ENOENT if we fail to find a match and @new_bp was NULL
> + *	0, with @found_bp:
> + *		- @new_bp if we inserted it into the cache
> + *		- the buffer we found and locked.
>   */
> -static struct xfs_buf *
> -_xfs_buf_find(
> +static int
> +xfs_buf_find(
>  	struct xfs_buftarg	*btp,
>  	struct xfs_buf_map	*map,
>  	int			nmaps,
>  	xfs_buf_flags_t		flags,
> -	struct xfs_buf		*new_bp)
> +	struct xfs_buf		*new_bp,
> +	struct xfs_buf		**found_bp)
>  {
>  	struct xfs_perag	*pag;
>  	xfs_buf_t		*bp;
> @@ -567,6 +581,8 @@ _xfs_buf_find(
>  	xfs_daddr_t		eofs;
>  	int			i;
>  
> +	*found_bp = NULL;
> +
>  	for (i = 0; i < nmaps; i++)
>  		cmap.bm_len += map[i].bm_len;
>  
> @@ -580,16 +596,11 @@ _xfs_buf_find(
>  	 */
>  	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
>  	if (cmap.bm_bn < 0 || cmap.bm_bn >= eofs) {
> -		/*
> -		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
> -		 * but none of the higher level infrastructure supports
> -		 * returning a specific error on buffer lookup failures.
> -		 */
>  		xfs_alert(btp->bt_mount,
>  			  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
>  			  __func__, cmap.bm_bn, eofs);
>  		WARN_ON(1);
> -		return NULL;
> +		return -EFSCORRUPTED;
>  	}
>  
>  	pag = xfs_perag_get(btp->bt_mount,
> @@ -604,19 +615,20 @@ _xfs_buf_find(
>  	}
>  
>  	/* No match found */
> -	if (new_bp) {
> -		/* the buffer keeps the perag reference until it is freed */
> -		new_bp->b_pag = pag;
> -		rhashtable_insert_fast(&pag->pag_buf_hash,
> -				       &new_bp->b_rhash_head,
> -				       xfs_buf_hash_params);
> -		spin_unlock(&pag->pag_buf_lock);
> -	} else {
> +	if (!new_bp) {
>  		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
>  		spin_unlock(&pag->pag_buf_lock);
>  		xfs_perag_put(pag);
> +		return -ENOENT;
>  	}
> -	return new_bp;
> +
> +	/* the buffer keeps the perag reference until it is freed */
> +	new_bp->b_pag = pag;
> +	rhashtable_insert_fast(&pag->pag_buf_hash, &new_bp->b_rhash_head,
> +			       xfs_buf_hash_params);
> +	spin_unlock(&pag->pag_buf_lock);
> +	*found_bp = new_bp;
> +	return 0;
>  
>  found:
>  	spin_unlock(&pag->pag_buf_lock);
> @@ -626,7 +638,7 @@ _xfs_buf_find(
>  		if (flags & XBF_TRYLOCK) {
>  			xfs_buf_rele(bp);
>  			XFS_STATS_INC(btp->bt_mount, xb_busy_locked);
> -			return NULL;
> +			return -EAGAIN;
>  		}
>  		xfs_buf_lock(bp);
>  		XFS_STATS_INC(btp->bt_mount, xb_get_locked_waited);
> @@ -646,7 +658,8 @@ _xfs_buf_find(
>  
>  	trace_xfs_buf_find(bp, flags, _RET_IP_);
>  	XFS_STATS_INC(btp->bt_mount, xb_get_locked);
> -	return bp;
> +	*found_bp = bp;
> +	return 0;
>  }
>  
>  struct xfs_buf *
> @@ -656,8 +669,14 @@ xfs_incore(
>  	size_t			numblks,
>  	xfs_buf_flags_t		flags)
>  {
> +	struct xfs_buf		*bp;
> +	int			error;
>  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -	return _xfs_buf_find(target, &map, 1, flags, NULL);
> +
> +	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
> +	if (error)
> +		return NULL;
> +	return bp;
>  }
>  
>  /*
> @@ -676,9 +695,27 @@ xfs_buf_get_map(
>  	struct xfs_buf		*new_bp;
>  	int			error = 0;
>  
> -	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
> -	if (likely(bp))
> +	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
> +
> +	switch (error) {
> +	case 0:
> +		/* cache hit */
>  		goto found;
> +	case -EAGAIN:
> +		/* cache hit, trylock failure, caller handles failure */
> +		ASSERT(flags & XBF_TRYLOCK);
> +		return NULL;
> +	case -ENOENT:
> +		/* cache miss, go for insert */
> +		break;
> +	case -EFSCORRUPTED:
> +	default:
> +		/*
> +		 * None of the higher layers understand failure types
> +		 * yet, so return NULL to signal a fatal lookup error.
> +		 */
> +		return NULL;
> +	}
>  
>  	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
>  	if (unlikely(!new_bp))
> @@ -690,8 +727,8 @@ xfs_buf_get_map(
>  		return NULL;
>  	}
>  
> -	bp = _xfs_buf_find(target, map, nmaps, flags, new_bp);
> -	if (!bp) {
> +	error = xfs_buf_find(target, map, nmaps, flags, new_bp, &bp);
> +	if (error) {
>  		xfs_buf_free(new_bp);
>  		return NULL;
>  	}
> -- 
> 2.16.1
> 
> --
> 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

-- 
Carlos

Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

[Darrick J. Wong]

On Wed, Apr 18, 2018 at 10:21:11AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When looking at an event trace recently, I noticed that non-blocking
> buffer lookup attempts would fail on cached locked buffers and then
> run the slow cache-miss path. This means we are doing an xfs_buf
> allocation, lookup and free unnecessarily every time we avoid
> blocking on a locked buffer.
> 
> Fix this by changing _xfs_buf_find() to return an error status to
> the caller to indicate that we failed the lock attempt rather than
> just returning a NULL. This allows the higher level code to
> discriminate between a cache miss and an cache hit that we failed to
> lock.
> 
> This also allows us to return a -EFSCORRUPTED state if we are asked
> to look up a block number outside the range of the filesystem in
> _xfs_buf_find(), which moves us one step closer to being able to
> handle such errors in a more graceful manner at the higher levels.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2ca8e2c7fbc4..41b386c26582 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -549,17 +549,31 @@ xfs_buf_hash_destroy(
>  }
>  
>  /*
> - * Look up (and insert if absent), a lockable buffer for a given
> - * range of an inode.  The buffer is returned locked. No I/O is
> - * implied by this call.
> + * Look up a buffer in the buffer cache and return it referenced and locked
> + * in @found_bp.
> + *
> + * If @new_bp is supplied and we have a lookup miss, insert @new_bp into the
> + * cache.
> + *
> + * If XBF_TRYLOCK is set in @flags, only try to lock the buffer and return
> + * -EAGAIN if we fail to lock it.
> + *
> + * Return values are:
> + *	-EFSCORRUPTED if have been supplied with an invalid address
> + *	-EAGAIN on trylock failure
> + *	-ENOENT if we fail to find a match and @new_bp was NULL
> + *	0, with @found_bp:
> + *		- @new_bp if we inserted it into the cache
> + *		- the buffer we found and locked.
>   */
> -static struct xfs_buf *
> -_xfs_buf_find(
> +static int
> +xfs_buf_find(
>  	struct xfs_buftarg	*btp,
>  	struct xfs_buf_map	*map,
>  	int			nmaps,
>  	xfs_buf_flags_t		flags,
> -	struct xfs_buf		*new_bp)
> +	struct xfs_buf		*new_bp,
> +	struct xfs_buf		**found_bp)
>  {
>  	struct xfs_perag	*pag;
>  	xfs_buf_t		*bp;
> @@ -567,6 +581,8 @@ _xfs_buf_find(
>  	xfs_daddr_t		eofs;
>  	int			i;
>  
> +	*found_bp = NULL;
> +
>  	for (i = 0; i < nmaps; i++)
>  		cmap.bm_len += map[i].bm_len;
>  
> @@ -580,16 +596,11 @@ _xfs_buf_find(
>  	 */
>  	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
>  	if (cmap.bm_bn < 0 || cmap.bm_bn >= eofs) {
> -		/*
> -		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
> -		 * but none of the higher level infrastructure supports
> -		 * returning a specific error on buffer lookup failures.
> -		 */
>  		xfs_alert(btp->bt_mount,
>  			  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
>  			  __func__, cmap.bm_bn, eofs);
>  		WARN_ON(1);
> -		return NULL;
> +		return -EFSCORRUPTED;
>  	}
>  
>  	pag = xfs_perag_get(btp->bt_mount,
> @@ -604,19 +615,20 @@ _xfs_buf_find(
>  	}
>  
>  	/* No match found */
> -	if (new_bp) {
> -		/* the buffer keeps the perag reference until it is freed */
> -		new_bp->b_pag = pag;
> -		rhashtable_insert_fast(&pag->pag_buf_hash,
> -				       &new_bp->b_rhash_head,
> -				       xfs_buf_hash_params);
> -		spin_unlock(&pag->pag_buf_lock);
> -	} else {
> +	if (!new_bp) {
>  		XFS_STATS_INC(btp->bt_mount, xb_miss_locked);
>  		spin_unlock(&pag->pag_buf_lock);
>  		xfs_perag_put(pag);
> +		return -ENOENT;
>  	}
> -	return new_bp;
> +
> +	/* the buffer keeps the perag reference until it is freed */
> +	new_bp->b_pag = pag;
> +	rhashtable_insert_fast(&pag->pag_buf_hash, &new_bp->b_rhash_head,
> +			       xfs_buf_hash_params);
> +	spin_unlock(&pag->pag_buf_lock);
> +	*found_bp = new_bp;
> +	return 0;
>  
>  found:
>  	spin_unlock(&pag->pag_buf_lock);
> @@ -626,7 +638,7 @@ _xfs_buf_find(
>  		if (flags & XBF_TRYLOCK) {
>  			xfs_buf_rele(bp);
>  			XFS_STATS_INC(btp->bt_mount, xb_busy_locked);
> -			return NULL;
> +			return -EAGAIN;
>  		}
>  		xfs_buf_lock(bp);
>  		XFS_STATS_INC(btp->bt_mount, xb_get_locked_waited);
> @@ -646,7 +658,8 @@ _xfs_buf_find(
>  
>  	trace_xfs_buf_find(bp, flags, _RET_IP_);
>  	XFS_STATS_INC(btp->bt_mount, xb_get_locked);
> -	return bp;
> +	*found_bp = bp;
> +	return 0;
>  }
>  
>  struct xfs_buf *
> @@ -656,8 +669,14 @@ xfs_incore(
>  	size_t			numblks,
>  	xfs_buf_flags_t		flags)
>  {
> +	struct xfs_buf		*bp;
> +	int			error;
>  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -	return _xfs_buf_find(target, &map, 1, flags, NULL);
> +
> +	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
> +	if (error)
> +		return NULL;
> +	return bp;
>  }
>  
>  /*
> @@ -676,9 +695,27 @@ xfs_buf_get_map(
>  	struct xfs_buf		*new_bp;
>  	int			error = 0;
>  
> -	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
> -	if (likely(bp))
> +	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
> +
> +	switch (error) {
> +	case 0:
> +		/* cache hit */
>  		goto found;
> +	case -EAGAIN:
> +		/* cache hit, trylock failure, caller handles failure */
> +		ASSERT(flags & XBF_TRYLOCK);
> +		return NULL;
> +	case -ENOENT:
> +		/* cache miss, go for insert */
> +		break;
> +	case -EFSCORRUPTED:
> +	default:
> +		/*
> +		 * None of the higher layers understand failure types
> +		 * yet, so return NULL to signal a fatal lookup error.
> +		 */
> +		return NULL;
> +	}
>  
>  	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
>  	if (unlikely(!new_bp))
> @@ -690,8 +727,8 @@ xfs_buf_get_map(
>  		return NULL;
>  	}
>  
> -	bp = _xfs_buf_find(target, map, nmaps, flags, new_bp);
> -	if (!bp) {
> +	error = xfs_buf_find(target, map, nmaps, flags, new_bp, &bp);
> +	if (error) {
>  		xfs_buf_free(new_bp);
>  		return NULL;
>  	}
> -- 
> 2.16.1
> 
> --
> 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