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

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

Commit Message

Darrick J. Wong Jan. 23, 2020, 7:42 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.

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(-)

Comments

Christoph Hellwig Jan. 23, 2020, 10:20 p.m. UTC | #1
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.
Darrick J. Wong Jan. 24, 2020, 12:16 a.m. UTC | #2
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.
>
Dave Chinner Jan. 24, 2020, 12:49 a.m. UTC | #3
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>
Darrick J. Wong Jan. 24, 2020, 1:08 a.m. UTC | #4
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.
> >

Patch
diff mbox series

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__);