| Message ID | 20191116182214.23711-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] xfs: simplify mappedbno case from xfs_da_get_buf and xfs_da_read_buf | expand |
On Sat, Nov 16, 2019 at 07:22:07PM +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 -ENOENT instead of the > nameless -1 to signal that case in the return value. Why not set *nirecs = 0 and return 0 like we sometimes do for bmap lookups? --D > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_da_btree.c | 25 +++++++++++++++---------- > fs/xfs/libxfs/xfs_da_btree.h | 3 +++ > 2 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index 681fba5731c2..c26f139bcf00 100644 > --- a/fs/xfs/libxfs/xfs_da_btree.c > +++ b/fs/xfs/libxfs/xfs_da_btree.c > @@ -2571,7 +2571,7 @@ 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 **map, > int *nmaps) > @@ -2596,8 +2596,8 @@ xfs_dabuf_map( > > if (!xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb)) { > /* Caller ok with no mapping. */ > - if (mappedbno == -2) { > - error = -1; > + if (flags & XFS_DABUF_MAP_HOLE_OK) { > + error = -ENOENT; > goto out; > } > > @@ -2655,10 +2655,12 @@ xfs_da_get_buf( > goto done; > } > > - 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; > } > @@ -2710,10 +2712,12 @@ xfs_da_read_buf( > goto done; > } > > - 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; > } > @@ -2757,11 +2761,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 5af4df71e92b..9ec0d0243e96 100644 > --- a/fs/xfs/libxfs/xfs_da_btree.h > +++ b/fs/xfs/libxfs/xfs_da_btree.h > @@ -204,6 +204,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 Sun, Nov 17, 2019 at 10:35:21AM -0800, Darrick J. Wong wrote: > On Sat, Nov 16, 2019 at 07:22:07PM +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 -ENOENT instead of the > > nameless -1 to signal that case in the return value. > > Why not set *nirecs = 0 and return 0 like we sometimes do for bmap > lookups? Sure, I can change it to that for the next version.
On Mon, Nov 18, 2019 at 07:25:05AM +0100, Christoph Hellwig wrote: > On Sun, Nov 17, 2019 at 10:35:21AM -0800, Darrick J. Wong wrote: > > On Sat, Nov 16, 2019 at 07:22:07PM +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 -ENOENT instead of the > > > nameless -1 to signal that case in the return value. > > > > Why not set *nirecs = 0 and return 0 like we sometimes do for bmap > > lookups? > > Sure, I can change it to that for the next version. Also, I forgot to mention that some of the comments (particularly xfs_dabuf_map) need to be updated to reflect the new "no mapping" return style since there's no more @mappedbno, etc. --D
On Tue, Nov 19, 2019 at 09:12:05AM -0800, Darrick J. Wong wrote: > Also, I forgot to mention that some of the comments (particularly > xfs_dabuf_map) need to be updated to reflect the new "no mapping" return > style since there's no more @mappedbno, etc. That is the one comment on xfs_dabuf_map, which in fact is already wrong in the current tree. When applying your changes I just removed the comment entirely as it is a static function with just two callers right below it.
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c index 681fba5731c2..c26f139bcf00 100644 --- a/fs/xfs/libxfs/xfs_da_btree.c +++ b/fs/xfs/libxfs/xfs_da_btree.c @@ -2571,7 +2571,7 @@ 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 **map, int *nmaps) @@ -2596,8 +2596,8 @@ xfs_dabuf_map( if (!xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb)) { /* Caller ok with no mapping. */ - if (mappedbno == -2) { - error = -1; + if (flags & XFS_DABUF_MAP_HOLE_OK) { + error = -ENOENT; goto out; } @@ -2655,10 +2655,12 @@ xfs_da_get_buf( goto done; } - 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; } @@ -2710,10 +2712,12 @@ xfs_da_read_buf( goto done; } - 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; } @@ -2757,11 +2761,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 5af4df71e92b..9ec0d0243e96 100644 --- a/fs/xfs/libxfs/xfs_da_btree.h +++ b/fs/xfs/libxfs/xfs_da_btree.h @@ -204,6 +204,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 -ENOENT instead of the nameless -1 to signal that case in the return value. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_da_btree.c | 25 +++++++++++++++---------- fs/xfs/libxfs/xfs_da_btree.h | 3 +++ 2 files changed, 18 insertions(+), 10 deletions(-)