[PATCH v2 00/11] xfs: make buffer functions return error codes

[PATCH v2 00/11] xfs: make buffer functions return error codes

[Darrick J. Wong]

Hi all,

Let's fix all the xfs read/get buffer functions to return the usual
integer error codes and pass the buffer pointer as a out-argument.  This
makes it so that we can return useful error output instead of making
callers infer ENOMEM or EAGAIN or whatever other reality they crave from
the NULL pointer that they get when things don't go perfectly.

FWIW, all XBF_TRYLOCK callers must now trigger retries if they receive
EAGAIN.  This may lead to a slight behavioral change in that TRYLOCK
callers will no longer retry for things like ENOMEM, though I didn't see
any obvious changes in user-visible behavior when running fstests.

This time around I dug further into the TRYLOCK callers, though I haven't
yet rearranged the io error reporting while I determine whether there's
enough of a pattern to refactor.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

[PATCH 01/11] xfs: make xfs_buf_alloc return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert _xfs_buf_alloc() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index a0229c368e78..a00e63d08a3b 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -198,20 +198,22 @@ xfs_buf_free_maps(
 	}
 }
 
-static struct xfs_buf *
+static int
 _xfs_buf_alloc(
 	struct xfs_buftarg	*target,
 	struct xfs_buf_map	*map,
 	int			nmaps,
-	xfs_buf_flags_t		flags)
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_buf		*bp;
 	int			error;
 	int			i;
 
+	*bpp = NULL;
 	bp = kmem_zone_zalloc(xfs_buf_zone, KM_NOFS);
 	if (unlikely(!bp))
-		return NULL;
+		return -ENOMEM;
 
 	/*
 	 * We don't want certain flags to appear in b_flags unless they are
@@ -239,7 +241,7 @@ _xfs_buf_alloc(
 	error = xfs_buf_get_maps(bp, nmaps);
 	if (error)  {
 		kmem_cache_free(xfs_buf_zone, bp);
-		return NULL;
+		return error;
 	}
 
 	bp->b_bn = map[0].bm_bn;
@@ -256,7 +258,8 @@ _xfs_buf_alloc(
 	XFS_STATS_INC(bp->b_mount, xb_create);
 	trace_xfs_buf_init(bp, _RET_IP_);
 
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 /*
@@ -715,8 +718,8 @@ xfs_buf_get_map(
 		return NULL;
 	}
 
-	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
-	if (unlikely(!new_bp))
+	error = _xfs_buf_alloc(target, map, nmaps, flags, &new_bp);
+	if (unlikely(error))
 		return NULL;
 
 	error = xfs_buf_allocate_memory(new_bp, flags);
@@ -917,8 +920,8 @@ xfs_buf_get_uncached(
 	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
 
 	/* flags might contain irrelevant bits, pass only what we care about */
-	bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT);
-	if (unlikely(bp == NULL))
+	error = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT, &bp);
+	if (unlikely(error))
 		goto fail;
 
 	page_count = PAGE_ALIGN(numblks << BBSHIFT) >> PAGE_SHIFT;

[PATCH 02/11] xfs: make xfs_buf_read return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_buf_read() to return numeric error codes like most
everywhere else in xfs.  Hoist the callers' error logging and EFSBADCRC
remapping code into xfs_buf_read to reduce code duplication.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr_remote.c |   16 +++-------------
 fs/xfs/xfs_buf.c                |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/xfs_buf.h                |   14 +++-----------
 fs/xfs/xfs_log_recover.c        |   26 +++++++-------------------
 fs/xfs/xfs_symlink.c            |   17 ++++-------------
 5 files changed, 50 insertions(+), 56 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index a266d05df146..46c516809086 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -418,20 +418,10 @@ xfs_attr_rmtval_get(
 			       (map[i].br_startblock != HOLESTARTBLOCK));
 			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
 			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
-			bp = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt, 0,
-					&xfs_attr3_rmt_buf_ops);
-			if (!bp)
-				return -ENOMEM;
-			error = bp->b_error;
-			if (error) {
-				xfs_buf_ioerror_alert(bp, __func__);
-				xfs_buf_relse(bp);
-
-				/* bad CRC means corrupted metadata */
-				if (error == -EFSBADCRC)
-					error = -EFSCORRUPTED;
+			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
+					0, &bp, &xfs_attr3_rmt_buf_ops);
+			if (error)
 				return error;
-			}
 
 			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
 							&offset, &valuelen,
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index a00e63d08a3b..8c9cd1ab870b 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -851,6 +851,39 @@ xfs_buf_read_map(
 	return bp;
 }
 
+int
+xfs_buf_read(
+	struct xfs_buftarg	*target,
+	xfs_daddr_t		blkno,
+	size_t			numblks,
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp,
+	const struct xfs_buf_ops *ops)
+{
+	struct xfs_buf		*bp;
+	int			error;
+	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+
+	*bpp = NULL;
+	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
+	if (!bp)
+		return -ENOMEM;
+	error = bp->b_error;
+	if (error) {
+		xfs_buf_ioerror_alert(bp, __func__);
+		xfs_buf_stale(bp);
+		xfs_buf_relse(bp);
+
+		/* bad CRC means corrupted metadata */
+		if (error == -EFSBADCRC)
+			error = -EFSCORRUPTED;
+		return error;
+	}
+
+	*bpp = bp;
+	return 0;
+}
+
 /*
  *	If we are not low on memory then do the readahead in a deadlock
  *	safe manner.
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 56e081dd1d96..f04722554f63 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -213,17 +213,9 @@ xfs_buf_get(
 	return xfs_buf_get_map(target, &map, 1, 0);
 }
 
-static inline struct xfs_buf *
-xfs_buf_read(
-	struct xfs_buftarg	*target,
-	xfs_daddr_t		blkno,
-	size_t			numblks,
-	xfs_buf_flags_t		flags,
-	const struct xfs_buf_ops *ops)
-{
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return xfs_buf_read_map(target, &map, 1, flags, ops);
-}
+int xfs_buf_read(struct xfs_buftarg *target, xfs_daddr_t blkno, size_t numblks,
+		xfs_buf_flags_t flags, struct xfs_buf **bpp,
+		const struct xfs_buf_ops *ops);
 
 static inline void
 xfs_buf_readahead(
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0d683fb96396..ac79537d3275 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2745,15 +2745,10 @@ xlog_recover_buffer_pass2(
 	if (buf_f->blf_flags & XFS_BLF_INODE_BUF)
 		buf_flags |= XBF_UNMAPPED;
 
-	bp = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
-			  buf_flags, NULL);
-	if (!bp)
-		return -ENOMEM;
-	error = bp->b_error;
-	if (error) {
-		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)");
-		goto out_release;
-	}
+	error = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
+			  buf_flags, &bp, NULL);
+	if (error)
+		return error;
 
 	/*
 	 * Recover the buffer only if we get an LSN from it and it's less than
@@ -2950,17 +2945,10 @@ xlog_recover_inode_pass2(
 	}
 	trace_xfs_log_recover_inode_recover(log, in_f);
 
-	bp = xfs_buf_read(mp->m_ddev_targp, in_f->ilf_blkno, in_f->ilf_len, 0,
-			  &xfs_inode_buf_ops);
-	if (!bp) {
-		error = -ENOMEM;
+	error = xfs_buf_read(mp->m_ddev_targp, in_f->ilf_blkno, in_f->ilf_len,
+			0, &bp, &xfs_inode_buf_ops);
+	if (error)
 		goto error;
-	}
-	error = bp->b_error;
-	if (error) {
-		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#2)");
-		goto out_release;
-	}
 	ASSERT(in_f->ilf_fields & XFS_ILOG_CORE);
 	dip = xfs_buf_offset(bp, in_f->ilf_boffset);
 
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index a25502bc2071..f4b14569039f 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -53,20 +53,11 @@ xfs_readlink_bmap_ilocked(
 		d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
 		byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
 
-		bp = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt), 0,
-				  &xfs_symlink_buf_ops);
-		if (!bp)
-			return -ENOMEM;
-		error = bp->b_error;
-		if (error) {
-			xfs_buf_ioerror_alert(bp, __func__);
-			xfs_buf_relse(bp);
-
-			/* bad CRC means corrupted metadata */
-			if (error == -EFSBADCRC)
-				error = -EFSCORRUPTED;
+		error = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt), 0,
+				&bp, &xfs_symlink_buf_ops);
+		if (error)
 			goto out;
-		}
+
 		byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
 		if (pathlen < byte_cnt)
 			byte_cnt = pathlen;

[PATCH 03/11] xfs: make xfs_buf_get return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_buf_get() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr_remote.c |    6 +++---
 fs/xfs/libxfs/xfs_sb.c          |    8 ++++----
 fs/xfs/xfs_buf.h                |   15 ++++++++++++---
 3 files changed, 19 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 46c516809086..8b7f74b3bea2 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -545,9 +545,9 @@ xfs_attr_rmtval_set(
 		dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock),
 		dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount);
 
