| Message ID | 20191120111727.16119-4-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [01/10] xfs: simplify mappedbno handling in xfs_da_{get,read}_buf | expand |
On Wed, Nov 20, 2019 at 12:17:20PM +0100, Christoph Hellwig wrote: > 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> > --- > fs/xfs/libxfs/xfs_da_btree.c | 39 +++++++++++++----------------------- > fs/xfs/libxfs/xfs_da_btree.h | 3 +++ > 2 files changed, 17 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index e078817fc26c..d85dd99d28a3 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,11 +2665,12 @@ xfs_da_reada_buf( > > mapp = ↦ > nmap = 1; > - 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. --D > error = 0; > goto out_free; > } > 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); > -- > 2.20.1 >
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.
On Wed, Nov 20, 2019 at 12:17:20PM +0100, Christoph Hellwig wrote: > 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> > --- > fs/xfs/libxfs/xfs_da_btree.c | 39 +++++++++++++----------------------- > fs/xfs/libxfs/xfs_da_btree.h | 3 +++ > 2 files changed, 17 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index e078817fc26c..d85dd99d28a3 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)) { Also I totally forgot to mention this flags test ^^^^^^^^ probably should have parentheses... --D > 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,11 +2665,12 @@ xfs_da_reada_buf( > > mapp = ↦ > nmap = 1; > - 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) > error = 0; > goto out_free; > } > 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); > -- > 2.20.1 >
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? --D
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.
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 = ↦ 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);
On Wed, Nov 20, 2019 at 11:15:30PM -0800, Darrick J. Wong wrote: > 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. This looks the same as the version I have. It creates a trivial conflict with one of the later patches that I've also fixed up.
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c index e078817fc26c..d85dd99d28a3 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,11 +2665,12 @@ xfs_da_reada_buf( mapp = ↦ nmap = 1; - 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) error = 0; goto out_free; } 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);
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> --- fs/xfs/libxfs/xfs_da_btree.c | 39 +++++++++++++----------------------- fs/xfs/libxfs/xfs_da_btree.h | 3 +++ 2 files changed, 17 insertions(+), 25 deletions(-)