xfs: optimise XBF_TRYLOCK buffer lookups
cover xfs: optimise XBF_TRYLOCK buffer lookups
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
From: Dave Chinner <[email protected]>
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.
--- 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(-)
fs/xfs/xfs_buf.c +16 -5
@@ -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
fs/xfs/xfs_buf.h +3 -14
@@ -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,
fs/xfs/xfs_qm.c +2 -3
@@ -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;
Signature · 1 line
2.16.1
Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Move
xfs_buf_incoreout 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()toxfs_buf_incore()and make_xfs_buf_find()static.Signed-off-by Dave Chinner <[email protected]>
Looks good,
Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line
In reply to Dave ChinnerQuoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Move
xfs_buf_incoreout 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()toxfs_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
Quoted 103 lines
Signed-off-by Dave Chinner <[email protected]>--- 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(-)fs/xfs/xfs_buf.c +16
-5@@ -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 fs/xfs/xfs_buf.h +3
-14@@ -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,fs/xfs/xfs_qm.c +2
-3@@ -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;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
Signature · 1 line
Carlos
Darrick J. Wong
Reviewed-byRe: [PATCH 1/2] xfs: make xfs_buf_incore out of line
Quoted 11 lines · Carlos Maiolino
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Move
xfs_buf_incoreout 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()toxfs_buf_incore()and make_xfs_buf_find()static.Looks good, but shouldn't it be
xfs_incore()instead ofxfs_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.
--D
Quoted 115 lines
Otherwise
Reviewed-by Carlos Maiolino <[email protected]>Quoted 103 lines
Signed-off-by Dave Chinner <[email protected]>--- 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(-)fs/xfs/xfs_buf.c +16
-5@@ -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 fs/xfs/xfs_buf.h +3
-14@@ -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,fs/xfs/xfs_qm.c +2
-3@@ -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;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.htmlSignature · 5 lines
Carlos -- 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
Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line
Quoted 10 lines · Darrick J. Wong
Quoted 9 lines · Carlos Maiolino
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Move
xfs_buf_incoreout 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()toxfs_buf_incore()and make_xfs_buf_find()static.
Quoted 6 lines
Quoted 2 lines
Looks good, but shouldn't it be
xfs_incore()instead ofxfs_buf_incore()in the Subject/description?I wonder if this function really should be named
xfs_buf_incorebecause 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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/2] xfs: make xfs_buf_incore out of line
Quoted 18 lines · Dave Chinner
Quoted 10 lines · Darrick J. Wong
Quoted 9 lines · Carlos Maiolino
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Move
xfs_buf_incoreout 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()toxfs_buf_incore()and make_xfs_buf_find()static.Quoted 6 lines
Quoted 2 lines
Looks good, but shouldn't it be
xfs_incore()instead ofxfs_buf_incore()in the Subject/description?I wonder if this function really should be named
xfs_buf_incorebecause 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.
Quoted 6 lines
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
Quoted 10 lines
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] -- 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
PATCH 2/2 xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure
In reply to Dave ChinnerFrom: 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 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.
--- fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 28 deletions(-)
fs/xfs/xfs_buf.c +65 -28
@@ -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 ; }
Signature · 1 line
2.16.1
Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure
Looks good,
Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure
In reply to Dave ChinnerQuoted 14 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_bufallocation, 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.
Quoted 6 lines
This also allows us to return a
-EFSCORRUPTEDstate 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 <[email protected]>
Looks good
Quoted 186 lines
--- fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 28 deletions(-)fs/xfs/xfs_buf.c +65
-28@@ -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 ; }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
Signature · 1 line
Carlos
Re: [PATCH 2/2] xfs: don't retry xfs_buf_find on XBF_TRYLOCK failure
In reply to Dave ChinnerQuoted 14 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_bufallocation, 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.
Quoted 6 lines
This also allows us to return a
-EFSCORRUPTEDstate 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 <[email protected]>
Looks ok,
--D
Quoted 186 lines
--- fs/xfs/xfs_buf.c | 93 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 28 deletions(-)fs/xfs/xfs_buf.c +65
-28@@ -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 ; }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