-		bp = xfs_buf_get(mp->m_ddev_targp, dblkno, dblkcnt);
-		if (!bp)
-			return -ENOMEM;
+		error = xfs_buf_get(mp->m_ddev_targp, dblkno, dblkcnt, &bp);
+		if (error)
+			return error;
 		bp->b_ops = &xfs_attr3_rmt_buf_ops;
 
 		xfs_attr_rmtval_copyin(mp, bp, args->dp->i_ino, &offset,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 0ac69751fe85..6fdd007f81ab 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -985,9 +985,9 @@ xfs_update_secondary_sbs(
 	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
 		struct xfs_buf		*bp;
 
-		bp = xfs_buf_get(mp->m_ddev_targp,
+		error = xfs_buf_get(mp->m_ddev_targp,
 				 XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
-				 XFS_FSS_TO_BB(mp, 1));
+				 XFS_FSS_TO_BB(mp, 1), &bp);
 		/*
 		 * If we get an error reading or writing alternate superblocks,
 		 * continue.  xfs_repair chooses the "best" superblock based
@@ -995,12 +995,12 @@ xfs_update_secondary_sbs(
 		 * superblocks un-updated than updated, and xfs_repair may
 		 * pick them over the properly-updated primary.
 		 */
-		if (!bp) {
+		if (error) {
 			xfs_warn(mp,
 		"error allocating secondary superblock for ag %d",
 				agno);
 			if (!saved_error)
-				saved_error = -ENOMEM;
+				saved_error = error;
 			continue;
 		}
 
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index f04722554f63..f957d734e4b6 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -203,14 +203,23 @@ void xfs_buf_readahead_map(struct xfs_buftarg *target,
 			       struct xfs_buf_map *map, int nmaps,
 			       const struct xfs_buf_ops *ops);
 
-static inline struct xfs_buf *
+static inline int
 xfs_buf_get(
 	struct xfs_buftarg	*target,
 	xfs_daddr_t		blkno,
-	size_t			numblks)
+	size_t			numblks,
+	struct xfs_buf		**bpp)
 {
+	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return xfs_buf_get_map(target, &map, 1, 0);
+
+	*bpp = NULL;
+	bp = xfs_buf_get_map(target, &map, 1, 0);
+	if (!bp)
+		return -ENOMEM;
+
+	*bpp = bp;
+	return 0;
 }
 
 int xfs_buf_read(struct xfs_buftarg *target, xfs_daddr_t blkno, size_t numblks,

[PATCH 04/11] xfs: make xfs_buf_get_uncached return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_buf_get_uncached() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_ag.c |   21 ++++++++++++---------
 fs/xfs/xfs_buf.c       |   25 ++++++++++++++++---------
 fs/xfs/xfs_buf.h       |    4 ++--
 3 files changed, 30 insertions(+), 20 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 14fbdf22b7e7..08d6beb54f8c 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -23,25 +23,28 @@
 #include "xfs_ag_resv.h"
 #include "xfs_health.h"
 
-static struct xfs_buf *
+static int
 xfs_get_aghdr_buf(
 	struct xfs_mount	*mp,
 	xfs_daddr_t		blkno,
 	size_t			numblks,
+	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
-	bp = xfs_buf_get_uncached(mp->m_ddev_targp, numblks, 0);
-	if (!bp)
-		return NULL;
+	error = xfs_buf_get_uncached(mp->m_ddev_targp, numblks, 0, &bp);
+	if (error)
+		return error;
 
 	xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
 	bp->b_bn = blkno;
 	bp->b_maps[0].bm_bn = blkno;
 	bp->b_ops = ops;
 
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 static inline bool is_log_ag(struct xfs_mount *mp, struct aghdr_init_data *id)
@@ -340,13 +343,13 @@ xfs_ag_init_hdr(
 	struct aghdr_init_data	*id,
 	aghdr_init_work_f	work,
 	const struct xfs_buf_ops *ops)
-
 {
 	struct xfs_buf		*bp;
+	int			error;
 
-	bp = xfs_get_aghdr_buf(mp, id->daddr, id->numblks, ops);
-	if (!bp)
-		return -ENOMEM;
+	error = xfs_get_aghdr_buf(mp, id->daddr, id->numblks, &bp, ops);
+	if (error)
+		return error;
 
 	(*work)(mp, bp, id);
 
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 8c9cd1ab870b..8778c4b38af7 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -916,12 +916,13 @@ xfs_buf_read_uncached(
 	const struct xfs_buf_ops *ops)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
 	*bpp = NULL;
 
-	bp = xfs_buf_get_uncached(target, numblks, flags);
-	if (!bp)
-		return -ENOMEM;
+	error = xfs_buf_get_uncached(target, numblks, flags, &bp);
+	if (error)
+		return error;
 
 	/* set up the buffer for a read IO */
 	ASSERT(bp->b_map_count == 1);
@@ -932,7 +933,7 @@ xfs_buf_read_uncached(
 
 	xfs_buf_submit(bp);
 	if (bp->b_error) {
-		int	error = bp->b_error;
+		error = bp->b_error;
 		xfs_buf_relse(bp);
 		return error;
 	}
@@ -941,17 +942,20 @@ xfs_buf_read_uncached(
 	return 0;
 }
 
-xfs_buf_t *
+int
 xfs_buf_get_uncached(
 	struct xfs_buftarg	*target,
 	size_t			numblks,
-	int			flags)
+	int			flags,
+	struct xfs_buf		**bpp)
 {
 	unsigned long		page_count;
 	int			error, i;
 	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
 
+	*bpp = NULL;
+
 	/* flags might contain irrelevant bits, pass only what we care about */
 	error = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT, &bp);
 	if (unlikely(error))
@@ -964,8 +968,10 @@ xfs_buf_get_uncached(
 
 	for (i = 0; i < page_count; i++) {
 		bp->b_pages[i] = alloc_page(xb_to_gfp(flags));
-		if (!bp->b_pages[i])
+		if (!bp->b_pages[i]) {
+			error = -ENOMEM;
 			goto fail_free_mem;
+		}
 	}
 	bp->b_flags |= _XBF_PAGES;
 
@@ -977,7 +983,8 @@ xfs_buf_get_uncached(
 	}
 
 	trace_xfs_buf_get_uncached(bp, _RET_IP_);
-	return bp;
+	*bpp = bp;
+	return 0;
 
  fail_free_mem:
 	while (--i >= 0)
@@ -987,7 +994,7 @@ xfs_buf_get_uncached(
 	xfs_buf_free_maps(bp);
 	kmem_cache_free(xfs_buf_zone, bp);
  fail:
-	return NULL;
+	return error;
 }
 
 /*
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index f957d734e4b6..1ee516ef2c66 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -237,8 +237,8 @@ xfs_buf_readahead(
 	return xfs_buf_readahead_map(target, &map, 1, ops);
 }
 
-struct xfs_buf *xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks,
-				int flags);
+int xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks, int flags,
+		struct xfs_buf **bpp);
 int xfs_buf_read_uncached(struct xfs_buftarg *target, xfs_daddr_t daddr,
 			  size_t numblks, int flags, struct xfs_buf **bpp,
 			  const struct xfs_buf_ops *ops);

[PATCH 05/11] xfs: make xfs_buf_read_map return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_buf_read_map() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |    4 ++--
 fs/xfs/xfs_buf.c          |   37 +++++++++++++++++++++----------------
 fs/xfs/xfs_buf.h          |   12 +++++-------
 fs/xfs/xfs_trans_buf.c    |    9 +++------
 4 files changed, 31 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fc93fd88ec89..51818245eae2 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2960,10 +2960,10 @@ xfs_read_agf(
 			mp, tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
 			XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops);
+	if (error == -EAGAIN)
+		return 0;
 	if (error)
 		return error;
-	if (!*bpp)
-		return 0;
 
 	ASSERT(!(*bpp)->b_error);
 	xfs_buf_set_ref(*bpp, XFS_AGF_REF);
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 8778c4b38af7..f95c999b3343 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -809,21 +809,23 @@ xfs_buf_reverify(
 	return bp->b_error;
 }
 
-xfs_buf_t *
+int
 xfs_buf_read_map(
 	struct xfs_buftarg	*target,
 	struct xfs_buf_map	*map,
 	int			nmaps,
 	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
 	struct xfs_buf		*bp;
 
 	flags |= XBF_READ;
+	*bpp = NULL;
 
 	bp = xfs_buf_get_map(target, map, nmaps, flags);
 	if (!bp)
-		return NULL;
+		return -ENOMEM;
 
 	trace_xfs_buf_read(bp, flags, _RET_IP_);
 
@@ -831,7 +833,8 @@ xfs_buf_read_map(
 		XFS_STATS_INC(target->bt_mount, xb_get_read);
 		bp->b_ops = ops;
 		_xfs_buf_read(bp, flags);
-		return bp;
+		*bpp = bp;
+		return 0;
 	}
 
 	xfs_buf_reverify(bp, ops);
@@ -842,13 +845,15 @@ xfs_buf_read_map(
 		 * drop the buffer
 		 */
 		xfs_buf_relse(bp);
-		return NULL;
+		*bpp = NULL;
+		return 0;
 	}
 
 	/* We do not want read in the flags */
 	bp->b_flags &= ~XBF_READ;
 	ASSERT(bp->b_ops != NULL || ops == NULL);
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 int
@@ -860,19 +865,18 @@ xfs_buf_read(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	struct xfs_buf		*bp;
 	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	*bpp = NULL;
-	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
-	if (!bp)
-		return -ENOMEM;
-	error = bp->b_error;
+	error = xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
+	if (error)
+		return error;
+	error = (*bpp)->b_error;
 	if (error) {
-		xfs_buf_ioerror_alert(bp, __func__);
-		xfs_buf_stale(bp);
-		xfs_buf_relse(bp);
+		xfs_buf_ioerror_alert(*bpp, __func__);
+		xfs_buf_stale(*bpp);
+		xfs_buf_relse(*bpp);
+		*bpp = NULL;
 
 		/* bad CRC means corrupted metadata */
 		if (error == -EFSBADCRC)
@@ -880,7 +884,6 @@ xfs_buf_read(
 		return error;
 	}
 
-	*bpp = bp;
 	return 0;
 }
 
@@ -895,11 +898,13 @@ xfs_buf_readahead_map(
 	int			nmaps,
 	const struct xfs_buf_ops *ops)
 {
+	struct xfs_buf		*bp;
+
 	if (bdi_read_congested(target->bt_bdev->bd_bdi))
 		return;
 
 	xfs_buf_read_map(target, map, nmaps,
-		     XBF_TRYLOCK|XBF_ASYNC|XBF_READ_AHEAD, ops);
+		     XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops);
 }
 
 /*
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 1ee516ef2c66..36d43bead158 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -195,13 +195,11 @@ struct xfs_buf *xfs_buf_incore(struct xfs_buftarg *target,
 struct xfs_buf *xfs_buf_get_map(struct xfs_buftarg *target,
 			       struct xfs_buf_map *map, int nmaps,
 			       xfs_buf_flags_t flags);
-struct xfs_buf *xfs_buf_read_map(struct xfs_buftarg *target,
-			       struct xfs_buf_map *map, int nmaps,
-			       xfs_buf_flags_t flags,
-			       const struct xfs_buf_ops *ops);
-void xfs_buf_readahead_map(struct xfs_buftarg *target,
-			       struct xfs_buf_map *map, int nmaps,
-			       const struct xfs_buf_ops *ops);
+int xfs_buf_read_map(struct xfs_buftarg *target, struct xfs_buf_map *map,
+		int nmaps, xfs_buf_flags_t flags, struct xfs_buf **bpp,
+		const struct xfs_buf_ops *ops);
+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_get(
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index b5b3a78ef31c..a2af6dec310d 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -298,12 +298,9 @@ xfs_trans_read_buf_map(
 		return 0;
 	}
 
-	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (!bp) {
-		if (!(flags & XBF_TRYLOCK))
-			return -ENOMEM;
-		return tp ? 0 : -EAGAIN;
-	}
+	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
+	if (error)
+		return error;
 
 	/*
 	 * If we've had a read error, then the contents of the buffer are

[PATCH 06/11] xfs: make xfs_buf_get_map return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_buf_get_map() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c       |   46 +++++++++++++++++-----------------------------
 fs/xfs/xfs_buf.h       |   14 +++-----------
 fs/xfs/xfs_trans_buf.c |   14 +++++++++-----
 3 files changed, 29 insertions(+), 45 deletions(-)


diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index f95c999b3343..34b0783587f4 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -685,53 +685,39 @@ xfs_buf_incore(
  * cache hits, as metadata intensive workloads will see 3 orders of magnitude
  * more hits than misses.
  */
-struct xfs_buf *
+int
 xfs_buf_get_map(
 	struct xfs_buftarg	*target,
 	struct xfs_buf_map	*map,
 	int			nmaps,
-	xfs_buf_flags_t		flags)
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_buf		*bp;
 	struct xfs_buf		*new_bp;
 	int			error = 0;
 
+	*bpp = NULL;
 	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
-
-	switch (error) {
-	case 0:
-		/* cache hit */
+	if (!error)
 		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;
-	}
+	if (error != -ENOENT)
+		return error;
 
 	error = _xfs_buf_alloc(target, map, nmaps, flags, &new_bp);
 	if (unlikely(error))
-		return NULL;
+		return error;
 
 	error = xfs_buf_allocate_memory(new_bp, flags);
 	if (error) {
 		xfs_buf_free(new_bp);
-		return NULL;
+		return error;
 	}
 
 	error = xfs_buf_find(target, map, nmaps, flags, new_bp, &bp);
 	if (error) {
 		xfs_buf_free(new_bp);
-		return NULL;
+		return error;
 	}
 
 	if (bp != new_bp)
@@ -744,7 +730,7 @@ xfs_buf_get_map(
 			xfs_warn(target->bt_mount,
 				"%s: failed to map pagesn", __func__);
 			xfs_buf_relse(bp);
-			return NULL;
+			return error;
 		}
 	}
 
@@ -757,7 +743,8 @@ xfs_buf_get_map(
 
 	XFS_STATS_INC(target->bt_mount, xb_get);
 	trace_xfs_buf_get(bp, flags, _RET_IP_);
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 STATIC int
@@ -819,13 +806,14 @@ xfs_buf_read_map(
 	const struct xfs_buf_ops *ops)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
 	flags |= XBF_READ;
 	*bpp = NULL;
 
-	bp = xfs_buf_get_map(target, map, nmaps, flags);
-	if (!bp)
-		return -ENOMEM;
+	error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
+	if (error)
+		return error;
 
 	trace_xfs_buf_read(bp, flags, _RET_IP_);
 
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 36d43bead158..8e7c1238ddf0 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -192,9 +192,8 @@ 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 *xfs_buf_get_map(struct xfs_buftarg *target,
-			       struct xfs_buf_map *map, int nmaps,
-			       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,
 		int nmaps, xfs_buf_flags_t flags, struct xfs_buf **bpp,
 		const struct xfs_buf_ops *ops);
@@ -208,16 +207,9 @@ xfs_buf_get(
 	size_t			numblks,
 	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	*bpp = NULL;
-	bp = xfs_buf_get_map(target, &map, 1, 0);
-	if (!bp)
-		return -ENOMEM;
-
-	*bpp = bp;
-	return 0;
+	return xfs_buf_get_map(target, &map, 1, 0, bpp);
 }
 
 int xfs_buf_read(struct xfs_buftarg *target, xfs_daddr_t blkno, size_t numblks,
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index a2af6dec310d..bf61e61b38c7 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -122,9 +122,14 @@ xfs_trans_get_buf_map(
 {
 	xfs_buf_t		*bp;
 	struct xfs_buf_log_item	*bip;
+	int			error;
 
-	if (!tp)
-		return xfs_buf_get_map(target, map, nmaps, flags);
+	if (!tp) {
+		error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
+		if (error)
+			return NULL;
+		return bp;
+	}
 
 	/*
 	 * If we find the buffer in the cache with this transaction
@@ -149,10 +154,9 @@ xfs_trans_get_buf_map(
 		return bp;
 	}
 
-	bp = xfs_buf_get_map(target, map, nmaps, flags);
-	if (bp == NULL) {
+	error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
+	if (error)
 		return NULL;
-	}
 
 	ASSERT(!bp->b_error);
 

[PATCH 07/11] xfs: make xfs_trans_get_buf_map return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_trans_get_buf_map() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_da_btree.c |    8 ++------
 fs/xfs/xfs_trans.h           |   15 ++++++++++-----
 fs/xfs/xfs_trans_buf.c       |   22 +++++++++++-----------
 3 files changed, 23 insertions(+), 22 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 8c3eafe280ed..875e04f82541 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -2591,13 +2591,9 @@ xfs_da_get_buf(
 	if (error || nmap == 0)
 		goto out_free;
 
-	bp = xfs_trans_get_buf_map(tp, mp->m_ddev_targp, mapp, nmap, 0);
-	error = bp ? bp->b_error : -EIO;
-	if (error) {
-		if (bp)
-			xfs_trans_brelse(tp, bp);
+	error = xfs_trans_get_buf_map(tp, mp->m_ddev_targp, mapp, nmap, 0, &bp);
+	if (error)
 		goto out_free;
-	}
 
 	*bpp = bp;
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 64d7f171ebd3..a0be934ec811 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -169,10 +169,9 @@ int		xfs_trans_alloc_empty(struct xfs_mount *mp,
 			struct xfs_trans **tpp);
 void		xfs_trans_mod_sb(xfs_trans_t *, uint, int64_t);
 
-struct xfs_buf	*xfs_trans_get_buf_map(struct xfs_trans *tp,
-				       struct xfs_buftarg *target,
-				       struct xfs_buf_map *map, int nmaps,
-				       uint flags);
+int xfs_trans_get_buf_map(struct xfs_trans *tp, struct xfs_buftarg *target,
+		struct xfs_buf_map *map, int nmaps, xfs_buf_flags_t flags,
+		struct xfs_buf **bpp);
 
 static inline struct xfs_buf *
 xfs_trans_get_buf(
@@ -182,8 +181,14 @@ xfs_trans_get_buf(
 	int			numblks,
 	uint			flags)
 {
+	struct xfs_buf		*bp;
+	int			error;
+
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return xfs_trans_get_buf_map(tp, target, &map, 1, flags);
+	error = xfs_trans_get_buf_map(tp, target, &map, 1, flags, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 int		xfs_trans_read_buf_map(struct xfs_mount *mp,
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index bf61e61b38c7..babbfddbe505 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -112,24 +112,22 @@ xfs_trans_bjoin(
  * If the transaction pointer is NULL, make this just a normal
  * get_buf() call.
  */
-struct xfs_buf *
+int
 xfs_trans_get_buf_map(
 	struct xfs_trans	*tp,
 	struct xfs_buftarg	*target,
 	struct xfs_buf_map	*map,
 	int			nmaps,
-	xfs_buf_flags_t		flags)
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp)
 {
 	xfs_buf_t		*bp;
 	struct xfs_buf_log_item	*bip;
 	int			error;
 
-	if (!tp) {
-		error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
-		if (error)
-			return NULL;
-		return bp;
-	}
+	*bpp = NULL;
+	if (!tp)
+		return xfs_buf_get_map(target, map, nmaps, flags, bpp);
 
 	/*
 	 * If we find the buffer in the cache with this transaction
@@ -151,18 +149,20 @@ xfs_trans_get_buf_map(
 		ASSERT(atomic_read(&bip->bli_refcount) > 0);
 		bip->bli_recur++;
 		trace_xfs_trans_get_buf_recur(bip);
-		return bp;
+		*bpp = bp;
+		return 0;
 	}
 
 	error = xfs_buf_get_map(target, map, nmaps, flags, &bp);
 	if (error)
-		return NULL;
+		return error;
 
 	ASSERT(!bp->b_error);
 
 	_xfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_get_buf(bp->b_log_item);
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 /*

[PATCH 08/11] xfs: make xfs_trans_get_buf return an error code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert xfs_trans_get_buf() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_btree.c  |   23 ++++++++++++++++-------
 fs/xfs/libxfs/xfs_ialloc.c |   12 ++++++------
 fs/xfs/libxfs/xfs_sb.c     |    9 +++++----
 fs/xfs/scrub/repair.c      |    8 ++++++--
 fs/xfs/xfs_attr_inactive.c |   17 +++++++++--------
 fs/xfs/xfs_dquot.c         |    8 ++++----
 fs/xfs/xfs_inode.c         |   12 ++++++------
 fs/xfs/xfs_rtalloc.c       |    8 +++-----
 fs/xfs/xfs_symlink.c       |   19 ++++++++-----------
 fs/xfs/xfs_trans.h         |   13 ++++---------
 10 files changed, 67 insertions(+), 62 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index b22c7e928eb1..2d53e5fdff70 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -688,11 +688,16 @@ xfs_btree_get_bufl(
 	xfs_trans_t	*tp,		/* transaction pointer */
 	xfs_fsblock_t	fsbno)		/* file system block number */
 {
+	struct xfs_buf		*bp;
 	xfs_daddr_t		d;		/* real disk block address */
+	int			error;
 
 	ASSERT(fsbno != NULLFSBLOCK);
 	d = XFS_FSB_TO_DADDR(mp, fsbno);
-	return xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0);
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -706,12 +711,17 @@ xfs_btree_get_bufs(
 	xfs_agnumber_t	agno,		/* allocation group number */
 	xfs_agblock_t	agbno)		/* allocation group block number */
 {
+	struct xfs_buf		*bp;
 	xfs_daddr_t		d;		/* real disk block address */
+	int			error;
 
 	ASSERT(agno != NULLAGNUMBER);
 	ASSERT(agbno != NULLAGBLOCK);
 	d = XFS_AGB_TO_DADDR(mp, agno, agbno);
-	return xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0);
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -1270,11 +1280,10 @@ xfs_btree_get_buf_block(
 	error = xfs_btree_ptr_to_daddr(cur, ptr, &d);
 	if (error)
 		return error;
-	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
-				 mp->m_bsize, 0);
-
-	if (!*bpp)
-		return -ENOMEM;
+	error = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d, mp->m_bsize,
+			0, bpp);
+	if (error)
+		return error;
 
 	(*bpp)->b_ops = cur->bc_ops->buf_ops;
 	*block = XFS_BUF_TO_BLOCK(*bpp);
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 5b759af4d165..bf161e930f1d 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -276,6 +276,7 @@ xfs_ialloc_inode_init(
 	int			i, j;
 	xfs_daddr_t		d;
 	xfs_ino_t		ino = 0;
+	int			error;
 
 	/*
 	 * Loop over the new block(s), filling in the inodes.  For small block
@@ -327,12 +328,11 @@ xfs_ialloc_inode_init(
 		 */
 		d = XFS_AGB_TO_DADDR(mp, agno, agbno +
 				(j * M_IGEO(mp)->blocks_per_cluster));
-		fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
-					 mp->m_bsize *
-					 M_IGEO(mp)->blocks_per_cluster,
-					 XBF_UNMAPPED);
-		if (!fbuf)
-			return -ENOMEM;
+		error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
+				mp->m_bsize * M_IGEO(mp)->blocks_per_cluster,
+				XBF_UNMAPPED, &fbuf);
+		if (error)
+			return error;
 
 		/* Initialize the inode buffers and log them appropriately. */
 		fbuf->b_ops = &xfs_inode_buf_ops;
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 6fdd007f81ab..2f60fc3c99a0 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1185,13 +1185,14 @@ xfs_sb_get_secondary(
 	struct xfs_buf		**bpp)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
 	ASSERT(agno != 0 && agno != NULLAGNUMBER);
-	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-			XFS_FSS_TO_BB(mp, 1), 0);
-	if (!bp)
-		return -ENOMEM;
+			XFS_FSS_TO_BB(mp, 1), 0, &bp);
+	if (error)
+		return error;
 	bp->b_ops = &xfs_sb_buf_ops;
 	xfs_buf_oneshot(bp);
 	*bpp = bp;
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index b70a88bc975e..3df49d487940 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -341,13 +341,17 @@ xrep_init_btblock(
 	struct xfs_trans		*tp = sc->tp;
 	struct xfs_mount		*mp = sc->mp;
 	struct xfs_buf			*bp;
+	int				error;
 
 	trace_xrep_init_btblock(mp, XFS_FSB_TO_AGNO(mp, fsb),
 			XFS_FSB_TO_AGBNO(mp, fsb), btnum);
 
 	ASSERT(XFS_FSB_TO_AGNO(mp, fsb) == sc->sa.agno);
-	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, XFS_FSB_TO_DADDR(mp, fsb),
-			XFS_FSB_TO_BB(mp, 1), 0);
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(mp, fsb), XFS_FSB_TO_BB(mp, 1), 0,
+			&bp);
+	if (error)
+		return error;
 	xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
 	xfs_btree_init_block(mp, bp, btnum, 0, 0, sc->sa.agno);
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_BTREE_BUF);
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index c75840a9e478..eddd5d311b0c 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -205,11 +205,12 @@ xfs_attr3_node_inactive(
 		/*
 		 * Remove the subsidiary block from the cache and from the log.
 		 */
-		child_bp = xfs_trans_get_buf(*trans, mp->m_ddev_targp,
+		error = xfs_trans_get_buf(*trans, mp->m_ddev_targp,
 				child_blkno,
-				XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0);
-		if (!child_bp)
-			return -EIO;
+				XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0,
+				&child_bp);
+		if (error)
+			return error;
 		error = bp->b_error;
 		if (error) {
 			xfs_trans_brelse(*trans, child_bp);
@@ -298,10 +299,10 @@ xfs_attr3_root_inactive(
 	/*
 	 * Invalidate the incore copy of the root block.
 	 */
-	bp = xfs_trans_get_buf(*trans, mp->m_ddev_targp, blkno,
-			XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0);
-	if (!bp)
-		return -EIO;
+	error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, blkno,
+			XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, &bp);
+	if (error)
+		return error;
 	error = bp->b_error;
 	if (error) {
 		xfs_trans_brelse(*trans, bp);
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 9cfd3209f52b..d223e1ae90a6 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -320,10 +320,10 @@ xfs_dquot_disk_alloc(
 	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
 
 	/* now we can just get the buffer (there's nothing to read yet) */
-	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno,
-			mp->m_quotainfo->qi_dqchunklen, 0);
-	if (!bp)
-		return -ENOMEM;
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno,
+			mp->m_quotainfo->qi_dqchunklen, 0, &bp);
+	if (error)
+		return error;
 	bp->b_ops = &xfs_dquot_buf_ops;
 
 	/*
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 1979a0055763..c5077e6326c7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2546,6 +2546,7 @@ xfs_ifree_cluster(
 	struct xfs_perag	*pag;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
 	xfs_ino_t		inum;
+	int			error;
 
 	inum = xic->first_ino;
 	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, inum));
@@ -2574,12 +2575,11 @@ xfs_ifree_cluster(
 		 * complete before we get a lock on it, and hence we may fail
 		 * to mark all the active inodes on the buffer stale.
 		 */
-		bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
-					mp->m_bsize * igeo->blocks_per_cluster,
-					XBF_UNMAPPED);
-
-		if (!bp)
-			return -ENOMEM;
+		error = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
+				mp->m_bsize * igeo->blocks_per_cluster,
+				XBF_UNMAPPED, &bp);
+		if (error)
+			return error;
 
 		/*
 		 * This buffer may not have been correctly initialised as we
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index d42b5a2047e0..6209e7b6b895 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -826,12 +826,10 @@ xfs_growfs_rt_alloc(
 			 * Get a buffer for the block.
 			 */
 			d = XFS_FSB_TO_DADDR(mp, fsbno);
-			bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
-				mp->m_bsize, 0);
-			if (bp == NULL) {
-				error = -EIO;
+			error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
+					mp->m_bsize, 0, &bp);
+			if (error)
 				goto out_trans_cancel;
