xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

PATCH In review Dave Chinner 3 participants 5 messages Mar 1, 2018 – Mar 2, 2018 XFS filesystem

lore

Share & tools

lore

Dave Chinner

author

2018-03-01 22:37 UTC #0 lore raw

PATCH 1/1 xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure

From: Dave Chinner <[email protected]>

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 encoded via ERR_PTR() 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.

Finally, to ensure code outside the buffer cache does not see any change, convert external callers to use xfs_incore() and change that to an inline function that maintains the old "buffer or NULL" return values so the external code doesn't need to care about this internal change to _xfs_buf_find() semantics.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_buf.c | 56 +++++++++++++++++++++++++++++++++++++++++---------------
 fs/xfs/xfs_buf.h |  8 +++++++-
 fs/xfs/xfs_qm.c  |  5 ++---
 3 files changed, 50 insertions(+), 19 deletions(-)

fs/xfs/xfs_buf.c +41 ~~-15~~

index 88587b33dd15..fa1e62ac5e8c 100644
@@ -549,20 +549,32 @@ 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 a buffer in the buffer cache and return it referenced and locked.
+ *
+ * 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
+ *	NULL if we fail to find a match and @new_bp was NULL
+ *	@new_bp if we inserted it into the cache
+ *	the buffer we found and locked.
  */
-xfs_buf_t *
+
+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;
+	struct xfs_buf		*bp;
 	struct xfs_buf_map	cmap = { .bm_bn = map[0].bm_bn };
 	xfs_daddr_t		eofs;
 	int			i;

