On Wed, Apr 18, 2018 at 10:21:11AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
Looks good
Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