-			}
 			memset(bp->b_addr, 0, mp->m_sb.sb_blocksize);
 			xfs_trans_log_buf(tp, bp, 0, mp->m_sb.sb_blocksize - 1);
 			/*
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index f4b14569039f..2c879d0948e6 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -281,12 +281,10 @@ xfs_symlink(
 
 			d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
 			byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
-			bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
-					       BTOBB(byte_cnt), 0);
-			if (!bp) {
-				error = -ENOMEM;
+			error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
+					       BTOBB(byte_cnt), 0, &bp);
+			if (error)
 				goto out_trans_cancel;
-			}
 			bp->b_ops = &xfs_symlink_buf_ops;
 
 			byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
@@ -424,13 +422,12 @@ xfs_inactive_symlink_rmt(
 	 * Invalidate the block(s). No validation is done.
 	 */
 	for (i = 0; i < nmaps; i++) {
-		bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
-			XFS_FSB_TO_DADDR(mp, mval[i].br_startblock),
-			XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
-		if (!bp) {
-			error = -ENOMEM;
+		error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+				XFS_FSB_TO_DADDR(mp, mval[i].br_startblock),
+				XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0,
+				&bp);
+		if (error)
 			goto error_trans_cancel;
-		}
 		xfs_trans_binval(tp, bp);
 	}
 	/*
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index a0be934ec811..752c7fef9de7 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -173,22 +173,17 @@ int xfs_trans_get_buf_map(struct xfs_trans *tp, struct xfs_buftarg *target,
 		struct xfs_buf_map *map, int nmaps, xfs_buf_flags_t flags,
 		struct xfs_buf **bpp);
 
-static inline struct xfs_buf *
+static inline int
 xfs_trans_get_buf(
 	struct xfs_trans	*tp,
 	struct xfs_buftarg	*target,
 	xfs_daddr_t		blkno,
 	int			numblks,
-	uint			flags)
+	uint			flags,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
-	int			error;
-
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	error = xfs_trans_get_buf_map(tp, target, &map, 1, flags, &bp);
-	if (error)
-		return NULL;
-	return bp;
+	return xfs_trans_get_buf_map(tp, target, &map, 1, flags, bpp);
 }
 
 int		xfs_trans_read_buf_map(struct xfs_mount *mp,

[PATCH 09/11] xfs: remove the xfs_btree_get_buf[ls] functions

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Remove the xfs_btree_get_bufs and xfs_btree_get_bufl functions, since
they're pretty trivial oneliners.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |   16 +++++++++-------
 fs/xfs/libxfs/xfs_bmap.c  |   14 +++++++++-----
 fs/xfs/libxfs/xfs_btree.c |   46 ---------------------------------------------
 fs/xfs/libxfs/xfs_btree.h |   21 ---------------------
 4 files changed, 18 insertions(+), 79 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 51818245eae2..83273975df77 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1070,11 +1070,11 @@ xfs_alloc_ag_vextent_small(
 	if (args->datatype & XFS_ALLOC_USERDATA) {
 		struct xfs_buf	*bp;
 
-		bp = xfs_btree_get_bufs(args->mp, args->tp, args->agno, fbno);
-		if (XFS_IS_CORRUPT(args->mp, !bp)) {
-			error = -EFSCORRUPTED;
+		error = xfs_trans_get_buf(args->tp, args->mp->m_ddev_targp,
+				XFS_AGB_TO_DADDR(args->mp, args->agno, fbno),
+				args->mp->m_bsize, 0, &bp);
+		if (error)
 			goto error;
-		}
 		xfs_trans_binval(args->tp, bp);
 	}
 	*fbnop = args->agbno = fbno;
@@ -2347,9 +2347,11 @@ xfs_free_agfl_block(
 	if (error)
 		return error;
 
-	bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno);
-	if (XFS_IS_CORRUPT(tp->t_mountp, !bp))
-		return -EFSCORRUPTED;
+	error = xfs_trans_get_buf(tp, tp->t_mountp->m_ddev_targp,
+			XFS_AGB_TO_DADDR(tp->t_mountp, agno, agbno),
+			tp->t_mountp->m_bsize, 0, &bp);
+	if (error)
+		return error;
 	xfs_trans_binval(tp, bp);
 
 	return 0;
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 4c2e046fbfad..cfcef076c72f 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -730,11 +730,11 @@ xfs_bmap_extents_to_btree(
 	cur->bc_private.b.allocated++;
 	ip->i_d.di_nblocks++;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
-	abp = xfs_btree_get_bufl(mp, tp, args.fsbno);
-	if (XFS_IS_CORRUPT(mp, !abp)) {
-		error = -EFSCORRUPTED;
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(mp, args.fsbno),
+			mp->m_bsize, 0, &abp);
+	if (error)
 		goto out_unreserve_dquot;
-	}
 
 	/*
 	 * Fill in the child block.
@@ -878,7 +878,11 @@ xfs_bmap_local_to_extents(
 	ASSERT(args.fsbno != NULLFSBLOCK);
 	ASSERT(args.len == 1);
 	tp->t_firstblock = args.fsbno;
-	bp = xfs_btree_get_bufl(args.mp, tp, args.fsbno);
+	error = xfs_trans_get_buf(tp, args.mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(args.mp, args.fsbno),
+			args.mp->m_bsize, 0, &bp);
+	if (error)
+		goto done;
 
 	/*
 	 * Initialize the block, copy the data and log the remote buffer.
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 2d53e5fdff70..fd300dc93ca4 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -678,52 +678,6 @@ xfs_btree_get_block(
 	return XFS_BUF_TO_BLOCK(*bpp);
 }
 
-/*
- * Get a buffer for the block, return it with no data read.
- * Long-form addressing.
- */
-xfs_buf_t *				/* buffer for fsbno */
-xfs_btree_get_bufl(
-	xfs_mount_t	*mp,		/* file system mount point */
-	xfs_trans_t	*tp,		/* transaction pointer */
-	xfs_fsblock_t	fsbno)		/* file system block number */
-{
-	struct xfs_buf		*bp;
-	xfs_daddr_t		d;		/* real disk block address */
-	int			error;
-
-	ASSERT(fsbno != NULLFSBLOCK);
-	d = XFS_FSB_TO_DADDR(mp, fsbno);
-	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
-	if (error)
-		return NULL;
-	return bp;
-}
-
-/*
- * Get a buffer for the block, return it with no data read.
- * Short-form addressing.
- */
-xfs_buf_t *				/* buffer for agno/agbno */
-xfs_btree_get_bufs(
-	xfs_mount_t	*mp,		/* file system mount point */
-	xfs_trans_t	*tp,		/* transaction pointer */
-	xfs_agnumber_t	agno,		/* allocation group number */
-	xfs_agblock_t	agbno)		/* allocation group block number */
-{
-	struct xfs_buf		*bp;
-	xfs_daddr_t		d;		/* real disk block address */
-	int			error;
-
-	ASSERT(agno != NULLAGNUMBER);
-	ASSERT(agbno != NULLAGBLOCK);
-	d = XFS_AGB_TO_DADDR(mp, agno, agbno);
-	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
-	if (error)
-		return NULL;
-	return bp;
-}
-
 /*
  * Change the cursor to point to the first record at the given level.
  * Other levels are unaffected.
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index fb9b2121c628..3eff7c321d43 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -296,27 +296,6 @@ xfs_btree_dup_cursor(
 	xfs_btree_cur_t		*cur,	/* input cursor */
 	xfs_btree_cur_t		**ncur);/* output cursor */
 
