All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 00/12] xfs: make buffer functions return error codes
@ 2020-01-23  7:41 Darrick J. Wong
  2020-01-23  7:41 ` [PATCH 01/12] xfs: make xfs_buf_alloc return an error code Darrick J. Wong
                   ` (11 more replies)
  0 siblings, 12 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:41 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david

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.

After finishing the error code conversion, we straighten out the TRYLOCK
callers to remove all this null pointer checks in favor of explicit
EAGAIN checks; and then we change the buffer IO/corruption error
reporting so that we report whoever called the buffer code even when
reading a buffer in transaction context.

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

--D

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 01/12] xfs: make xfs_buf_alloc return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
@ 2020-01-23  7:41 ` Darrick J. Wong
  2020-01-24  0:47   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 02/12] xfs: make xfs_buf_read " Darrick J. Wong
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:41 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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..f9a6cf71f4ab 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 (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 (error)
 		goto fail;
 
 	page_count = PAGE_ALIGN(numblks << BBSHIFT) >> PAGE_SHIFT;


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 02/12] xfs: make xfs_buf_read return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
  2020-01-23  7:41 ` [PATCH 01/12] xfs: make xfs_buf_alloc return an error code Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-23 22:20   ` Christoph Hellwig
  2020-01-24  0:49   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 03/12] xfs: make xfs_buf_get " Darrick J. Wong
                   ` (9 subsequent siblings)
  11 siblings, 2 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david

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

Convert xfs_buf_read() 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_attr_remote.c |    8 ++++----
 fs/xfs/xfs_buf.h                |   13 +++++++++++--
 fs/xfs/xfs_log_recover.c        |   16 +++++++---------
 fs/xfs/xfs_symlink.c            |    8 ++++----
 4 files changed, 26 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index a266d05df146..d82985571a5f 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -418,10 +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 = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
+					0, &bp, &xfs_attr3_rmt_buf_ops);
+			if (error)
+				return error;
 			error = bp->b_error;
 			if (error) {
 				xfs_buf_ioerror_alert(bp, __func__);
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 56e081dd1d96..fb60c36a8a5b 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -213,16 +213,25 @@ xfs_buf_get(
 	return xfs_buf_get_map(target, &map, 1, 0);
 }
 
-static inline struct xfs_buf *
+static inline 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;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return xfs_buf_read_map(target, &map, 1, flags, ops);
+
+	*bpp = NULL;
+	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
+	if (!bp)
+		return -ENOMEM;
+
+	*bpp = bp;
+	return 0;
 }
 
 static inline void
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0d683fb96396..b29806846916 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2745,10 +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 = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
+			  buf_flags, &bp, NULL);
+	if (error)
+		return error;
 	error = bp->b_error;
 	if (error) {
 		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)");
@@ -2950,12 +2950,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)");
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index a25502bc2071..4f10d764163b 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -53,10 +53,10 @@ 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 = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt), 0,
+				&bp, &xfs_symlink_buf_ops);
+		if (error)
+			return error;
 		error = bp->b_error;
 		if (error) {
 			xfs_buf_ioerror_alert(bp, __func__);


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 03/12] xfs: make xfs_buf_get return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
  2020-01-23  7:41 ` [PATCH 01/12] xfs: make xfs_buf_alloc return an error code Darrick J. Wong
  2020-01-23  7:42 ` [PATCH 02/12] xfs: make xfs_buf_read " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  0:50   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 04/12] xfs: make xfs_buf_get_uncached " Darrick J. Wong
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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 d82985571a5f..46f055804433 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -555,9 +555,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 fb60c36a8a5b..fe9ad8ee4eea 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;
 }
 
 static inline int


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 04/12] xfs: make xfs_buf_get_uncached return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (2 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 03/12] xfs: make xfs_buf_get " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  0:53   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 05/12] xfs: make xfs_buf_read_map " Darrick J. Wong
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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 f9a6cf71f4ab..09b8f00d4182 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -883,12 +883,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);
@@ -899,7 +900,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;
 	}
@@ -908,17 +909,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 (error)
@@ -931,8 +935,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;
 
