Re: [PATCH 1/6] xfs: rework xfs_buf_incore() API

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

On Mon, Jun 27, 2022 at 04:08:36PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Make it consistent with the other buffer APIs to return a error and
> the buffer is placed in a parameter.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

LGTM
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr_remote.c | 15 ++++++++++-----
>  fs/xfs/scrub/repair.c           | 15 +++++++++------
>  fs/xfs/xfs_buf.c                | 19 ++-----------------
>  fs/xfs/xfs_buf.h                | 20 ++++++++++++++++----
>  fs/xfs/xfs_qm.c                 |  9 ++++-----
>  5 files changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 7298c148f848..d440393b40eb 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -543,6 +543,7 @@ xfs_attr_rmtval_stale(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_buf		*bp;
> +	int			error;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> @@ -550,14 +551,18 @@ xfs_attr_rmtval_stale(
>  	    XFS_IS_CORRUPT(mp, map->br_startblock == HOLESTARTBLOCK))
>  		return -EFSCORRUPTED;
>  
> -	bp = xfs_buf_incore(mp->m_ddev_targp,
> +	error = xfs_buf_incore(mp->m_ddev_targp,
>  			XFS_FSB_TO_DADDR(mp, map->br_startblock),
> -			XFS_FSB_TO_BB(mp, map->br_blockcount), incore_flags);
> -	if (bp) {
> -		xfs_buf_stale(bp);
> -		xfs_buf_relse(bp);
> +			XFS_FSB_TO_BB(mp, map->br_blockcount),
> +			incore_flags, &bp);
> +	if (error) {
> +		if (error == -ENOENT)
> +			return 0;
> +		return error;
>  	}
>  
> +	xfs_buf_stale(bp);
> +	xfs_buf_relse(bp);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 1e7b6b209ee8..5e7428782571 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -457,16 +457,19 @@ xrep_invalidate_blocks(
>  	 * assume it's owned by someone else.
>  	 */
>  	for_each_xbitmap_block(fsbno, bmr, n, bitmap) {
> +		int		error;
> +
>  		/* Skip AG headers and post-EOFS blocks */
>  		if (!xfs_verify_fsbno(sc->mp, fsbno))
>  			continue;
> -		bp = xfs_buf_incore(sc->mp->m_ddev_targp,
> +		error = xfs_buf_incore(sc->mp->m_ddev_targp,
>  				XFS_FSB_TO_DADDR(sc->mp, fsbno),
> -				XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK);
> -		if (bp) {
> -			xfs_trans_bjoin(sc->tp, bp);
> -			xfs_trans_binval(sc->tp, bp);
> -		}
> +				XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK, &bp);
> +		if (error)
> +			continue;
> +
> +		xfs_trans_bjoin(sc->tp, bp);
> +		xfs_trans_binval(sc->tp, bp);
>  	}
>  
>  	return 0;
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index bf4e60871068..143e1c70df5d 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -616,23 +616,6 @@ xfs_buf_find(
>  	return 0;
>  }
>  
> -struct xfs_buf *
> -xfs_buf_incore(
> -	struct xfs_buftarg	*target,
> -	xfs_daddr_t		blkno,
> -	size_t			numblks,
> -	xfs_buf_flags_t		flags)
> -{
> -	struct xfs_buf		*bp;
> -	int			error;
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -
> -	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
> -	if (error)
> -		return NULL;
> -	return bp;
> -}
> -
>  /*
>   * Assembles a buffer covering the specified range. The code is optimised for
>   * cache hits, as metadata intensive workloads will see 3 orders of magnitude
> @@ -656,6 +639,8 @@ xfs_buf_get_map(
>  		goto found;
>  	if (error != -ENOENT)
>  		return error;
> +	if (flags & XBF_INCORE)
> +		return -ENOENT;
>  
>  	error = _xfs_buf_alloc(target, map, nmaps, flags, &new_bp);
>  	if (error)
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 1ee3056ff9cf..58e9034d51bd 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -42,9 +42,11 @@ struct xfs_buf;
>  #define _XBF_DELWRI_Q	 (1u << 22)/* buffer on a delwri queue */
>  
>  /* flags used only as arguments to access routines */
> +#define XBF_INCORE	 (1u << 29)/* lookup only, return if found in cache */
>  #define XBF_TRYLOCK	 (1u << 30)/* lock requested, but do not wait */
>  #define XBF_UNMAPPED	 (1u << 31)/* do not map the buffer */
>  
> +
>  typedef unsigned int xfs_buf_flags_t;
>  
>  #define XFS_BUF_FLAGS \
> @@ -63,6 +65,7 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ _XBF_KMEM,		"KMEM" }, \
>  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
>  	/* The following interface flags should never be set */ \
> +	{ XBF_INCORE,		"INCORE" }, \
>  	{ XBF_TRYLOCK,		"TRYLOCK" }, \
>  	{ XBF_UNMAPPED,		"UNMAPPED" }
>  
> @@ -196,10 +199,6 @@ struct xfs_buf {
>  };
>  
>  /* Finding and Reading Buffers */
> -struct xfs_buf *xfs_buf_incore(struct xfs_buftarg *target,
> -			   xfs_daddr_t blkno, size_t numblks,
> -			   xfs_buf_flags_t flags);
> -
>  int xfs_buf_get_map(struct xfs_buftarg *target, struct xfs_buf_map *map,
>  		int nmaps, xfs_buf_flags_t flags, struct xfs_buf **bpp);
>  int xfs_buf_read_map(struct xfs_buftarg *target, struct xfs_buf_map *map,
> @@ -209,6 +208,19 @@ void xfs_buf_readahead_map(struct xfs_buftarg *target,
>  			       struct xfs_buf_map *map, int nmaps,
>  			       const struct xfs_buf_ops *ops);
>  
> +static inline int
> +xfs_buf_incore(
> +	struct xfs_buftarg	*target,
> +	xfs_daddr_t		blkno,
> +	size_t			numblks,
> +	xfs_buf_flags_t		flags,
> +	struct xfs_buf		**bpp)
> +{
> +	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +
> +	return xfs_buf_get_map(target, &map, 1, XBF_INCORE | flags, bpp);
> +}
> +
>  static inline int
>  xfs_buf_get(
>  	struct xfs_buftarg	*target,
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index abf08bbf34a9..3517a6be8dad 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1229,12 +1229,11 @@ xfs_qm_flush_one(
>  	 */
>  	if (!xfs_dqflock_nowait(dqp)) {
>  		/* buf is pinned in-core by delwri list */
> -		bp = xfs_buf_incore(mp->m_ddev_targp, dqp->q_blkno,
> -				mp->m_quotainfo->qi_dqchunklen, 0);
> -		if (!bp) {
> -			error = -EINVAL;
> +		error = xfs_buf_incore(mp->m_ddev_targp, dqp->q_blkno,
> +				mp->m_quotainfo->qi_dqchunklen, 0, &bp);
> +		if (error)
>  			goto out_unlock;
> -		}
> +
>  		xfs_buf_unlock(bp);
>  
>  		xfs_buf_delwri_pushbuf(bp, buffer_list);
> -- 
> 2.36.1
>