Re: [PATCH 03/10] xfs: improve the xfs_dabuf_map calling conventions

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Nov 21, 2019 at 07:06:27AM +0100, Christoph Hellwig wrote:
> On Wed, Nov 20, 2019 at 09:44:58PM -0800, Darrick J. Wong wrote:
> > On Wed, Nov 20, 2019 at 07:20:35PM +0100, Christoph Hellwig wrote:
> > > On Wed, Nov 20, 2019 at 10:17:08AM -0800, Darrick J. Wong wrote:
> > > > > -	error = xfs_dabuf_map(dp, bno, mappedbno, whichfork,
> > > > > -				&mapp, &nmap);
> > > > > +	error = xfs_dabuf_map(dp, bno,
> > > > > +			mappedbno == -1 ? XFS_DABUF_MAP_HOLE_OK : 0,
> > > > > +			whichfork, &mapp, &nmap);
> > > > >  	if (error) {
> > > > >  		/* mapping a hole is not an error, but we don't continue */
> > > > > -		if (error == -1)
> > > > > +		if (error == -ENOENT)
> > > > 
> > > > Shouldn't this turn into:
> > > > 
> > > > if (error || !nmap)
> > > > 	goto out_free;
> > > > 
> > > > Otherwise looks ok to me.
> > > 
> > > Yes, it should.  Looks like that hunk got lost in the reshuffle.
> > 
> > With that and the other change I mentioned, it seems to test ok.  Do you
> > want to respin the patch, or just let me keep my staged version?
> 
> I've just done the respin and was about to re-start testing.  If you
> have a sensible version I'll skip that and will let you proceed.

Here's what I've been testing with, though FWIW I'm about to go to bed
so you might as well keep going, particularly if you see anything funny
here.

--D

xfs: improve the xfs_dabuf_map calling conventions

Use a flags argument with the XFS_DABUF_MAP_HOLE_OK flag to signal that
a hole is okay and not corruption, and return 0 with *nmap set to 0 to
signal that case in the return value instead of a nameless -1 return
code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: fix a few minor error and flags checking mistakes]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_da_btree.c |   43 ++++++++++++++----------------------------
 fs/xfs/libxfs/xfs_da_btree.h |    3 +++
 2 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index e078817fc26c..4d582a327c12 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -2460,19 +2460,11 @@ xfs_da_shrink_inode(
 	return error;
 }
 
-/*
- * Map the block we are given ready for reading. There are three possible return
- * values:
- *	-1 - will be returned if we land in a hole and mappedbno == -2 so the
- *	     caller knows not to execute a subsequent read.
- *	 0 - if we mapped the block successfully
- *	>0 - positive error number if there was an error.
- */
 static int
 xfs_dabuf_map(
 	struct xfs_inode	*dp,
 	xfs_dablk_t		bno,
-	xfs_daddr_t		mappedbno,
+	unsigned int		flags,
 	int			whichfork,
 	struct xfs_buf_map	**mapp,
 	int			*nmaps)
@@ -2527,7 +2519,7 @@ xfs_dabuf_map(
 
 invalid_mapping:
 	/* Caller ok with no mapping. */
-	if (XFS_IS_CORRUPT(mp, mappedbno != -2)) {
+	if (XFS_IS_CORRUPT(mp, !(flags & XFS_DABUF_MAP_HOLE_OK))) {
 		error = -EFSCORRUPTED;
 		if (xfs_error_level >= XFS_ERRLEVEL_LOW) {
 			xfs_alert(mp, "%s: bno %u inode %llu",
@@ -2575,13 +2567,11 @@ xfs_da_get_buf(
 		goto done;
 	}
 
-	error = xfs_dabuf_map(dp, bno, mappedbno, whichfork, &mapp, &nmap);
-	if (error) {
-		/* mapping a hole is not an error, but we don't continue */
-		if (error == -1)
-			error = 0;
+	error = xfs_dabuf_map(dp, bno,
+			mappedbno == -1 ? XFS_DABUF_MAP_HOLE_OK : 0,
+			whichfork, &mapp, &nmap);
+	if (error || nmap == 0)
 		goto out_free;
-	}
 
 	bp = xfs_trans_get_buf_map(tp, mp->m_ddev_targp, mapp, nmap, 0);
 done:
@@ -2630,13 +2620,11 @@ xfs_da_read_buf(
 		goto done;
 	}
 
-	error = xfs_dabuf_map(dp, bno, mappedbno, whichfork, &mapp, &nmap);
-	if (error) {
-		/* mapping a hole is not an error, but we don't continue */
-		if (error == -1)
-			error = 0;
+	error = xfs_dabuf_map(dp, bno,
+			mappedbno == -1 ? XFS_DABUF_MAP_HOLE_OK : 0,
+			whichfork, &mapp, &nmap);
+	if (error || !nmap)
 		goto out_free;
-	}
 
 	error = xfs_trans_read_buf_map(mp, tp, mp->m_ddev_targp, mapp, nmap, 0,
 			&bp, ops);
@@ -2677,14 +2665,11 @@ xfs_da_reada_buf(
 
 	mapp = &map;
 	nmap = 1;
-	error = xfs_dabuf_map(dp, bno, mappedbno, whichfork,
-				&mapp, &nmap);
-	if (error) {
-		/* mapping a hole is not an error, but we don't continue */
-		if (error == -1)
-			error = 0;
+	error = xfs_dabuf_map(dp, bno,
+			mappedbno == -1 ? XFS_DABUF_MAP_HOLE_OK : 0,
+			whichfork, &mapp, &nmap);
+	if (error || !nmap)
 		goto out_free;
-	}
 
 	mappedbno = mapp[0].bm_bn;
 	xfs_buf_readahead_map(dp->i_mount->m_ddev_targp, mapp, nmap, ops);
diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index ed3b558a9c1a..64624d5717c9 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -194,6 +194,9 @@ int	xfs_da3_node_read(struct xfs_trans *tp, struct xfs_inode *dp,
 /*
  * Utility routines.
  */
+
+#define XFS_DABUF_MAP_HOLE_OK	(1 << 0)
+
 int	xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno);
 int	xfs_da_grow_inode_int(struct xfs_da_args *args, xfs_fileoff_t *bno,
 			      int count);