@@ -944,7 +950,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)
@@ -954,7 +961,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 fe9ad8ee4eea..eace3e285157 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -254,8 +254,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);


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (3 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 04/12] xfs: make xfs_buf_get_uncached " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-23 22:24   ` Christoph Hellwig
  2020-01-24  1:31   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 06/12] xfs: make xfs_buf_get_map " Darrick J. Wong
                   ` (6 subsequent siblings)
  11 siblings, 2 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david

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

Convert xfs_buf_read_map() to return numeric error codes like most
everywhere else in xfs.  This involves moving the open-coded logic that
reports metadata IO read / corruption errors and stales the buffer into
xfs_buf_read_map so that the logic is all in one place.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c       |    7 ++--
 fs/xfs/libxfs/xfs_attr_remote.c |   10 ------
 fs/xfs/xfs_buf.c                |   64 +++++++++++++++++++++++++++++----------
 fs/xfs/xfs_buf.h                |   21 ++++---------
 fs/xfs/xfs_log_recover.c        |   10 ------
 fs/xfs/xfs_symlink.c            |   10 ------
 fs/xfs/xfs_trans_buf.c          |   39 ++++++------------------
 7 files changed, 67 insertions(+), 94 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fc93fd88ec89..df25024275a1 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2956,14 +2956,13 @@ 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;
-	if (!*bpp)
-		return 0;
 
 	ASSERT(!(*bpp)->b_error);
 	xfs_buf_set_ref(*bpp, XFS_AGF_REF);
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 46f055804433..8b7f74b3bea2 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -422,16 +422,6 @@ xfs_attr_rmtval_get(
 					0, &bp, &xfs_attr3_rmt_buf_ops);
 			if (error)
 				return error;
-			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;
-				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 09b8f00d4182..8a1c5c705b29 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -809,46 +809,76 @@ 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;
+	int			error;
 
 	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_);
 
 	if (!(bp->b_flags & XBF_DONE)) {
+		/* Initiate the buffer read and wait. */
 		XFS_STATS_INC(target->bt_mount, xb_get_read);
 		bp->b_ops = ops;
-		_xfs_buf_read(bp, flags);
-		return bp;
+		error = _xfs_buf_read(bp, flags);
+
+		/* Readahead iodone already dropped the buffer, so exit. */
+		if (flags & XBF_ASYNC)
+			return 0;
+	} else {
+		/* Buffer already read; all we need to do is check it. */
+		error = xfs_buf_reverify(bp, ops);
+
+		/* Readahead already finished; drop the buffer and exit. */
+		if (flags & XBF_ASYNC) {
+			xfs_buf_relse(bp);
+			return 0;
+		}
+
+		/* We do not want read in the flags */
+		bp->b_flags &= ~XBF_READ;
+		ASSERT(bp->b_ops != NULL || ops == NULL);
 	}
 
-	xfs_buf_reverify(bp, ops);
+	/*
+	 * If we've had a read error, then the contents of the buffer are
+	 * invalid and should not be used. To ensure that a followup read tries
+	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
+	 * mark the buffer stale. This ensures that anyone who has a current
+	 * reference to the buffer will interpret it's contents correctly and
+	 * future cache lookups will also treat it as an empty, uninitialised
+	 * buffer.
+	 */
+	if (error) {
+		if (!XFS_FORCED_SHUTDOWN(target->bt_mount))
+			xfs_buf_ioerror_alert(bp, __func__);
 
-	if (flags & XBF_ASYNC) {
-		/*
-		 * Read ahead call which is already satisfied,
-		 * drop the buffer
-		 */
+		bp->b_flags &= ~XBF_DONE;
+		xfs_buf_stale(bp);
 		xfs_buf_relse(bp);
-		return NULL;
+
+		/* bad CRC means corrupted metadata */
+		if (error == -EFSBADCRC)
+			error = -EFSCORRUPTED;
+		return error;
 	}
 
-	/* 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;
 }
 
 /*
@@ -862,11 +892,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 eace3e285157..14209db07684 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(
@@ -231,16 +229,9 @@ xfs_buf_read(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	*bpp = NULL;
-	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
-	if (!bp)
-		return -ENOMEM;
-
-	*bpp = bp;
-	return 0;
+	return xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
 }
 
 static inline void
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b29806846916..ac79537d3275 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2749,11 +2749,6 @@ xlog_recover_buffer_pass2(
 			  buf_flags, &bp, NULL);
 	if (error)
 		return error;
-	error = bp->b_error;
-	if (error) {
-		xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)");
-		goto out_release;
-	}
 
 	/*
 	 * Recover the buffer only if we get an LSN from it and it's less than
@@ -2954,11 +2949,6 @@ xlog_recover_inode_pass2(
 			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 4f10d764163b..b94d7b9b55d0 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -57,16 +57,6 @@ xfs_readlink_bmap_ilocked(
 				&bp, &xfs_symlink_buf_ops);
 		if (error)
 			return error;
-		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;
-			goto out;
-		}
 		byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
 		if (pathlen < byte_cnt)
 			byte_cnt = pathlen;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index b5b3a78ef31c..56e7f8126cd7 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -298,36 +298,17 @@ 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;
-	}
-
-	/*
-	 * If we've had a read error, then the contents of the buffer are
-	 * invalid and should not be used. To ensure that a followup read tries
-	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
-	 * mark the buffer stale. This ensures that anyone who has a current
-	 * reference to the buffer will interpret it's contents correctly and
-	 * future cache lookups will also treat it as an empty, uninitialised
-	 * buffer.
-	 */
-	if (bp->b_error) {
-		error = bp->b_error;
-		if (!XFS_FORCED_SHUTDOWN(mp))
-			xfs_buf_ioerror_alert(bp, __func__);
-		bp->b_flags &= ~XBF_DONE;
-		xfs_buf_stale(bp);
-
+	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
+	switch (error) {
+	case 0:
+		break;
+	case -EFSCORRUPTED:
+	case -EIO:
 		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
-			xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
-		xfs_buf_relse(bp);
-
-		/* bad CRC means corrupted metadata */
-		if (error == -EFSBADCRC)
-			error = -EFSCORRUPTED;
+			xfs_force_shutdown(tp->t_mountp,
+					SHUTDOWN_META_IO_ERROR);
+		/* fall through */
+	default:
 		return error;
 	}
 


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 06/12] xfs: make xfs_buf_get_map return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (4 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 05/12] xfs: make xfs_buf_read_map " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  1:41   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 07/12] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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       |   45 ++++++++++++++++-----------------------------
 fs/xfs/xfs_buf.h       |   14 +++-----------
 fs/xfs/xfs_trans_buf.c |   14 +++++++++-----
 3 files changed, 28 insertions(+), 45 deletions(-)


diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 8a1c5c705b29..b420e865b32e 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 (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
@@ -824,9 +811,9 @@ xfs_buf_read_map(
 	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 14209db07684..d1908a5038a2 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);
 }
 
 static inline int
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 56e7f8126cd7..6b7ef9dc167d 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);
 


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 07/12] xfs: make xfs_trans_get_buf_map return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (5 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 06/12] xfs: make xfs_buf_get_map " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  1:44   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 08/12] xfs: make xfs_trans_get_buf " Darrick J. Wong
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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 6b7ef9dc167d..449be0f8c10f 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;
 }
 
 /*


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 08/12] xfs: make xfs_trans_get_buf return an error code
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (6 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 07/12] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  1:48   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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 b94d7b9b55d0..d762d42ed0ff 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -280,12 +280,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);
@@ -423,13 +421,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,


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (7 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 08/12] xfs: make xfs_trans_get_buf " Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  1:50   ` Dave Chinner
  2020-01-23  7:42 ` [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 df25024275a1..7be6c8fbfcf9 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.


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (8 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
@ 2020-01-23  7:42 ` Darrick J. Wong
  2020-01-24  2:00   ` Dave Chinner
  2020-01-23  7:43 ` [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
  2020-01-23  7:43 ` [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting Darrick J. Wong
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:42 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david

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 |   34 +++++++++++++++-------------------
 fs/xfs/libxfs/xfs_bmap.c  |   11 ++++++-----
 fs/xfs/xfs_filestream.c   |   11 ++++++-----
 3 files changed, 27 insertions(+), 29 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 7be6c8fbfcf9..ed0172850400 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2502,12 +2502,11 @@ xfs_alloc_fix_freelist(
 
 	if (!pag->pagf_init) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
-		if (!pag->pagf_init) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
-			goto out_agbp_relse;
 		}
 	}
 
@@ -2533,11 +2532,10 @@ 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) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
 		}
 	}
@@ -2768,11 +2766,10 @@ xfs_alloc_pagf_init(
 	xfs_buf_t		*bp;
 	int			error;
 
-	if ((error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp)))
-		return error;
-	if (bp)
+	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
+	if (!error)
 		xfs_trans_brelse(tp, bp);
-	return 0;
+	return error;
 }
 
 /*
@@ -2961,8 +2958,6 @@ xfs_read_agf(
 	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;
 
@@ -2992,10 +2987,11 @@ xfs_alloc_read_agf(
 	error = xfs_read_agf(mp, tp, agno,
 			(flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
 			bpp);
-	if (error)
+	if (error) {
+		/* We don't support trylock when freeing. */
+		ASSERT(error != -EAGAIN || !(flags & XFS_ALLOC_FLAG_FREEING));
 		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..9a6d7a84689a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3311,11 +3311,12 @@ 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) {
-			*notinit = 1;
+		if (error) {
+			/* Couldn't lock the AGF, so skip this AG. */
+			if (error == -EAGAIN) {
+				*notinit = 1;
+				error = 0;
+			}
 			goto out;
 		}
 	}
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;


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (9 preceding siblings ...)
  2020-01-23  7:42 ` [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
@ 2020-01-23  7:43 ` Darrick J. Wong
  2020-01-24  2:01   ` Dave Chinner
  2020-01-23  7:43 ` [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting Darrick J. Wong
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:43 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 e723b267a247..b0ce04ffd3cd 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);
 


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting
  2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
                   ` (10 preceding siblings ...)
  2020-01-23  7:43 ` [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
@ 2020-01-23  7:43 ` Darrick J. Wong
  2020-01-24  2:07   ` Dave Chinner
  11 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-23  7:43 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david

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

Instead of passing __func__ to the error reporting function, let's use
the return address builtins so that the messages actually tell you which
higher level function called the buffer functions.  This was previously
true for the xfs_buf_read callers, but not for the xfs_trans_read_buf
callers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_buf.c         |   12 +++++++-----
 fs/xfs/xfs_buf.h         |    7 ++++---
 fs/xfs/xfs_buf_item.c    |    2 +-
 fs/xfs/xfs_log_recover.c |    4 ++--
 fs/xfs/xfs_trans_buf.c   |    5 +++--
 5 files changed, 17 insertions(+), 13 deletions(-)


diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index b420e865b32e..217e4f82a44a 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -803,7 +803,8 @@ xfs_buf_read_map(
 	int			nmaps,
 	xfs_buf_flags_t		flags,
 	struct xfs_buf		**bpp,
-	const struct xfs_buf_ops *ops)
+	const struct xfs_buf_ops *ops,
+	xfs_failaddr_t		fa)
 {
 	struct xfs_buf		*bp;
 	int			error;
@@ -852,7 +853,7 @@ xfs_buf_read_map(
 	 */
 	if (error) {
 		if (!XFS_FORCED_SHUTDOWN(target->bt_mount))
-			xfs_buf_ioerror_alert(bp, __func__);
+			xfs_buf_ioerror_alert(bp, fa);
 
 		bp->b_flags &= ~XBF_DONE;
 		xfs_buf_stale(bp);
@@ -885,7 +886,8 @@ xfs_buf_readahead_map(
 		return;
 
 	xfs_buf_read_map(target, map, nmaps,
-		     XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops);
+		     XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops,
+		     __this_address);
 }
 
 /*
@@ -1234,10 +1236,10 @@ __xfs_buf_ioerror(
 void
 xfs_buf_ioerror_alert(
 	struct xfs_buf		*bp,
-	const char		*func)
+	xfs_failaddr_t		func)
 {
 	xfs_alert(bp->b_mount,
-"metadata I/O error in \"%s\" at daddr 0x%llx len %d error %d",
+"metadata I/O error in \"%pS\" at daddr 0x%llx len %d error %d",
 			func, (uint64_t)XFS_BUF_ADDR(bp), bp->b_length,
 			-bp->b_error);
 }
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index d1908a5038a2..567ec2c24244 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -196,7 +196,7 @@ 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);
+		const struct xfs_buf_ops *ops, xfs_failaddr_t fa);
 void xfs_buf_readahead_map(struct xfs_buftarg *target, struct xfs_buf_map *map,
 		int nmaps, const struct xfs_buf_ops *ops);
 
@@ -223,7 +223,8 @@ xfs_buf_read(
 {
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	return xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
+	return xfs_buf_read_map(target, &map, 1, flags, bpp, ops,
+			__builtin_return_address(0));
 }
 
 static inline void
@@ -260,7 +261,7 @@ extern void xfs_buf_ioend(struct xfs_buf *bp);
 extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
 		xfs_failaddr_t failaddr);
 #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
-extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func);
+extern void xfs_buf_ioerror_alert(struct xfs_buf *bp, xfs_failaddr_t fa);
 
 extern int __xfs_buf_submit(struct xfs_buf *bp, bool);
 static inline int xfs_buf_submit(struct xfs_buf *bp)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 5be8973a452c..663810e6cd59 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1113,7 +1113,7 @@ xfs_buf_iodone_callback_error(
 	if (bp->b_target != lasttarg ||
 	    time_after(jiffies, (lasttime + 5*HZ))) {
 		lasttime = jiffies;
-		xfs_buf_ioerror_alert(bp, __func__);
+		xfs_buf_ioerror_alert(bp, __this_address);
 	}
 	lasttarg = bp->b_target;
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ac79537d3275..25cfc85dbaa7 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -294,7 +294,7 @@ xlog_recover_iodone(
 		 * this during recovery. One strike!
 		 */
 		if (!XFS_FORCED_SHUTDOWN(bp->b_mount)) {
-			xfs_buf_ioerror_alert(bp, __func__);
+			xfs_buf_ioerror_alert(bp, __this_address);
 			xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
 		}
 	}
@@ -5627,7 +5627,7 @@ xlog_do_recover(
 	error = xfs_buf_submit(bp);
 	if (error) {
 		if (!XFS_FORCED_SHUTDOWN(mp)) {
-			xfs_buf_ioerror_alert(bp, __func__);
+			xfs_buf_ioerror_alert(bp, __this_address);
 			ASSERT(0);
 		}
 		xfs_buf_relse(bp);
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 449be0f8c10f..2ffa401ebcc6 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -280,7 +280,7 @@ xfs_trans_read_buf_map(
 		ASSERT(bp->b_ops != NULL);
 		error = xfs_buf_reverify(bp, ops);
 		if (error) {
-			xfs_buf_ioerror_alert(bp, __func__);
+			xfs_buf_ioerror_alert(bp, __return_address);
 
 			if (tp->t_flags & XFS_TRANS_DIRTY)
 				xfs_force_shutdown(tp->t_mountp,
@@ -302,7 +302,8 @@ xfs_trans_read_buf_map(
 		return 0;
 	}
 
-	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
+	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops,
+			__return_address);
 	switch (error) {
 	case 0:
 		break;


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] xfs: make xfs_buf_read return an error code
  2020-01-23  7:42 ` [PATCH 02/12] xfs: make xfs_buf_read " Darrick J. Wong
@ 2020-01-23 22:20   ` Christoph Hellwig
  2020-01-24  0:16     ` Darrick J. Wong
  2020-01-24  0:49   ` Dave Chinner
  1 sibling, 1 reply; 35+ messages in thread
From: Christoph Hellwig @ 2020-01-23 22:20 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, david

On Wed, Jan 22, 2020 at 11:42:03PM -0800, Darrick J. Wong wrote:
> -				return -ENOMEM;
> +			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
> +					0, &bp, &xfs_attr3_rmt_buf_ops);
> +			if (error)
> +				return error;
>  			error = bp->b_error;
>  			if (error) {
>  				xfs_buf_ioerror_alert(bp, __func__);

This still has the bogus b_error check where it should just check the
error and report it based on the return value.

> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 56e081dd1d96..fb60c36a8a5b 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -213,16 +213,25 @@ xfs_buf_get(
>  	return xfs_buf_get_map(target, &map, 1, 0);
>  }
>  
> -static inline struct xfs_buf *
> +static inline 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;
>  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> -	return xfs_buf_read_map(target, &map, 1, flags, ops);
> +
> +	*bpp = NULL;
> +	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> +	if (!bp)
> +		return -ENOMEM;
> +
> +	*bpp = bp;
> +	return 0;
>  }
>  
>  static inline void
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 0d683fb96396..b29806846916 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2745,10 +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 = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> +			  buf_flags, &bp, NULL);
> +	if (error)
> +		return error;
>  	error = bp->b_error;
>  	if (error) {

.. and same here.


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-23  7:42 ` [PATCH 05/12] xfs: make xfs_buf_read_map " Darrick J. Wong
@ 2020-01-23 22:24   ` Christoph Hellwig
  2020-01-24  0:23     ` Darrick J. Wong
  2020-01-24  1:31   ` Dave Chinner
  1 sibling, 1 reply; 35+ messages in thread
From: Christoph Hellwig @ 2020-01-23 22:24 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, david

On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> index fc93fd88ec89..df25024275a1 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2956,14 +2956,13 @@ 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;
> -	if (!*bpp)
> -		return 0;

Shouldn't the change in calling conventions for xfs_trans_read_buf be
in another patch dealing just with xfs_trans_read_buf?

> +		/* bad CRC means corrupted metadata */
> +		if (error == -EFSBADCRC)
> +			error = -EFSCORRUPTED;
> +		return error;

Note that this coukd and should now also go away in the xfs_buf_read()
callers, not just the direct xfs_buf_read_map ones.

> +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> +	switch (error) {
> +	case 0:
> +		break;
> +	case -EFSCORRUPTED:
> +	case -EIO:
>  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> +			xfs_force_shutdown(tp->t_mountp,
> +					SHUTDOWN_META_IO_ERROR);
> +		/* fall through */
> +	default:

Isn't it really EAGAIN the only special case here?  I.e. something
more like:

	if (error && error != -EAGAIN) {
  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
			xfs_force_shutdown(tp->t_mountp,
					SHUTDOWN_META_IO_ERROR);
	}

	return error;

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] xfs: make xfs_buf_read return an error code
  2020-01-23 22:20   ` Christoph Hellwig
@ 2020-01-24  0:16     ` Darrick J. Wong
  2020-01-24  1:08       ` Darrick J. Wong
  0 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  0:16 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, david

On Thu, Jan 23, 2020 at 02:20:15PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2020 at 11:42:03PM -0800, Darrick J. Wong wrote:
> > -				return -ENOMEM;
> > +			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
> > +					0, &bp, &xfs_attr3_rmt_buf_ops);
> > +			if (error)
> > +				return error;
> >  			error = bp->b_error;
> >  			if (error) {
> >  				xfs_buf_ioerror_alert(bp, __func__);
> 
> This still has the bogus b_error check where it should just check the
> error and report it based on the return value.

Yes, because at this point midway through the series xfs_buf_read only
knows how to return -ENOMEM.  I'm changing the *interfaces* here, saving
most of the behavior changes for the xfs_buf_read_map change...

> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index 56e081dd1d96..fb60c36a8a5b 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -213,16 +213,25 @@ xfs_buf_get(
> >  	return xfs_buf_get_map(target, &map, 1, 0);
> >  }
> >  
> > -static inline struct xfs_buf *
> > +static inline 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;
> >  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > -	return xfs_buf_read_map(target, &map, 1, flags, ops);
> > +
> > +	*bpp = NULL;
> > +	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> > +	if (!bp)
> > +		return -ENOMEM;
> > +
> > +	*bpp = bp;

...because otherwise I end having to migrate all the bp->b_error
checking into this static inline function, which causes compile errors.

I could work around /that/ by moving the xfs_buf_read_map conversion
earlier in the series, but I'd have to rebase the whole series to
achieve the same end result, which seems pointless.

But I guess I could go mess around with it and see just how much of a
pain doing that /actually/ is...

--D

> > +	return 0;
> >  }
> >  
> >  static inline void
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 0d683fb96396..b29806846916 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2745,10 +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 = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> > +			  buf_flags, &bp, NULL);
> > +	if (error)
> > +		return error;
> >  	error = bp->b_error;
> >  	if (error) {
> 
> .. and same here.
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-23 22:24   ` Christoph Hellwig
@ 2020-01-24  0:23     ` Darrick J. Wong
  2020-01-24  4:59       ` Darrick J. Wong
  0 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  0:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, david

On Thu, Jan 23, 2020 at 02:24:41PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> > index fc93fd88ec89..df25024275a1 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2956,14 +2956,13 @@ 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;
> > -	if (!*bpp)
> > -		return 0;
> 
> Shouldn't the change in calling conventions for xfs_trans_read_buf be
> in another patch dealing just with xfs_trans_read_buf?

Actually ... it really needs to be in the next patch because it's the
xfs_buf_get_map transition that makes it so that xfs_trans_read_buf can
return EAGAIN.

> > +		/* bad CRC means corrupted metadata */
> > +		if (error == -EFSBADCRC)
> > +			error = -EFSCORRUPTED;
> > +		return error;
> 
> Note that this coukd and should now also go away in the xfs_buf_read()
> callers, not just the direct xfs_buf_read_map ones.

Huh?  This patch /does/ remove the EFSBADCRC->EFSCORRUPTED code in the
xfs_buf_read callers... <confused>

> > +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> > +	switch (error) {
> > +	case 0:
> > +		break;
> > +	case -EFSCORRUPTED:
> > +	case -EIO:
> >  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> > +			xfs_force_shutdown(tp->t_mountp,
> > +					SHUTDOWN_META_IO_ERROR);
> > +		/* fall through */
> > +	default:
> 
> Isn't it really EAGAIN the only special case here?  I.e. something
> more like:
> 
> 	if (error && error != -EAGAIN) {
>   		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> 			xfs_force_shutdown(tp->t_mountp,
> 					SHUTDOWN_META_IO_ERROR);
> 	}
> 
> 	return error;

Yes, I think so.

--D

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 01/12] xfs: make xfs_buf_alloc return an error code
  2020-01-23  7:41 ` [PATCH 01/12] xfs: make xfs_buf_alloc return an error code Darrick J. Wong
@ 2020-01-24  0:47   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  0:47 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:41:56PM -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>
> ---
>  fs/xfs/xfs_buf.c |   21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] xfs: make xfs_buf_read return an error code
  2020-01-23  7:42 ` [PATCH 02/12] xfs: make xfs_buf_read " Darrick J. Wong
  2020-01-23 22:20   ` Christoph Hellwig
