On 28 Sep 2021 at 04:16, Dave Chinner wrote:

On Thu, Sep 16, 2021 at 03:36:40PM +0530, Chandan Babu R wrote:

quoted

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.

Ok. I will fix this.

quoted

+	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.

I will fix this too.

quoted

--- 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....

I will add this cleanup to my todo list. 

-- 
chandan