[02/11] xfs: make xfs_buf_read return an error code
diff mbox series

Message ID 157924222437.3029431.18011964422343623236.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: make buffer functions return error codes
Related show

Commit Message

Darrick J. Wong Jan. 17, 2020, 6:23 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

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

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

Comments

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

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

Also:

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


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

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

Cheers,

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

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

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

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

--D

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

Patch
diff mbox series

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