@ 2020-01-24  0:49   ` Dave Chinner
  1 sibling, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  0:49 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch

On Wed, Jan 22, 2020 at 11:42:03PM -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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c |    8 ++++----
>  fs/xfs/xfs_buf.h                |   13 +++++++++++--
>  fs/xfs/xfs_log_recover.c        |   16 +++++++---------
>  fs/xfs/xfs_symlink.c            |    8 ++++----
>  4 files changed, 26 insertions(+), 19 deletions(-)

Looks fine. All of the callers have exactly the same error handling,
so this makes sense to pull that up into xfs_buf_read() as a first
step.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 03/12] xfs: make xfs_buf_get return an error code
  2020-01-23  7:42 ` [PATCH 03/12] xfs: make xfs_buf_get " Darrick J. Wong
@ 2020-01-24  0:50   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  0:50 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:09PM -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 API change as xfs_buf_read(), makes sense.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 04/12] xfs: make xfs_buf_get_uncached return an error code
  2020-01-23  7:42 ` [PATCH 04/12] xfs: make xfs_buf_get_uncached " Darrick J. Wong
@ 2020-01-24  0:53   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  0:53 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:16PM -0800, Darrick J. Wong wrote:
> 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>

looks good,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] xfs: make xfs_buf_read return an error code
  2020-01-24  0:16     ` Darrick J. Wong
