Re: [PATCH 2/5] xfs: replace xfs_buf_incore with an XBF_NOALLOC flag to xfs_buf_get*

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Sun, Apr 03, 2022 at 02:01:16PM +0200, Christoph Hellwig wrote:
> Replace the special purpose xfs_buf_incore interface with a new
> XBF_NOALLOC flag for the xfs_buf_get* routines.

I think this is the wrong direction to go in the greater scheme of
things. _XBF_NOALLOC needs to be an internal implementation detail
similar to _XBF_PAGES, not exposed as part of the caller API.

That is, xfs_buf_incore() clearly documents the operation "return me
the locked buffer if it currently cached in memory" that the callers
want, while XBF_NOALLOC doesn't clearly mean anything as obvious as
this at the caller level.  Hence I'd prefer this to end up as:

/*
 * Lock and return the buffer that matches the requested range if
 * and only if it is present in the cache already.
 */
static inline 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_get_map(target, &map, 1, _XBF_NOALLOC | flags,
				NULL, &bp);
	if (error)
		return NULL;
	return bp;
}

Then none of the external callers need to be changed, and we don't
introduce new external xfs_buf_get() callers.


> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c |  6 +++---
>  fs/xfs/scrub/repair.c           |  6 +++---
>  fs/xfs/xfs_buf.c                | 22 +++-------------------
>  fs/xfs/xfs_buf.h                |  5 +----
>  fs/xfs/xfs_qm.c                 |  6 +++---
>  5 files changed, 13 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 3fc62ff92441d5..9aff2ce203c9b6 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -550,10 +550,10 @@ xfs_attr_rmtval_stale(
>  	    XFS_IS_CORRUPT(mp, map->br_startblock == HOLESTARTBLOCK))
>  		return -EFSCORRUPTED;
>  
> -	bp = xfs_buf_incore(mp->m_ddev_targp,
> +	if (!xfs_buf_get(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_FSB_TO_BB(mp, map->br_blockcount),
> +			incore_flags | XBF_NOALLOC, &bp)) {
>  		xfs_buf_stale(bp);
>  		xfs_buf_relse(bp);
>  	}

FWIW, I also think that the this pattern change is a regression.
We've spent the past decade+ moving function calls that return
objects and errors out of if() scope to clean up the code. Reversing
that pattern here doesn't make the code cleaner...

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx