Re: [PATCH 6/9] xfs: map buffers in xfs_buf_alloc_folios

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Mar 19, 2024 at 09:45:57AM +1100, Dave Chinner wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> With the concept of unmapped buffer gone, there is no reason to not
> vmap the buffer pages directly in xfs_buf_alloc_folios.

"..no reason to not map the buffer pages..."?

I say that mostly because the first dumb thought I had was "wait, we're
going to vm_map_ram everything now??" which of course is not true.

> [dchinner: port to folio-enabled buffers.]
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

With that changed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 37 ++++++-------------------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2cd3671f3ce3..a77e2d8c8107 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -469,18 +469,7 @@ xfs_buf_alloc_folios(
>  		XFS_STATS_INC(bp->b_mount, xb_page_retries);
>  		memalloc_retry_wait(gfp_mask);
>  	}
> -	return 0;
> -}
>  
> -/*
> - *	Map buffer into kernel address-space if necessary.
> - */
> -STATIC int
> -_xfs_buf_map_folios(
> -	struct xfs_buf		*bp,
> -	xfs_buf_flags_t		flags)
> -{
> -	ASSERT(bp->b_flags & _XBF_FOLIOS);
>  	if (bp->b_folio_count == 1) {
>  		/* A single folio buffer is always mappable */
>  		bp->b_addr = folio_address(bp->b_folios[0]);
> @@ -513,8 +502,13 @@ _xfs_buf_map_folios(
>  		} while (retried++ <= 1);
>  		memalloc_nofs_restore(nofs_flag);
>  
> -		if (!bp->b_addr)
> +		if (!bp->b_addr) {
> +			xfs_warn_ratelimited(bp->b_mount,
> +				"%s: failed to map %u folios", __func__,
> +				bp->b_folio_count);
> +			xfs_buf_free_folios(bp);
>  			return -ENOMEM;
> +		}
>  	}
>  
>  	return 0;
> @@ -816,18 +810,6 @@ xfs_buf_get_map(
>  			xfs_perag_put(pag);
>  	}
>  
> -	/* We do not hold a perag reference anymore. */
> -	if (!bp->b_addr) {
> -		error = _xfs_buf_map_folios(bp, flags);
> -		if (unlikely(error)) {
> -			xfs_warn_ratelimited(btp->bt_mount,
> -				"%s: failed to map %u folios", __func__,
> -				bp->b_folio_count);
> -			xfs_buf_relse(bp);
> -			return error;
> -		}
> -	}
> -
>  	/*
>  	 * Clear b_error if this is a lookup from a caller that doesn't expect
>  	 * valid data to be found in the buffer.
> @@ -1068,13 +1050,6 @@ xfs_buf_get_uncached(
>  	if (error)
>  		goto fail_free_buf;
>  
> -	error = _xfs_buf_map_folios(bp, 0);
> -	if (unlikely(error)) {
> -		xfs_warn(target->bt_mount,
> -			"%s: failed to map folios", __func__);
> -		goto fail_free_buf;
> -	}
> -
>  	trace_xfs_buf_get_uncached(bp, _RET_IP_);
>  	*bpp = bp;
>  	return 0;
> -- 
> 2.43.0
> 
>