@ 2020-01-24  1:08       ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  1:08 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, david

On Thu, Jan 23, 2020 at 04:16:58PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 23, 2020 at 02:20:15PM -0800, Christoph Hellwig wrote:
> > On Wed, Jan 22, 2020 at 11:42:03PM -0800, Darrick J. Wong wrote:
> > > -				return -ENOMEM;
> > > +			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
> > > +					0, &bp, &xfs_attr3_rmt_buf_ops);
> > > +			if (error)
> > > +				return error;
> > >  			error = bp->b_error;
> > >  			if (error) {
> > >  				xfs_buf_ioerror_alert(bp, __func__);
> > 
> > This still has the bogus b_error check where it should just check the
> > error and report it based on the return value.
> 
> Yes, because at this point midway through the series xfs_buf_read only
> knows how to return -ENOMEM.  I'm changing the *interfaces* here, saving
> most of the behavior changes for the xfs_buf_read_map change...
> 
> > > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > > index 56e081dd1d96..fb60c36a8a5b 100644
> > > --- a/fs/xfs/xfs_buf.h
> > > +++ b/fs/xfs/xfs_buf.h
> > > @@ -213,16 +213,25 @@ xfs_buf_get(
> > >  	return xfs_buf_get_map(target, &map, 1, 0);
> > >  }
> > >  
> > > -static inline struct xfs_buf *
> > > +static inline 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;
> > >  	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> > > -	return xfs_buf_read_map(target, &map, 1, flags, ops);
> > > +
> > > +	*bpp = NULL;
> > > +	bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> > > +	if (!bp)
> > > +		return -ENOMEM;
> > > +
> > > +	*bpp = bp;
> 
> ...because otherwise I end having to migrate all the bp->b_error
> checking into this static inline function, which causes compile errors.
> 
> I could work around /that/ by moving the xfs_buf_read_map conversion
> earlier in the series, but I'd have to rebase the whole series to
> achieve the same end result, which seems pointless.
> 
> But I guess I could go mess around with it and see just how much of a
> pain doing that /actually/ is...

Changing the order to the following:

xfs: make xfs_buf_alloc return an error code
xfs: make xfs_buf_get_map return an error code
xfs: make xfs_buf_read_map return an error code
xfs: make xfs_buf_get return an error code
xfs: make xfs_buf_get_uncached return an error code
xfs: make xfs_buf_read return an error code
xfs: make xfs_trans_get_buf_map return an error code
xfs: make xfs_trans_get_buf return an error code
xfs: remove the xfs_btree_get_buf[ls] functions
xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers
xfs: remove unnecessary null pointer checks from _read_agf callers
xfs: fix xfs_buf_ioerror_alert location reporting

Was a lot less code churn than I thought it would be.

--D

> --D
> 
> > > +	return 0;
> > >  }
> > >  
> > >  static inline void
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 0d683fb96396..b29806846916 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -2745,10 +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 = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
> > > +			  buf_flags, &bp, NULL);
> > > +	if (error)
> > > +		return error;
> > >  	error = bp->b_error;
> > >  	if (error) {
> > 
> > .. and same here.
> > 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-23  7:42 ` [PATCH 05/12] xfs: make xfs_buf_read_map " Darrick J. Wong
  2020-01-23 22:24   ` Christoph Hellwig
@ 2020-01-24  1:31   ` Dave Chinner
  2020-01-24  4:34     ` Darrick J. Wong
  1 sibling, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  1:31 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch

On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert xfs_buf_read_map() to return numeric error codes like most
> everywhere else in xfs.  This involves moving the open-coded logic that
> reports metadata IO read / corruption errors and stales the buffer into
> xfs_buf_read_map so that the logic is all in one place.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
.....

> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index b5b3a78ef31c..56e7f8126cd7 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -298,36 +298,17 @@ 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;
> -	}
> -
> -	/*
> -	 * If we've had a read error, then the contents of the buffer are
> -	 * invalid and should not be used. To ensure that a followup read tries
> -	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
> -	 * mark the buffer stale. This ensures that anyone who has a current
> -	 * reference to the buffer will interpret it's contents correctly and
> -	 * future cache lookups will also treat it as an empty, uninitialised
> -	 * buffer.
> -	 */
> -	if (bp->b_error) {
> -		error = bp->b_error;
> -		if (!XFS_FORCED_SHUTDOWN(mp))
> -			xfs_buf_ioerror_alert(bp, __func__);
> -		bp->b_flags &= ~XBF_DONE;
> -		xfs_buf_stale(bp);
> -
> +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> +	switch (error) {
> +	case 0:
> +		break;
> +	case -EFSCORRUPTED:
> +	case -EIO:
>  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> -			xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
> -		xfs_buf_relse(bp);
> -
> -		/* bad CRC means corrupted metadata */
> -		if (error == -EFSBADCRC)
> -			error = -EFSCORRUPTED;
> +			xfs_force_shutdown(tp->t_mountp,
> +					SHUTDOWN_META_IO_ERROR);
> +		/* fall through */
> +	default:
>  		return error;
>  	}

Same question as Christoph - we're only trying to avoid ENOMEM and
EAGAIN errors from shutting down the filesystem here, right?
Every other type of IO error that could end up on bp->b_error would
result in a shutdown, so perhaps this should be the other way
around:

	switch (error) {
	case 0:
		break;
	default:
		/* shutdown stuff */
		/* fall through */
	case -ENOMEM:
	case -EAGAIN:
		return error;
	}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 06/12] xfs: make xfs_buf_get_map return an error code
  2020-01-23  7:42 ` [PATCH 06/12] xfs: make xfs_buf_get_map " Darrick J. Wong
@ 2020-01-24  1:41   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  1:41 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:28PM -0800, Darrick J. Wong wrote:
> 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       |   45 ++++++++++++++++-----------------------------
>  fs/xfs/xfs_buf.h       |   14 +++-----------
>  fs/xfs/xfs_trans_buf.c |   14 +++++++++-----
>  3 files changed, 28 insertions(+), 45 deletions(-)

Looks good,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/12] xfs: make xfs_trans_get_buf_map return an error code
  2020-01-23  7:42 ` [PATCH 07/12] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
