On Thu, Sep 16, 2021 at 03:36:40PM +0530, Chandan Babu R wrote:
> 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 <chandan.babu@xxxxxxxxxx>
> ---
> 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.
> --- 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@xxxxxxxxxxxxx