@@ -580,16 +592,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 ERR_PTR(-EFSCORRUPTED);
 	}
 
 	pag = xfs_perag_get(btp->bt_mount,

@@ -626,7 +633,7 @@ _xfs_buf_find(
 		if (flags & XBF_TRYLOCK) {
 			xfs_buf_rele(bp);
 			XFS_STATS_INC(btp->bt_mount, xb_busy_locked);
-			return NULL;
+			return ERR_PTR(-EAGAIN);
 		}
 		xfs_buf_lock(bp);
 		XFS_STATS_INC(btp->bt_mount, xb_get_locked_waited);

@@ -666,9 +673,28 @@ xfs_buf_get_map(
 	int			error = 0;
 
 	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
-	if (likely(bp))
+	if (!IS_ERR_OR_NULL(bp))
 		goto found;
 
+	switch (PTR_ERR(bp)) {
+		case 0:
+			/* cache miss, need to insert new buffer */
+			break;
+
+		case -EAGAIN:
+			/* cache hit, trylock failure, caller handles failure */
+			ASSERT(flags & XBF_TRYLOCK);
+			return NULL;
+
+		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))
 		return NULL;

@@ -680,7 +706,7 @@ xfs_buf_get_map(
 	}
 
 	bp = _xfs_buf_find(target, map, nmaps, flags, new_bp);
-	if (!bp) {
+	if (IS_ERR_OR_NULL(bp)) {
 		xfs_buf_free(new_bp);
 		return NULL;
 	}

fs/xfs/xfs_buf.h +7 -1

index 2f4c91452861..db87ad0b9b79 100644
@@ -229,8 +229,14 @@ xfs_incore(
 	size_t			numblks,
 	xfs_buf_flags_t		flags)
 {
+	struct xfs_buf		*bp;
+
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
+
+	bp = _xfs_buf_find(target, &map, 1, flags, NULL);
+	if (IS_ERR_OR_NULL(bp))
+		return NULL;
+	return bp;
 }
 
 struct xfs_buf *_xfs_buf_alloc(struct xfs_buftarg *target,

fs/xfs/xfs_qm.c +2 -3

index 5b848f4b637f..8d90d19684a7 100644
@@ -1249,9 +1249,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;

Signature · 1 line

2.16.1

Darrick J. Wong

2018-03-02 17:37 UTC #1 lore raw

Quoted 117 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_buf.c | 56 +++++++++++++++++++++++++++++++++++++++++---------------
 fs/xfs/xfs_buf.h |  8 +++++++-
 fs/xfs/xfs_qm.c  |  5 ++---
 3 files changed, 50 insertions(+), 19 deletions(-)

fs/xfs/xfs_buf.c +28 ~~-14~~

index 88587b33dd15..fa1e62ac5e8c 100644
@@ -549,20 +549,32 @@ 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 a buffer in the buffer cache and return it referenced and locked.
+ *
+ * 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
+ *	NULL if we fail to find a match and @new_bp was NULL
+ *	@new_bp if we inserted it into the cache
+ *	the buffer we found and locked.
  */
-xfs_buf_t *
+
+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;
+	struct xfs_buf		*bp;
 	struct xfs_buf_map	cmap = { .bm_bn = map[0].bm_bn };
 	xfs_daddr_t		eofs;
 	int			i;

@@ -580,16 +592,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 ERR_PTR(-EFSCORRUPTED);
 	}
 
 	pag = xfs_perag_get(btp->bt_mount,

@@ -626,7 +633,7 @@ _xfs_buf_find(
 		if (flags & XBF_TRYLOCK) {
 			xfs_buf_rele(bp);
 			XFS_STATS_INC(btp->bt_mount, xb_busy_locked);
-			return NULL;
+			return ERR_PTR(-EAGAIN);
 		}
 		xfs_buf_lock(bp);
 		XFS_STATS_INC(btp->bt_mount, xb_get_locked_waited);

@@ -666,9 +673,28 @@ xfs_buf_get_map(
 	int			error = 0;
 
 	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
-	if (likely(bp))
+	if (!IS_ERR_OR_NULL(bp))
 		goto found;
 
+	switch (PTR_ERR(bp)) {
+		case 0:
+			/* cache miss, need to insert new buffer */
+			break;
+
+		case -EAGAIN:
+			/* cache hit, trylock failure, caller handles failure */

fs/xfs/xfs_buf.c xfs_buf_get_map( · line 695

Quoted 10 lines

fs/xfs/xfs_buf.c +10 -0

+			ASSERT(flags & XBF_TRYLOCK);
+			return NULL;
+
+		case -EFSCORRUPTED:
+		default:
+			/*
+			 * None of the higher layers understand failure types
+			 * yet, so return NULL to signal a fatal lookup error.
+			 */
+			return NULL;

Should I expect a follow-on patch to fix the higher layers?

Quoted 39 lines

fs/xfs/xfs_buf.c +3 -1

+	}
+
 	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
 	if (unlikely(!new_bp))
 		return NULL;

@@ -680,7 +706,7 @@ xfs_buf_get_map(
 	}
 
 	bp = _xfs_buf_find(target, map, nmaps, flags, new_bp);
-	if (!bp) {
+	if (IS_ERR_OR_NULL(bp)) {
 		xfs_buf_free(new_bp);
 		return NULL;
 	}

fs/xfs/xfs_buf.h +7 -1

index 2f4c91452861..db87ad0b9b79 100644
@@ -229,8 +229,14 @@ xfs_incore(
 	size_t			numblks,
 	xfs_buf_flags_t		flags)
 {
+	struct xfs_buf		*bp;
+
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
+
+	bp = _xfs_buf_find(target, &map, 1, flags, NULL);
+	if (IS_ERR_OR_NULL(bp))
+		return NULL;
+	return bp;
 }
 
 struct xfs_buf *_xfs_buf_alloc(struct xfs_buftarg *target,

fs/xfs/xfs_qm.c +0 -0

index 5b848f4b637f..8d90d19684a7 100644

fs/xfs/xfs_qm.c xfs_qm_flush_one( · line 1253

Quoted 8 lines

fs/xfs/xfs_qm.c +2 -3

@@ -1249,9 +1249,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);

Looks ok otherwise.

--D

Quoted 10 lines

fs/xfs/xfs_qm.c +0 -0

 		if (!bp) {
 			error = -EINVAL;
 			goto out_unlock;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner

author

2018-03-02 21:56 UTC #2 lore raw

Quoted 46 lines · Darrick J. Wong

Quoted 45 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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 encoded via ERR_PTR() 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.
Finally, to ensure code outside the buffer cache does not see any change, convert external callers to use xfs_incore() and change that to an inline function that maintains the old "buffer or NULL" return values so the external code doesn't need to care about this internal change to _xfs_buf_find() semantics.
Signed-off-by Dave Chinner <[email protected]>
fs/xfs/xfs_buf.c +12 -1
@@ -666,9 +673,28 @@ xfs_buf_get_map(
 	int			error = 0;
 
 	bp = _xfs_buf_find(target, map, nmaps, flags, NULL);
-	if (likely(bp))
+	if (!IS_ERR_OR_NULL(bp))
 		goto found;
 
+	switch (PTR_ERR(bp)) {
+		case 0:
+			/* cache miss, need to insert new buffer */
+			break;
+
+		case -EAGAIN:
+			/* cache hit, trylock failure, caller handles failure */
+			ASSERT(flags & XBF_TRYLOCK);
+			return NULL;
+
+		case -EFSCORRUPTED:

Quoted 8 lines

fs/xfs/xfs_buf.c xfs_buf_get_map( · line 695
Quoted 6 lines
fs/xfs/xfs_buf.c +6 -0
+		default:
+			/*
+			 * None of the higher layers understand failure types
+			 * yet, so return NULL to signal a fatal lookup error.
+			 */
+			return NULL;
Should I expect a follow-on patch to fix the higher layers?

Not immediately. There's much larger quantity of code affected by pushing it another layer up, and that's way outside the scope of fixing this inefficiency. I've just fixed this layer in a way that moves us towards being able to report more accurate failures higher up without changing the current status quo...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

2018-03-02 22:50 UTC #3 lore raw

In reply to Dave Chinner

Quoted 7 lines

/*
+ * Look up a buffer in the buffer cache and return it referenced and locked.
+ *
+ * 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

Quoted 8 lines

+ * -EAGAIN if we fail to lock it.
+ *
+ * Return values are:
+ * -EFSCORRUPTED if have been supplied with an invalid address
+ * -EAGAIN on trylock failure
+ * NULL if we fail to find a match and @new_bp was NULL
+ * @new_bp if we inserted it into the cache
+ * the buffer we found and locked.

NULL or error calling conventions are nasty. Can we switch to always return an ERR_PTR?

Quoted 8 lines

fs/xfs/xfs_buf.c +8 -0

+	switch (PTR_ERR(bp)) {
+		case 0:
+			/* cache miss, need to insert new buffer */
+			break;
+
+		case -EAGAIN:
+			/* cache hit, trylock failure, caller handles failure */
+			ASSERT(flags & XBF_TRYLOCK);

fs/xfs/xfs_buf.c xfs_buf_get_map( · line 696

Quoted 10 lines

fs/xfs/xfs_buf.c +10 -0

+			return NULL;
+
+		case -EFSCORRUPTED:
+		default:
+			/*
+			 * None of the higher layers understand failure types
+			 * yet, so return NULL to signal a fatal lookup error.
+			 */
+			return NULL;
+	}

Usual style is to not indent the switch labels.

Quoted 8 lines

fs/xfs/xfs_buf.h +0 -0

index 2f4c91452861..db87ad0b9b79 100644
@@ -229,8 +229,14 @@ xfs_incore(
 	size_t			numblks,
 	xfs_buf_flags_t		flags)
 {

fs/xfs/xfs_buf.h xfs_incore( · line 240

Quoted 10 lines

fs/xfs/xfs_buf.h +7 -1

+	struct xfs_buf		*bp;
+
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
+
+	bp = _xfs_buf_find(target, &map, 1, flags, NULL);
+	if (IS_ERR_OR_NULL(bp))
+		return NULL;
+	return bp;
 }

Can we move xfs_incore to xfs_buf.c and stop exporting _xfs_buf_find while we're at it?

Quoted 6 lines

fs/xfs/xfs_qm.c +0 -0

index 5b848f4b637f..8d90d19684a7 100644
@@ -1249,9 +1249,8 @@ xfs_qm_flush_one(
 	 */

fs/xfs/xfs_qm.c xfs_qm_flush_one( · line 1256

Quoted 10 lines

fs/xfs/xfs_qm.c +2 -3

 	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;

Make this a separate prep patch, please. Maybe together with moving xfs_incore out of line.

Dave Chinner

author

2018-03-02 22:55 UTC #4 lore raw

Quoted 16 lines · Christoph Hellwig

Quoted 7 lines
/*
+ * Look up a buffer in the buffer cache and return it referenced and locked.
+ *
+ * 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
Quoted 8 lines
+ * -EAGAIN if we fail to lock it.
+ *
+ * Return values are:
+ * -EFSCORRUPTED if have been supplied with an invalid address
+ * -EAGAIN on trylock failure
+ * NULL if we fail to find a match and @new_bp was NULL
+ * @new_bp if we inserted it into the cache
+ * the buffer we found and locked.

Quoted 2 lines

NULL or error calling conventions are nasty. Can we switch to always return an ERR_PTR?

Ok, I'll return bp as a parameter and just use the return variable as a pure error code.

Quoted 12 lines

fs/xfs/xfs_buf.c +12 -0

+	switch (PTR_ERR(bp)) {
+		case 0:
+			/* cache miss, need to insert new buffer */
+			break;
+
+		case -EAGAIN:
+			/* cache hit, trylock failure, caller handles failure */
+			ASSERT(flags & XBF_TRYLOCK);
+			return NULL;
+
+		case -EFSCORRUPTED:
+		default:

Quoted 8 lines

fs/xfs/xfs_buf.c xfs_buf_get_map( · line 696
Quoted 6 lines
fs/xfs/xfs_buf.c +6 -0
+			/*
+			 * None of the higher layers understand failure types
+			 * yet, so return NULL to signal a fatal lookup error.
+			 */
+			return NULL;
+	}
Usual style is to not indent the switch labels.

*nod*

Quoted 13 lines

fs/xfs/xfs_buf.h +3 -1

index 2f4c91452861..db87ad0b9b79 100644
@@ -229,8 +229,14 @@ xfs_incore(
 	size_t			numblks,
 	xfs_buf_flags_t		flags)
 {
+	struct xfs_buf		*bp;
+
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return _xfs_buf_find(target, &map, 1, flags, NULL);
+

Quoted 8 lines

fs/xfs/xfs_buf.h xfs_incore( · line 240
Quoted 5 lines
fs/xfs/xfs_buf.h +4 -0
+	bp = _xfs_buf_find(target, &map, 1, flags, NULL);
+	if (IS_ERR_OR_NULL(bp))
+		return NULL;
+	return bp;
 }
Can we move xfs_incore to xfs_buf.c and stop exporting _xfs_buf_find while we're at it?

Yes. :)

Quoted 11 lines

fs/xfs/xfs_qm.c +0 -3

index 5b848f4b637f..8d90d19684a7 100644
@@ -1249,9 +1249,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);

Quoted 8 lines

fs/xfs/xfs_qm.c xfs_qm_flush_one( · line 1256
Quoted 5 lines
fs/xfs/xfs_qm.c +2 -0
+		bp = xfs_incore(mp->m_ddev_targp, dqp->q_blkno,
+				mp->m_quotainfo->qi_dqchunklen, 0);
 		if (!bp) {
 			error = -EINVAL;
 			goto out_unlock;
Make this a separate prep patch, please. Maybe together with moving xfs_incore out of line.

Ok.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]