-/*
- * Get a buffer for the block, return it with no data read.
- * Long-form addressing.
- */
-struct xfs_buf *				/* buffer for fsbno */
-xfs_btree_get_bufl(
-	struct xfs_mount	*mp,	/* file system mount point */
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_fsblock_t		fsbno);	/* file system block number */
-
-/*
- * Get a buffer for the block, return it with no data read.
- * Short-form addressing.
- */
-struct xfs_buf *				/* buffer for agno/agbno */
-xfs_btree_get_bufs(
-	struct xfs_mount	*mp,	/* file system mount point */
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_agnumber_t		agno,	/* allocation group number */
-	xfs_agblock_t		agbno);	/* allocation group block number */
-
 /*
  * Compute first and last byte offsets for the fields given.
  * Interprets the offsets table, which contains struct field offsets.

[PATCH 10/11] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the
caller passed TRYLOCK and we weren't able to get the lock; and change
the callers to recognize this.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |   31 +++++++++++++++----------------
 fs/xfs/libxfs/xfs_bmap.c  |    9 +++++----
 fs/xfs/xfs_filestream.c   |   11 ++++++-----
 3 files changed, 26 insertions(+), 25 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 83273975df77..26f3e4db84e0 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist(
 
 	if (!pag->pagf_init) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
-			goto out_no_agbp;
-		if (!pag->pagf_init) {
+		if (error == -EAGAIN) {
+			/* Couldn't lock the AGF so skip this AG. */
 			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
 			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
