Re: [PATCH V3 05/12] xfs: Introduce xfs_dfork_nextents() helper
From: Dave Chinner <david@fromorbit.com>
Date: 2021-09-27 22:46:54
On Thu, Sep 16, 2021 at 03:36:40PM +0530, Chandan Babu R wrote:
quoted hunk ↗ jump to hunk
This commit replaces the macro XFS_DFORK_NEXTENTS() with the helper function xfs_dfork_nextents(). As of this commit, xfs_dfork_nextents() returns the same value as XFS_DFORK_NEXTENTS(). A future commit which extends inode's extent counter fields will add more logic to this helper. This commit also replaces direct accesses to xfs_dinode->di_[a]nextents with calls to xfs_dfork_nextents(). No functional changes have been made. Signed-off-by: Chandan Babu R <redacted> --- fs/xfs/libxfs/xfs_format.h | 28 +++++++++++++++++++++---- fs/xfs/libxfs/xfs_inode_buf.c | 16 +++++++++----- fs/xfs/libxfs/xfs_inode_fork.c | 10 +++++---- fs/xfs/scrub/inode.c | 18 +++++++++------- fs/xfs/scrub/inode_repair.c | 38 +++++++++++++++++++++------------- 5 files changed, 75 insertions(+), 35 deletions(-)diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index ed8a5354bcbf..b4638052801f 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h@@ -930,10 +930,30 @@ enum xfs_dinode_fmt { ((w) == XFS_DATA_FORK ? \ (dip)->di_format : \ (dip)->di_aformat) -#define XFS_DFORK_NEXTENTS(dip,w) \ - ((w) == XFS_DATA_FORK ? \ - be32_to_cpu((dip)->di_nextents) : \ - be16_to_cpu((dip)->di_anextents)) + +static inline xfs_extnum_t +xfs_dfork_nextents( + struct xfs_dinode *dip, + int whichfork) +{ + xfs_extnum_t nextents = 0; + + switch (whichfork) { + case XFS_DATA_FORK: + nextents = be32_to_cpu(dip->di_nextents); + break; +
No need for whitespace line after the break, and this could just return the value directly.
+ case XFS_ATTR_FORK: + nextents = be16_to_cpu(dip->di_anextents); + break; + + default: + ASSERT(0); + break; + } + + return nextents; +}
I think that all the conditional inode fork macros
should be moved to libxfs/xfs_inode_fork.h as they are converted.
These macros are not acutally part of the on-disk format definition
(which is what xfs_format.h is supposed to contain) - it's code that
parses the on-disk format and that is supposed to be in
libxfs/xfs_inode_fork.[ch]....
Next thing: the caller almost always knows what fork it wants
the extents for - only 3 callers have a whichfork variable. So,
perhaps:
static inline xfs_extnum_t
xfs_dfork_data_extents(
struct xfs_dinode *dip)
{
return be32_to_cpu(dip->di_nextents);
}
static inline xfs_extnum_t
xfs_dfork_attr_extents(
struct xfs_dinode *dip)
{
return be16_to_cpu(dip->di_anextents);
}
static inline xfs_extnum_t
xfs_dfork_extents(
struct xfs_dinode *dip,
int whichfork)
{
switch (whichfork) {
case XFS_DATA_FORK:
return xfs_dfork_data_extents(dip);
case XFS_ATTR_FORK:
return xfs_dfork_attr_extents(dip);
default:
ASSERT(0);
break;
}
return 0;
}
So we don't have to rely on the compiler optimising away the switch
statement correctly to produce optimal code.
quoted hunk ↗ jump to hunk
--- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c@@ -342,9 +342,11 @@ xfs_dinode_verify_fork( struct xfs_mount *mp, int whichfork) { - xfs_extnum_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); + xfs_extnum_t di_nextents; xfs_extnum_t max_extents; + di_nextents = xfs_dfork_nextents(dip, whichfork); + switch (XFS_DFORK_FORMAT(dip, whichfork)) { case XFS_DINODE_FMT_LOCAL: /*@@ -474,6 +476,8 @@ xfs_dinode_verify( uint16_t flags; uint64_t flags2; uint64_t di_size; + xfs_extnum_t nextents; + xfs_rfsblock_t nblocks;
That's a block number type, not a block count: typedef uint64_t xfs_rfsblock_t; /* blockno in filesystem (raw) */ .... typedef uint64_t xfs_filblks_t; /* number of blocks in a file */ The latter is the appropriate type to use here. Oh, the struct xfs_inode and the struct xfs_log_dinode makes this same type mistake. Ok, that's a cleanup for another day.... Cheers, Dave. -- Dave Chinner david@fromorbit.com