@ 2020-01-24  1:44   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  1:44 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:35PM -0800, Darrick J. Wong wrote:
> 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(-)

looks ok from here.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/12] xfs: make xfs_trans_get_buf return an error code
  2020-01-23  7:42 ` [PATCH 08/12] xfs: make xfs_trans_get_buf " Darrick J. Wong
@ 2020-01-24  1:48   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  1:48 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:41PM -0800, Darrick J. Wong wrote:
> 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(-)

Pretty straight forward.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions
  2020-01-23  7:42 ` [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
@ 2020-01-24  1:50   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  1:50 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:42:48PM -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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  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(-)

Nice simplification!

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers
  2020-01-23  7:42 ` [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
@ 2020-01-24  2:00   ` Dave Chinner
  2020-01-24  4:47     ` Darrick J. Wong
  0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  2:00 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch

On Wed, Jan 22, 2020 at 11:42:54PM -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 |   34 +++++++++++++++-------------------
>  fs/xfs/libxfs/xfs_bmap.c  |   11 ++++++-----
>  fs/xfs/xfs_filestream.c   |   11 ++++++-----
>  3 files changed, 27 insertions(+), 29 deletions(-)
.....
> @@ -2992,10 +2987,11 @@ xfs_alloc_read_agf(
>  	error = xfs_read_agf(mp, tp, agno,
>  			(flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
>  			bpp);
> -	if (error)
> +	if (error) {
> +		/* We don't support trylock when freeing. */
> +		ASSERT(error != -EAGAIN || !(flags & XFS_ALLOC_FLAG_FREEING));
>  		return error;

Shouldn't we check this with asserts before we call xfs_read_agf()?
i.e.

	/* We don't support trylock when freeing. */
	ASSERT((flags & (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK)) !=
			(XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK));
	....

> -	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..9a6d7a84689a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3311,11 +3311,12 @@ 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) {
> -			*notinit = 1;
> +		if (error) {
> +			/* Couldn't lock the AGF, so skip this AG. */
> +			if (error == -EAGAIN) {
> +				*notinit = 1;
> +				error = 0;
> +			}
>  			goto out;
>  		}
>  	}
> 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 neater to do:

		if (!pag->pagf_init) {
			err = xfs_alloc_pagf_init(mp, NULL, ag, trylock);
			if (err) {
				xfs_perag_put(pag);
				if (err != -EAGAIN)
					return err;
				/* Couldn't lock the AGF, skip this AG. */
				continue;
			}
		}

Otherwise it all looks ok.

-Dave
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers
  2020-01-23  7:43 ` [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
@ 2020-01-24  2:01   ` Dave Chinner
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  2:01 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, Christoph Hellwig

On Wed, Jan 22, 2020 at 11:43:01PM -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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  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(-)

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting
  2020-01-23  7:43 ` [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting Darrick J. Wong
@ 2020-01-24  2:07   ` Dave Chinner
  2020-01-24  4:21     ` Darrick J. Wong
  0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2020-01-24  2:07 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch

On Wed, Jan 22, 2020 at 11:43:07PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Instead of passing __func__ to the error reporting function, let's use
> the return address builtins so that the messages actually tell you which
> higher level function called the buffer functions.  This was previously
> true for the xfs_buf_read callers, but not for the xfs_trans_read_buf
> callers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_buf.c         |   12 +++++++-----
>  fs/xfs/xfs_buf.h         |    7 ++++---
>  fs/xfs/xfs_buf_item.c    |    2 +-
>  fs/xfs/xfs_log_recover.c |    4 ++--
>  fs/xfs/xfs_trans_buf.c   |    5 +++--
>  5 files changed, 17 insertions(+), 13 deletions(-)

Makes sense, but I have a concern that this series now has added two
new parameters to the buffer read functions. Perhaps we should consider
wrapping this all up in an args structure? That's a separate piece
of work, not for this patchset.

So far this patch,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting
  2020-01-24  2:07   ` Dave Chinner
@ 2020-01-24  4:21     ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  4:21 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, hch

On Fri, Jan 24, 2020 at 01:07:27PM +1100, Dave Chinner wrote:
> On Wed, Jan 22, 2020 at 11:43:07PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Instead of passing __func__ to the error reporting function, let's use
> > the return address builtins so that the messages actually tell you which
> > higher level function called the buffer functions.  This was previously
> > true for the xfs_buf_read callers, but not for the xfs_trans_read_buf
> > callers.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_buf.c         |   12 +++++++-----
> >  fs/xfs/xfs_buf.h         |    7 ++++---
> >  fs/xfs/xfs_buf_item.c    |    2 +-
> >  fs/xfs/xfs_log_recover.c |    4 ++--
> >  fs/xfs/xfs_trans_buf.c   |    5 +++--
> >  5 files changed, 17 insertions(+), 13 deletions(-)
> 
> Makes sense, but I have a concern that this series now has added two
> new parameters to the buffer read functions. Perhaps we should consider
> wrapping this all up in an args structure?

Yes.  Next cycle I'll try to cough up a patchset to turn this into an
args structure so that callers can pass additional things like health
reporting breadcrumbs to the verifiers so that we can update that status
without having to explode a ton of error handling boilerplate all over
the place.

--D

> That's a separate piece
> of work, not for this patchset.
> 
> So far this patch,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> -- 
> Dave Chinner
> david@fromorbit.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-24  1:31   ` Dave Chinner
@ 2020-01-24  4:34     ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  4:34 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, hch

On Fri, Jan 24, 2020 at 12:31:52PM +1100, Dave Chinner wrote:
> On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert xfs_buf_read_map() to return numeric error codes like most
> > everywhere else in xfs.  This involves moving the open-coded logic that
> > reports metadata IO read / corruption errors and stales the buffer into
> > xfs_buf_read_map so that the logic is all in one place.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> .....
> 
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index b5b3a78ef31c..56e7f8126cd7 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -298,36 +298,17 @@ 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;
> > -	}
> > -
> > -	/*
> > -	 * If we've had a read error, then the contents of the buffer are
> > -	 * invalid and should not be used. To ensure that a followup read tries
> > -	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
> > -	 * mark the buffer stale. This ensures that anyone who has a current
> > -	 * reference to the buffer will interpret it's contents correctly and
> > -	 * future cache lookups will also treat it as an empty, uninitialised
> > -	 * buffer.
> > -	 */
> > -	if (bp->b_error) {
> > -		error = bp->b_error;
> > -		if (!XFS_FORCED_SHUTDOWN(mp))
> > -			xfs_buf_ioerror_alert(bp, __func__);
> > -		bp->b_flags &= ~XBF_DONE;
> > -		xfs_buf_stale(bp);
> > -
> > +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> > +	switch (error) {
> > +	case 0:
> > +		break;
> > +	case -EFSCORRUPTED:
> > +	case -EIO:
> >  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> > -			xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
> > -		xfs_buf_relse(bp);
> > -
> > -		/* bad CRC means corrupted metadata */
> > -		if (error == -EFSBADCRC)
> > -			error = -EFSCORRUPTED;
> > +			xfs_force_shutdown(tp->t_mountp,
> > +					SHUTDOWN_META_IO_ERROR);
> > +		/* fall through */
> > +	default:
> >  		return error;
> >  	}
> 
> Same question as Christoph - we're only trying to avoid ENOMEM and
> EAGAIN errors from shutting down the filesystem here, right?
> Every other type of IO error that could end up on bp->b_error would
> result in a shutdown, so perhaps this should be the other way
> around:
> 
> 	switch (error) {
> 	case 0:
> 		break;
> 	default:
> 		/* shutdown stuff */
> 		/* fall through */
> 	case -ENOMEM:
> 	case -EAGAIN:
> 		return error;
> 	}

I agree that ENOMEM ought to be on the list of things that don't
immediately cause a shutdown if the transaction is dirty.

--D

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers
  2020-01-24  2:00   ` Dave Chinner
@ 2020-01-24  4:47     ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  4:47 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, hch

On Fri, Jan 24, 2020 at 01:00:54PM +1100, Dave Chinner wrote:
> On Wed, Jan 22, 2020 at 11:42:54PM -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 |   34 +++++++++++++++-------------------
> >  fs/xfs/libxfs/xfs_bmap.c  |   11 ++++++-----
> >  fs/xfs/xfs_filestream.c   |   11 ++++++-----
> >  3 files changed, 27 insertions(+), 29 deletions(-)
> .....
> > @@ -2992,10 +2987,11 @@ xfs_alloc_read_agf(
> >  	error = xfs_read_agf(mp, tp, agno,
> >  			(flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
> >  			bpp);
> > -	if (error)
> > +	if (error) {
> > +		/* We don't support trylock when freeing. */
> > +		ASSERT(error != -EAGAIN || !(flags & XFS_ALLOC_FLAG_FREEING));
> >  		return error;
> 
> Shouldn't we check this with asserts before we call xfs_read_agf()?
> i.e.
> 
> 	/* We don't support trylock when freeing. */
> 	ASSERT((flags & (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK)) !=
> 			(XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK));
> 	....

Yeah.

> > -	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..9a6d7a84689a 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3311,11 +3311,12 @@ 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) {
> > -			*notinit = 1;
> > +		if (error) {
> > +			/* Couldn't lock the AGF, so skip this AG. */
> > +			if (error == -EAGAIN) {
> > +				*notinit = 1;
> > +				error = 0;
> > +			}
> >  			goto out;
> >  		}
> >  	}
> > 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 neater to do:
> 
> 		if (!pag->pagf_init) {
> 			err = xfs_alloc_pagf_init(mp, NULL, ag, trylock);
> 			if (err) {
> 				xfs_perag_put(pag);
> 				if (err != -EAGAIN)
> 					return err;
> 				/* Couldn't lock the AGF, skip this AG. */
> 				continue;
> 			}
> 		}
> 
> Otherwise it all looks ok.

Cool.  Will fix.

--D

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code
  2020-01-24  0:23     ` Darrick J. Wong
@ 2020-01-24  4:59       ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  4:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, david

On Thu, Jan 23, 2020 at 04:23:21PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 23, 2020 at 02:24:41PM -0800, Christoph Hellwig wrote:
> > On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> > > index fc93fd88ec89..df25024275a1 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > @@ -2956,14 +2956,13 @@ 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;
> > > -	if (!*bpp)
> > > -		return 0;
> > 
> > Shouldn't the change in calling conventions for xfs_trans_read_buf be
> > in another patch dealing just with xfs_trans_read_buf?
> 
> Actually ... it really needs to be in the next patch because it's the
> xfs_buf_get_map transition that makes it so that xfs_trans_read_buf can
> return EAGAIN.

Now that I've reshuffled the whole patchset I realize that it more or
less has to be this way because this particular change insulates the
callers of xfs_read_agf() from needing to learn about EAGAIN right now.

I /could/ change all of those callers in this patch instead of handling
it separately in "xfs: make xfs_*read_agf return EAGAIN to
ALLOC_FLAG_TRYLOCK callers", but now the patch would be changing the
behavior of three separate API calls, and I'm trying to avoid
monsters like that.

(Anyway, onward to v5...)

> > > +		/* bad CRC means corrupted metadata */
> > > +		if (error == -EFSBADCRC)
> > > +			error = -EFSCORRUPTED;
> > > +		return error;
> > 
> > Note that this coukd and should now also go away in the xfs_buf_read()
> > callers, not just the direct xfs_buf_read_map ones.
> 
> Huh?  This patch /does/ remove the EFSBADCRC->EFSCORRUPTED code in the
> xfs_buf_read callers... <confused>

The reshuffle makes adding this bit unnecessary since I converted
xfs_buf_read_map earlier in the sequence.

> > > +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> > > +	switch (error) {
> > > +	case 0:
> > > +		break;
> > > +	case -EFSCORRUPTED:
> > > +	case -EIO:
> > >  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> > > +			xfs_force_shutdown(tp->t_mountp,
> > > +					SHUTDOWN_META_IO_ERROR);
> > > +		/* fall through */
> > > +	default:
> > 
> > Isn't it really EAGAIN the only special case here?  I.e. something
> > more like:
> > 
> > 	if (error && error != -EAGAIN) {
> >   		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> > 			xfs_force_shutdown(tp->t_mountp,
> > 					SHUTDOWN_META_IO_ERROR);
> > 	}
> > 
> > 	return error;
> 
> Yes, I think so.
> 
> --D

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers
  2020-01-24  5:18 [PATCH v5 00/12] xfs: make buffer functions return error codes Darrick J. Wong
@ 2020-01-24  5:20 ` Darrick J. Wong
  0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2020-01-24  5:20 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs, hch, david, Christoph Hellwig, Dave Chinner

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.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 e723b267a247..b0ce04ffd3cd 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);
 


^ permalink raw reply related	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2020-01-24  5:20 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23  7:41 [PATCH v4 00/12] xfs: make buffer functions return error codes Darrick J. Wong
2020-01-23  7:41 ` [PATCH 01/12] xfs: make xfs_buf_alloc return an error code Darrick J. Wong
2020-01-24  0:47   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 02/12] xfs: make xfs_buf_read " Darrick J. Wong
2020-01-23 22:20   ` Christoph Hellwig
2020-01-24  0:16     ` Darrick J. Wong
2020-01-24  1:08       ` Darrick J. Wong
2020-01-24  0:49   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 03/12] xfs: make xfs_buf_get " Darrick J. Wong
2020-01-24  0:50   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 04/12] xfs: make xfs_buf_get_uncached " Darrick J. Wong
2020-01-24  0:53   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 05/12] xfs: make xfs_buf_read_map " Darrick J. Wong
2020-01-23 22:24   ` Christoph Hellwig
2020-01-24  0:23     ` Darrick J. Wong
2020-01-24  4:59       ` Darrick J. Wong
2020-01-24  1:31   ` Dave Chinner
2020-01-24  4:34     ` Darrick J. Wong
2020-01-23  7:42 ` [PATCH 06/12] xfs: make xfs_buf_get_map " Darrick J. Wong
2020-01-24  1:41   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 07/12] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
2020-01-24  1:44   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 08/12] xfs: make xfs_trans_get_buf " Darrick J. Wong
2020-01-24  1:48   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 09/12] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
2020-01-24  1:50   ` Dave Chinner
2020-01-23  7:42 ` [PATCH 10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
2020-01-24  2:00   ` Dave Chinner
2020-01-24  4:47     ` Darrick J. Wong
2020-01-23  7:43 ` [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
2020-01-24  2:01   ` Dave Chinner
2020-01-23  7:43 ` [PATCH 12/12] xfs: fix xfs_buf_ioerror_alert location reporting Darrick J. Wong
2020-01-24  2:07   ` Dave Chinner
2020-01-24  4:21     ` Darrick J. Wong
2020-01-24  5:18 [PATCH v5 00/12] xfs: make buffer functions return error codes Darrick J. Wong
2020-01-24  5:20 ` [PATCH 11/12] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.