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

Dave Chinner
2022-04-03T21:54:50Z

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@lst.de>
> ---
> 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@fromorbit.com