-			goto out_agbp_relse;
+			error = 0;
+			goto out_no_agbp;
 		}
+		if (error)
+			goto out_no_agbp;
 	}
 
 	/*
@@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist(
 	 */
 	if (!agbp) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
-			goto out_no_agbp;
-		if (!agbp) {
+		if (error == -EAGAIN) {
+			/* Couldn't lock the AGF so skip this AG. */
 			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
 			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
+			error = 0;
 			goto out_no_agbp;
 		}
+		if (error)
+			goto out_no_agbp;
 	}
 
 	/* reset a padding mismatched agfl before final free space check */
@@ -2768,10 +2772,10 @@ xfs_alloc_pagf_init(
 	xfs_buf_t		*bp;
 	int			error;
 
-	if ((error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp)))
+	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
+	if (error)
 		return error;
-	if (bp)
-		xfs_trans_brelse(tp, bp);
+	xfs_trans_brelse(tp, bp);
 	return 0;
 }
 
@@ -2958,12 +2962,9 @@ xfs_read_agf(
 	trace_xfs_read_agf(mp, agno);
 
 	ASSERT(agno != NULLAGNUMBER);
-	error = xfs_trans_read_buf(
-			mp, tp, mp->m_ddev_targp,
+	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
 			XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops);
-	if (error == -EAGAIN)
-		return 0;
 	if (error)
 		return error;
 
@@ -2995,8 +2996,6 @@ xfs_alloc_read_agf(
 			bpp);
 	if (error)
 		return error;
-	if (!*bpp)
-		return 0;
 	ASSERT(!(*bpp)->b_error);
 
 	agf = XFS_BUF_TO_AGF(*bpp);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index cfcef076c72f..10b7284cac35 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3311,13 +3311,14 @@ xfs_bmap_longest_free_extent(
 	pag = xfs_perag_get(mp, ag);
 	if (!pag->pagf_init) {
 		error = xfs_alloc_pagf_init(mp, tp, ag, XFS_ALLOC_FLAG_TRYLOCK);
-		if (error)
-			goto out;
-
-		if (!pag->pagf_init) {
+		if (error == -EAGAIN) {
+			/* Couldn't lock the AGF, so skip this AG. */
 			*notinit = 1;
+			error = 0;
 			goto out;
 		}
+		if (error)
+			goto out;
 	}
 
 	longest = xfs_alloc_longest_free_extent(pag,
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index 5f12b5d8527a..3ccdab463359 100644
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -159,16 +159,17 @@ xfs_filestream_pick_ag(
 
 		if (!pag->pagf_init) {
 			err = xfs_alloc_pagf_init(mp, NULL, ag, trylock);
-			if (err && !trylock) {
+			if (err == -EAGAIN) {
+				/* Couldn't lock the AGF, skip this AG. */
+				xfs_perag_put(pag);
+				continue;
+			}
+			if (err) {
 				xfs_perag_put(pag);
 				return err;
 			}
 		}
 
-		/* Might fail sometimes during the 1st pass with trylock set. */
-		if (!pag->pagf_init)
-			goto next_ag;
-
 		/* Keep track of the AG with the most free blocks. */
 		if (pag->pagf_freeblks > maxfree) {
 			maxfree = pag->pagf_freeblks;

[PATCH 11/11] xfs: remove unnecessary null pointer checks from _read_agf callers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Drop the null buffer pointer checks in all code that calls
xfs_alloc_read_agf and doesn't pass XFS_ALLOC_FLAG_TRYLOCK because
they're no longer necessary.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_refcount.c   |    6 ------
 fs/xfs/scrub/agheader_repair.c |    4 ----
 fs/xfs/scrub/fscounters.c      |    3 ---
 fs/xfs/scrub/repair.c          |    2 --
 fs/xfs/xfs_discard.c           |    2 +-
 fs/xfs/xfs_reflink.c           |    2 --
 6 files changed, 1 insertion(+), 18 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index d7d702ee4d1a..6e1665f2cb67 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1177,8 +1177,6 @@ xfs_refcount_finish_one(
 				XFS_ALLOC_FLAG_FREEING, &agbp);
 		if (error)
 			return error;
-		if (XFS_IS_CORRUPT(tp->t_mountp, !agbp))
-			return -EFSCORRUPTED;
 
 		rcur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno);
 		if (!rcur) {
@@ -1718,10 +1716,6 @@ xfs_refcount_recover_cow_leftovers(
 	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
 	if (error)
 		goto out_trans;
-	if (!agbp) {
-		error = -ENOMEM;
-		goto out_trans;
-	}
 	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno);
 
 	/* Find all the leftover CoW staging extents. */
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 7a1a38b636a9..d5e6db9af434 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -659,8 +659,6 @@ xrep_agfl(
 	error = xfs_alloc_read_agf(mp, sc->tp, sc->sa.agno, 0, &agf_bp);
 	if (error)
 		return error;
-	if (!agf_bp)
-		return -ENOMEM;
 
 	/*
 	 * Make sure we have the AGFL buffer, as scrub might have decided it
@@ -735,8 +733,6 @@ xrep_agi_find_btrees(
 	error = xfs_alloc_read_agf(mp, sc->tp, sc->sa.agno, 0, &agf_bp);
 	if (error)
 		return error;
-	if (!agf_bp)
-		return -ENOMEM;
 
 	/* Find the btree roots. */
 	error = xrep_find_ag_btree_roots(sc, agf_bp, fab, NULL);
diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
index 7251c66a82c9..ec2064ed3c30 100644
--- a/fs/xfs/scrub/fscounters.c
+++ b/fs/xfs/scrub/fscounters.c
@@ -83,9 +83,6 @@ xchk_fscount_warmup(
 		error = xfs_alloc_read_agf(mp, sc->tp, agno, 0, &agf_bp);
 		if (error)
 			break;
-		error = -ENOMEM;
-		if (!agf_bp || !agi_bp)
-			break;
 
 		/*
 		 * These are supposed to be initialized by the header read
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 3df49d487940..e489d7a8446a 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -546,8 +546,6 @@ xrep_reap_block(
 		error = xfs_alloc_read_agf(sc->mp, sc->tp, agno, 0, &agf_bp);
 		if (error)
 			return error;
-		if (!agf_bp)
-			return -ENOMEM;
 	} else {
 		agf_bp = sc->sa.agf_bp;
 	}
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index cae613620175..0b8350e84d28 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -45,7 +45,7 @@ xfs_trim_extents(
 	xfs_log_force(mp, XFS_LOG_SYNC);
 
 	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agbp);
-	if (error || !agbp)
+	if (error)
 		goto out_put_perag;
 
 	cur = xfs_allocbt_init_cursor(mp, NULL, agbp, agno, XFS_BTNUM_CNT);
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 7a6c94295b8a..89483d1f262c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -143,8 +143,6 @@ xfs_reflink_find_shared(
 	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
 	if (error)
 		return error;
-	if (!agbp)
-		return -ENOMEM;
 
 	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno);
 

Re: [PATCH 05/11] xfs: make xfs_buf_read_map return an error code

[Christoph Hellwig]

> @@ -842,13 +845,15 @@ xfs_buf_read_map(
>  		 * drop the buffer
>  		 */
>  		xfs_buf_relse(bp);
> -		return NULL;
> +		*bpp = NULL;

We already set *bpp to NULL at the very beginning, so this line is
redundant.

> @@ -860,19 +865,18 @@ xfs_buf_read(
>  	struct xfs_buf		**bpp,
>  	const struct xfs_buf_ops *ops)
>  {
>  	int			error;
>  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
>  
> -	*bpp = NULL;
> -	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> -	if (!bp)
> -		return -ENOMEM;
> -	error = bp->b_error;
> +	error = xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
> +	if (error)
> +		return error;
> +	error = (*bpp)->b_error;
>  	if (error) {
> +		xfs_buf_ioerror_alert(*bpp, __func__);
> +		xfs_buf_stale(*bpp);
> +		xfs_buf_relse(*bpp);
> +		*bpp = NULL;
>  
>  		/* bad CRC means corrupted metadata */
>  		if (error == -EFSBADCRC)

I still think we have a problem here.  We should not have to check
->b_error, and the xfs_buf_ioerror_alert should be either in the callers
or in xfs_buf_read_map, as xfs_buf_read is just supposed to be a trivial
wrapper for the single map case, not add functionality of its own.

Re: [PATCH 09/11] xfs: remove the xfs_btree_get_buf[ls] functions

[Christoph Hellwig]

On Thu, Jan 16, 2020 at 10:24:35PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Remove the xfs_btree_get_bufs and xfs_btree_get_bufl functions, since
> they're pretty trivial oneliners.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 10/11] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers

[Christoph Hellwig]

On Thu, Jan 16, 2020 at 10:24:41PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the
> caller passed TRYLOCK and we weren't able to get the lock; and change
> the callers to recognize this.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_alloc.c |   31 +++++++++++++++----------------
>  fs/xfs/libxfs/xfs_bmap.c  |    9 +++++----
>  fs/xfs/xfs_filestream.c   |   11 ++++++-----
>  3 files changed, 26 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 83273975df77..26f3e4db84e0 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist(
>  
>  	if (!pag->pagf_init) {
>  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> -		if (error)
> -			goto out_no_agbp;
> -		if (!pag->pagf_init) {
> +		if (error == -EAGAIN) {
> +			/* Couldn't lock the AGF so skip this AG. */
>  			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
>  			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> -			goto out_agbp_relse;
> +			error = 0;
> +			goto out_no_agbp;
>  		}
> +		if (error)
> +			goto out_no_agbp;

I wonder if something like:

		if (error) {
			if (error == -EAGAIN) {
				/* Couldn't lock the AGF so skip this AG. */
	 			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
	 			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
				error = 0;
			}
			goto out_no_agbp;
		}

would be a little nicer here?

> @@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist(
>  	 */
>  	if (!agbp) {
>  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> -		if (error)
> -			goto out_no_agbp;
> -		if (!agbp) {
> +		if (error == -EAGAIN) {
> +			/* Couldn't lock the AGF so skip this AG. */
>  			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
>  			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> +			error = 0;
>  			goto out_no_agbp;
>  		}
> +		if (error)
> +			goto out_no_agbp;
>  	}

Same here.  Also shouldn't those asserts just move into
xfs_alloc_read_agf or go away now that we have a proper return value
and not the magic NULL buffer?

> +	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
> +	if (error)
>  		return error;
> +	xfs_trans_brelse(tp, bp);
>  	return 0;

Maybe simplify this further to:

	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
	if (!error)
		xfs_trans_brelse(tp, bp);
	return error;

> @@ -2958,12 +2962,9 @@ xfs_read_agf(
>  	trace_xfs_read_agf(mp, agno);
>  
>  	ASSERT(agno != NULLAGNUMBER);
> -	error = xfs_trans_read_buf(
> -			mp, tp, mp->m_ddev_targp,
> +	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,

This hunk should probably go into the patch that changed the
xfs_trans_read_buf return value instead.

> +		if (error == -EAGAIN) {
> +			/* Couldn't lock the AGF, so skip this AG. */
>  			*notinit = 1;
> +			error = 0;
>  			goto out;
>  		}
> +		if (error)
> +			goto out;

Should probably be:

		if (error) {
			if (error == -EAGAIN) {
				/* Couldn't lock the AGF, so skip this AG. */
	 			*notinit = 1;
				error = 0;
			}
 			goto out;

Re: [PATCH 11/11] xfs: remove unnecessary null pointer checks from _read_agf callers

[Christoph Hellwig]

On Thu, Jan 16, 2020 at 10:24:47PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Drop the null buffer pointer checks in all code that calls
> xfs_alloc_read_agf and doesn't pass XFS_ALLOC_FLAG_TRYLOCK because
> they're no longer necessary.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/11] xfs: make xfs_buf_read_map return an error code

[Darrick J. Wong]

On Thu, Jan 16, 2020 at 10:50:20PM -0800, Christoph Hellwig wrote:
> > @@ -842,13 +845,15 @@ xfs_buf_read_map(
> >  		 * drop the buffer
> >  		 */
> >  		xfs_buf_relse(bp);
> > -		return NULL;
> > +		*bpp = NULL;
> 
> We already set *bpp to NULL at the very beginning, so this line is
> redundant.

Will fix.

> > @@ -860,19 +865,18 @@ xfs_buf_read(
> >  	struct xfs_buf		**bpp,
> >  	const struct xfs_buf_ops *ops)
> >  {
> >  	int			error;
> >  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> >  
> > -	*bpp = NULL;
> > -	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> > -	if (!bp)
> > -		return -ENOMEM;
> > -	error = bp->b_error;
> > +	error = xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
> > +	if (error)
> > +		return error;
> > +	error = (*bpp)->b_error;
> >  	if (error) {
> > +		xfs_buf_ioerror_alert(*bpp, __func__);
> > +		xfs_buf_stale(*bpp);
> > +		xfs_buf_relse(*bpp);
> > +		*bpp = NULL;
> >  
> >  		/* bad CRC means corrupted metadata */
> >  		if (error == -EFSBADCRC)
> 
> I still think we have a problem here.  We should not have to check
> ->b_error, and the xfs_buf_ioerror_alert should be either in the callers
> or in xfs_buf_read_map, as xfs_buf_read is just supposed to be a trivial
> wrapper for the single map case, not add functionality of its own.

Yeah.  I've redone the patchset to keep xfs_buf_read() as a static
inline function, then refactored the ioerror/stale/relse bits as a
separate patch on the end.

--D

Re: [PATCH 10/11] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers

[Darrick J. Wong]

On Thu, Jan 16, 2020 at 10:59:57PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 16, 2020 at 10:24:41PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the
> > caller passed TRYLOCK and we weren't able to get the lock; and change
> > the callers to recognize this.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c |   31 +++++++++++++++----------------
> >  fs/xfs/libxfs/xfs_bmap.c  |    9 +++++----
> >  fs/xfs/xfs_filestream.c   |   11 ++++++-----
> >  3 files changed, 26 insertions(+), 25 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 83273975df77..26f3e4db84e0 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist(
> >  
> >  	if (!pag->pagf_init) {
> >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> > -		if (error)
> > -			goto out_no_agbp;
> > -		if (!pag->pagf_init) {
> > +		if (error == -EAGAIN) {
> > +			/* Couldn't lock the AGF so skip this AG. */
> >  			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
> >  			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> > -			goto out_agbp_relse;
> > +			error = 0;
> > +			goto out_no_agbp;
> >  		}
> > +		if (error)
> > +			goto out_no_agbp;
> 
> I wonder if something like:
> 
> 		if (error) {
> 			if (error == -EAGAIN) {
> 				/* Couldn't lock the AGF so skip this AG. */
> 	 			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
> 	 			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> 				error = 0;
> 			}
> 			goto out_no_agbp;
> 		}
> 
> would be a little nicer here?

> > @@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist(
> >  	 */
> >  	if (!agbp) {
> >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> > -		if (error)
> > -			goto out_no_agbp;
> > -		if (!agbp) {
> > +		if (error == -EAGAIN) {
> > +			/* Couldn't lock the AGF so skip this AG. */
> >  			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
> >  			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> > +			error = 0;
> >  			goto out_no_agbp;
> >  		}
> > +		if (error)
> > +			goto out_no_agbp;
> >  	}
> 
> Same here.  Also shouldn't those asserts just move into
> xfs_alloc_read_agf or go away now that we have a proper return value
> and not the magic NULL buffer?

I'll move the assert into xfs_alloc_read_agf, so these all turn into:

if (error) {
	if (error == -EAGAIN)
		error = 0;
	goto next ag;
}

> 
> > +	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
> > +	if (error)
> >  		return error;
> > +	xfs_trans_brelse(tp, bp);
> >  	return 0;
> 
> Maybe simplify this further to:
> 
> 	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
> 	if (!error)
> 		xfs_trans_brelse(tp, bp);
> 	return error;

Ok.

> > @@ -2958,12 +2962,9 @@ xfs_read_agf(
> >  	trace_xfs_read_agf(mp, agno);
> >  
> >  	ASSERT(agno != NULLAGNUMBER);
> > -	error = xfs_trans_read_buf(
> > -			mp, tp, mp->m_ddev_targp,
> > +	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
> 
> This hunk should probably go into the patch that changed the
> xfs_trans_read_buf return value instead.

Ok.

> > +		if (error == -EAGAIN) {
> > +			/* Couldn't lock the AGF, so skip this AG. */
> >  			*notinit = 1;
> > +			error = 0;
> >  			goto out;
> >  		}
> > +		if (error)
> > +			goto out;
> 
> Should probably be:
> 
> 		if (error) {
> 			if (error == -EAGAIN) {
> 				/* Couldn't lock the AGF, so skip this AG. */
> 	 			*notinit = 1;
> 				error = 0;
> 			}
>  			goto out;

Fixed.

--D

Re: [PATCH 01/11] xfs: make xfs_buf_alloc return an error code

[Dave Chinner]

On Thu, Jan 16, 2020 at 10:23:38PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert _xfs_buf_alloc() to return numeric error codes like most
> everywhere else in xfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
....
> @@ -715,8 +718,8 @@ xfs_buf_get_map(
>  		return NULL;
>  	}
>  
> -	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
> -	if (unlikely(!new_bp))
> +	error = _xfs_buf_alloc(target, map, nmaps, flags, &new_bp);
> +	if (unlikely(error))
>  		return NULL;

You can kill the unlikely() while you are at it. They are largely
unnecessary as the compiler already considers branches with returns
in them as unlikely and we don't need "unlikely" as code
documentation for error conditions like this...

>  	error = xfs_buf_allocate_memory(new_bp, flags);
> @@ -917,8 +920,8 @@ xfs_buf_get_uncached(
>  	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
>  
>  	/* flags might contain irrelevant bits, pass only what we care about */
> -	bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT);
> -	if (unlikely(bp == NULL))
> +	error = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT, &bp);
> +	if (unlikely(error))
>  		goto fail;

here too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 02/11] xfs: make xfs_buf_read return an error code

[Dave Chinner]

On Thu, Jan 16, 2020 at 10:23:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert xfs_buf_read() to return numeric error codes like most
> everywhere else in xfs.  Hoist the callers' error logging and EFSBADCRC
> remapping code into xfs_buf_read to reduce code duplication.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c |   16 +++-------------
>  fs/xfs/xfs_buf.c                |   33 +++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_buf.h                |   14 +++-----------
>  fs/xfs/xfs_log_recover.c        |   26 +++++++-------------------
>  fs/xfs/xfs_symlink.c            |   17 ++++-------------
>  5 files changed, 50 insertions(+), 56 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index a266d05df146..46c516809086 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -418,20 +418,10 @@ xfs_attr_rmtval_get(
>  			       (map[i].br_startblock != HOLESTARTBLOCK));
>  			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
>  			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
> -			bp = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt, 0,
> -					&xfs_attr3_rmt_buf_ops);
> -			if (!bp)
> -				return -ENOMEM;
> -			error = bp->b_error;
> -			if (error) {
> -				xfs_buf_ioerror_alert(bp, __func__);
> -				xfs_buf_relse(bp);
> -
> -				/* bad CRC means corrupted metadata */
> -				if (error == -EFSBADCRC)
> -					error = -EFSCORRUPTED;
> +			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
> +					0, &bp, &xfs_attr3_rmt_buf_ops);
> +			if (error)
>  				return error;
> -			}
>  
>  			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
>  							&offset, &valuelen,
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index a00e63d08a3b..8c9cd1ab870b 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -851,6 +851,39 @@ xfs_buf_read_map(
>  	return bp;
>  }
>  
> +int
> +xfs_buf_read(
> +	struct xfs_buftarg	*target,
> +	xfs_daddr_t		blkno,
> +	size_t			numblks,
> +	xfs_buf_flags_t		flags,
> +	struct xfs_buf		**bpp,
> +	const struct xfs_buf_ops *ops)
> +{
> +	struct xfs_buf		*bp;
> +	int			error;
> +	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +
> +	*bpp = NULL;
> +	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> +	if (!bp)
> +		return -ENOMEM;
> +	error = bp->b_error;
> +	if (error) {
> +		xfs_buf_ioerror_alert(bp, __func__);
> +		xfs_buf_stale(bp);
> +		xfs_buf_relse(bp);
> +
> +		/* bad CRC means corrupted metadata */
> +		if (error == -EFSBADCRC)
> +			error = -EFSCORRUPTED;
> +		return error;
> +	}
> +
> +	*bpp = bp;
> +	return 0;
> +}

I'd just put all this in xfs_buf_read_map() and leave
xfs_buf_read() as a simple wrapper around xfs_buf_read_map().

Also:

	if (!bp->b_error) {
		*bpp = bp;
		return 0;
	}
	/* handle error without extra indenting */


> -	const struct xfs_buf_ops *ops)
> -{
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -	return xfs_buf_read_map(target, &map, 1, flags, ops);
> -}
> +int xfs_buf_read(struct xfs_buftarg *target, xfs_daddr_t blkno, size_t numblks,
> +		xfs_buf_flags_t flags, struct xfs_buf **bpp,
> +		const struct xfs_buf_ops *ops);
>  
>  static inline void
>  xfs_buf_readahead(
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 0d683fb96396..ac79537d3275 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2745,15 +2745,10 @@ xlog_recover_buffer_pass2(
>  	if (buf_f->blf_flags & XFS_BLF_INODE_BUF)
>  		buf_flags |= XBF_UNMAPPED;
>  
> -	bp = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> -			  buf_flags, NULL);
> -	if (!bp)
> -		return -ENOMEM;
> -	error = bp->b_error;
> -	if (error) {
> -		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)");
> -		goto out_release;
> -	}
> +	error = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> +			  buf_flags, &bp, NULL);
> +	if (error)
> +		return error;

I'd argue that if we are touching every remaining xfs_buf_read() call
like this, we should get rid of it and just call xfs_buf_read_map()
instead.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 03/11] xfs: make xfs_buf_get return an error code

[Dave Chinner]

On Thu, Jan 16, 2020 at 10:23:50PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert xfs_buf_get() to return numeric error codes like most
> everywhere else in xfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Same comments about xfs_buf_get() vs xfs_buf_get_map() as the last
patch (i.e. replace xfs_buf_get with xfs_buf_get_map).

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/11] xfs: make xfs_buf_alloc return an error code

[Darrick J. Wong]

On Mon, Jan 20, 2020 at 08:49:03AM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2020 at 10:23:38PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert _xfs_buf_alloc() to return numeric error codes like most
> > everywhere else in xfs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> ....
> > @@ -715,8 +718,8 @@ xfs_buf_get_map(
> >  		return NULL;
> >  	}
> >  
> > -	new_bp = _xfs_buf_alloc(target, map, nmaps, flags);
> > -	if (unlikely(!new_bp))
> > +	error = _xfs_buf_alloc(target, map, nmaps, flags, &new_bp);
> > +	if (unlikely(error))
> >  		return NULL;
> 
> You can kill the unlikely() while you are at it. They are largely
> unnecessary as the compiler already considers branches with returns
> in them as unlikely and we don't need "unlikely" as code
> documentation for error conditions like this...
> 
> >  	error = xfs_buf_allocate_memory(new_bp, flags);
> > @@ -917,8 +920,8 @@ xfs_buf_get_uncached(
> >  	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
> >  
> >  	/* flags might contain irrelevant bits, pass only what we care about */
> > -	bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT);
> > -	if (unlikely(bp == NULL))
> > +	error = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT, &bp);
> > +	if (unlikely(error))
> >  		goto fail;
> 
> here too.

Will fix.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 02/11] xfs: make xfs_buf_read return an error code

[Darrick J. Wong]

On Mon, Jan 20, 2020 at 08:57:20AM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2020 at 10:23:44PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert xfs_buf_read() to return numeric error codes like most
> > everywhere else in xfs.  Hoist the callers' error logging and EFSBADCRC
> > remapping code into xfs_buf_read to reduce code duplication.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/libxfs/xfs_attr_remote.c |   16 +++-------------
> >  fs/xfs/xfs_buf.c                |   33 +++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_buf.h                |   14 +++-----------
> >  fs/xfs/xfs_log_recover.c        |   26 +++++++-------------------
> >  fs/xfs/xfs_symlink.c            |   17 ++++-------------
> >  5 files changed, 50 insertions(+), 56 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > index a266d05df146..46c516809086 100644
> > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > @@ -418,20 +418,10 @@ xfs_attr_rmtval_get(
> >  			       (map[i].br_startblock != HOLESTARTBLOCK));
> >  			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
> >  			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
> > -			bp = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt, 0,
> > -					&xfs_attr3_rmt_buf_ops);
> > -			if (!bp)
> > -				return -ENOMEM;
> > -			error = bp->b_error;
> > -			if (error) {
> > -				xfs_buf_ioerror_alert(bp, __func__);
> > -				xfs_buf_relse(bp);
> > -
> > -				/* bad CRC means corrupted metadata */
> > -				if (error == -EFSBADCRC)
> > -					error = -EFSCORRUPTED;
> > +			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
> > +					0, &bp, &xfs_attr3_rmt_buf_ops);
> > +			if (error)
> >  				return error;
> > -			}
> >  
> >  			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
> >  							&offset, &valuelen,
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index a00e63d08a3b..8c9cd1ab870b 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -851,6 +851,39 @@ xfs_buf_read_map(
> >  	return bp;
> >  }
> >  
> > +int
> > +xfs_buf_read(
> > +	struct xfs_buftarg	*target,
> > +	xfs_daddr_t		blkno,
> > +	size_t			numblks,
> > +	xfs_buf_flags_t		flags,
> > +	struct xfs_buf		**bpp,
> > +	const struct xfs_buf_ops *ops)
> > +{
> > +	struct xfs_buf		*bp;
> > +	int			error;
> > +	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > +
> > +	*bpp = NULL;
> > +	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> > +	if (!bp)
> > +		return -ENOMEM;
> > +	error = bp->b_error;
> > +	if (error) {
> > +		xfs_buf_ioerror_alert(bp, __func__);
> > +		xfs_buf_stale(bp);
> > +		xfs_buf_relse(bp);
> > +
> > +		/* bad CRC means corrupted metadata */
> > +		if (error == -EFSBADCRC)
> > +			error = -EFSCORRUPTED;
> > +		return error;
> > +	}
> > +
> > +	*bpp = bp;
> > +	return 0;
> > +}
> 
> I'd just put all this in xfs_buf_read_map() and leave
> xfs_buf_read() as a simple wrapper around xfs_buf_read_map().
> 
> Also:
> 
> 	if (!bp->b_error) {
> 		*bpp = bp;
> 		return 0;
> 	}
> 	/* handle error without extra indenting */

Yeah, Christoph nudged me towards that at the end of last week, too.

> 
> > -	const struct xfs_buf_ops *ops)
> > -{
> > -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > -	return xfs_buf_read_map(target, &map, 1, flags, ops);
> > -}
> > +int xfs_buf_read(struct xfs_buftarg *target, xfs_daddr_t blkno, size_t numblks,
> > +		xfs_buf_flags_t flags, struct xfs_buf **bpp,
> > +		const struct xfs_buf_ops *ops);
> >  
> >  static inline void
> >  xfs_buf_readahead(
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 0d683fb96396..ac79537d3275 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2745,15 +2745,10 @@ xlog_recover_buffer_pass2(
> >  	if (buf_f->blf_flags & XFS_BLF_INODE_BUF)
> >  		buf_flags |= XBF_UNMAPPED;
> >  
> > -	bp = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> > -			  buf_flags, NULL);
> > -	if (!bp)
> > -		return -ENOMEM;
> > -	error = bp->b_error;
> > -	if (error) {
> > -		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)");
> > -		goto out_release;
> > -	}
> > +	error = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> > +			  buf_flags, &bp, NULL);
> > +	if (error)
> > +		return error;
> 
> I'd argue that if we are touching every remaining xfs_buf_read() call
> like this, we should get rid of it and just call xfs_buf_read_map()
> instead.

FWIW I changed out this patch to leave xfs_buf_read as a static inline
and shoved all the buffer state and ioerror_alert refactoring to the end
of the series, so at least this part is a straight conversion.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com