Re: [PATCH V2 05/12] xfs: Introduce xfs_dfork_nextents() helper

[Chandan Babu R <chandanrlinux@gmail.com>]

On 28 Jul 2021 at 03:40, Darrick J. Wong wrote:
> On Mon, Jul 26, 2021 at 05:15:34PM +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
>
> Yay fewer shouty macros!
>
>> 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 <chandanrlinux@gmail.com>
>> ---
>>  fs/xfs/libxfs/xfs_format.h     |  4 ----
>>  fs/xfs/libxfs/xfs_inode_buf.c  | 41 +++++++++++++++++++++++++++++-----
>>  fs/xfs/libxfs/xfs_inode_buf.h  |  2 ++
>>  fs/xfs/libxfs/xfs_inode_fork.c | 12 ++++++----
>>  fs/xfs/scrub/inode.c           | 19 +++++++++-------
>>  fs/xfs/scrub/inode_repair.c    | 38 +++++++++++++++++++------------
>>  6 files changed, 81 insertions(+), 35 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 920e3f9c418f..001a4077a7c6 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -1166,10 +1166,6 @@ 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))
>>
>>  /*
>>   * For block and character special files the 32bit dev_t is stored at the
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index cba9a38f3270..6bef0757fca4 100644
>> --- 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(mp, dip, whichfork);
>> +
>>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>>  	case XFS_DINODE_FMT_LOCAL:
>>  		/*
>> @@ -375,6 +377,31 @@ xfs_dinode_verify_fork(
>>  	return NULL;
>>  }
>>
>> +xfs_extnum_t
>> +xfs_dfork_nextents(
>> +	struct xfs_mount	*mp,
>> +	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;
>> +
>> +	case XFS_ATTR_FORK:
>> +		nextents = be16_to_cpu(dip->di_anextents);
>> +		break;
>> +
>> +	default:
>> +		ASSERT(0);
>> +		break;
>> +	}
>> +
>> +	return nextents;
>> +}
>
> This could be a static inline function taking the place of
> XFS_DFORK_NEXTENTS instead of another new function with a stack frame,
> etc:
>
> static inline xfs_extnum_t
> xfs_dfork_nextents(
> 	struct xfs_mount	*mp,
> 	struct xfs_dinode	*dip,
> 	int			whichfork)
> {
> 	switch (whichfork) {
> 	case XFS_DATA_FORK:
> 		return be32_to_cpu(dip->di_nextents);
> 	case XFS_ATTR_FORK:
> 		return be16_to_cpu(dip->di_anextents);
> 	default:
> 		ASSERT(0);
> 		return 0;
> 	}
> }
>

I initially tried implementing this as an inline function defined in
xfs_format.h. However, this results in compilation issues since the definition
of 'struct xfs_mount' isn't available.

I could get it working by changing the first argument to 'struct xfs_sb *sbp'
and get each call to pass a pointer to the xfs super block. However that
results in many calls having the first argument mentioned as
"xfs_dfork_nextents(&mp->m_sb, ...);". I think the benefit of saving stack
space beats the inconvenience of having to explicitly write mp->m_sb in calls
to xfs_dfork_nextents().


>> +
>>  static xfs_failaddr_t
>>  xfs_dinode_verify_forkoff(
>>  	struct xfs_dinode	*dip,
>> @@ -474,6 +501,8 @@ xfs_dinode_verify(
>>  	uint16_t		flags;
>>  	uint64_t		flags2;
>>  	uint64_t		di_size;
>> +	xfs_extnum_t            nextents;
>> +	int64_t			nblocks;
>
> xfs_rfsblock_t, since that's the type we use for nblocks in xfs_inode.

Ok. I will fix it.
>
>>
>>  	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
>>  		return __this_address;
>> @@ -504,10 +533,12 @@ xfs_dinode_verify(
>>  	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
>>  		return __this_address;
>>
>> +	nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>> +	nextents += xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> +	nblocks = be64_to_cpu(dip->di_nblocks);
>> +
>>  	/* Fork checks carried over from xfs_iformat_fork */
>> -	if (mode &&
>> -	    be32_to_cpu(dip->di_nextents) + be16_to_cpu(dip->di_anextents) >
>> -			be64_to_cpu(dip->di_nblocks))
>> +	if (mode && nextents > nblocks)
>>  		return __this_address;
>>
>>  	if (mode && XFS_DFORK_BOFF(dip) > mp->m_sb.sb_inodesize)
>> @@ -564,7 +595,7 @@ xfs_dinode_verify(
>>  		default:
>>  			return __this_address;
>>  		}
>> -		if (dip->di_anextents)
>> +		if (xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK))
>>  			return __this_address;
>>  	}
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
>> index a30b7676098a..ea2c35091609 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.h
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
>> @@ -36,6 +36,8 @@ xfs_failaddr_t xfs_inode_validate_extsize(struct xfs_mount *mp,
>>  xfs_failaddr_t xfs_inode_validate_cowextsize(struct xfs_mount *mp,
>>  		uint32_t cowextsize, uint16_t mode, uint16_t flags,
>>  		uint64_t flags2);
>> +xfs_extnum_t xfs_dfork_nextents(struct xfs_mount *mp, struct xfs_dinode *dip,
>> +		int whichfork);
>>
>>  static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
>>  {
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index a1e40df585a3..38dd2dfc31fa 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -107,7 +107,7 @@ xfs_iformat_extents(
>>  	struct xfs_mount	*mp = ip->i_mount;
>>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>>  	int			state = xfs_bmap_fork_to_state(whichfork);
>> -	xfs_extnum_t		nex = XFS_DFORK_NEXTENTS(dip, whichfork);
>> +	xfs_extnum_t		nex = xfs_dfork_nextents(mp, dip, whichfork);
>>  	int			size = nex * sizeof(xfs_bmbt_rec_t);
>>  	struct xfs_iext_cursor	icur;
>>  	struct xfs_bmbt_rec	*dp;
>> @@ -226,6 +226,7 @@ xfs_iformat_data_fork(
>>  	struct xfs_inode	*ip,
>>  	struct xfs_dinode	*dip)
>>  {
>> +	struct xfs_mount	*mp = ip->i_mount;
>>  	struct inode		*inode = VFS_I(ip);
>>  	int			error;
>>
>> @@ -234,7 +235,7 @@ xfs_iformat_data_fork(
>>  	 * depend on it.
>>  	 */
>>  	ip->i_df.if_format = dip->di_format;
>> -	ip->i_df.if_nextents = be32_to_cpu(dip->di_nextents);
>> +	ip->i_df.if_nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>>
>>  	switch (inode->i_mode & S_IFMT) {
>>  	case S_IFIFO:
>> @@ -301,14 +302,17 @@ xfs_iformat_attr_fork(
>>  	struct xfs_inode	*ip,
>>  	struct xfs_dinode	*dip)
>>  {
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	xfs_extnum_t		nextents;
>
> naextents for consistency?

Ok. I will fix it.

>
>>  	int			error = 0;
>>
>> +	nextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> +
>>  	/*
>>  	 * Initialize the extent count early, as the per-format routines may
>>  	 * depend on it.
>>  	 */
>> -	ip->i_afp = xfs_ifork_alloc(dip->di_aformat,
>> -				be16_to_cpu(dip->di_anextents));
>> +	ip->i_afp = xfs_ifork_alloc(dip->di_aformat, nextents);
>>
>>  	switch (ip->i_afp->if_format) {
>>  	case XFS_DINODE_FMT_LOCAL:
>> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
>> index 246d11ca133f..a161dac31a6f 100644
>> --- a/fs/xfs/scrub/inode.c
>> +++ b/fs/xfs/scrub/inode.c
>> @@ -220,6 +220,7 @@ xchk_dinode(
>>  	unsigned long long	isize;
>>  	uint64_t		flags2;
>>  	xfs_extnum_t		nextents;
>> +	xfs_extnum_t		naextents;
>>  	prid_t			prid;
>>  	uint16_t		flags;
>>  	uint16_t		mode;
>> @@ -378,7 +379,7 @@ xchk_dinode(
>>  	xchk_inode_extsize(sc, dip, ino, mode, flags);
>>
>>  	/* di_nextents */
>> -	nextents = be32_to_cpu(dip->di_nextents);
>> +	nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
>>  	fork_recs =  XFS_DFORK_DSIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
>>  	switch (dip->di_format) {
>>  	case XFS_DINODE_FMT_EXTENTS:
>> @@ -395,10 +396,12 @@ xchk_dinode(
>>  		break;
>>  	}
>>
>> +	naextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
>> +
>>  	/* di_forkoff */
>>  	if (XFS_DFORK_APTR(dip) >= (char *)dip + mp->m_sb.sb_inodesize)
>>  		xchk_ino_set_corrupt(sc, ino);
>> -	if (dip->di_anextents != 0 && dip->di_forkoff == 0)
>> +	if (naextents != 0 && dip->di_forkoff == 0)
>>  		xchk_ino_set_corrupt(sc, ino);
>>  	if (dip->di_forkoff == 0 && dip->di_aformat != XFS_DINODE_FMT_EXTENTS)
>>  		xchk_ino_set_corrupt(sc, ino);
>> @@ -410,19 +413,18 @@ xchk_dinode(
>>  		xchk_ino_set_corrupt(sc, ino);
>>
>>  	/* di_anextents */
>> -	nextents = be16_to_cpu(dip->di_anextents);
>>  	fork_recs =  XFS_DFORK_ASIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
>>  	switch (dip->di_aformat) {
>>  	case XFS_DINODE_FMT_EXTENTS:
>> -		if (nextents > fork_recs)
>> +		if (naextents > fork_recs)
>>  			xchk_ino_set_corrupt(sc, ino);
>>  		break;
>>  	case XFS_DINODE_FMT_BTREE:
>> -		if (nextents <= fork_recs)
>> +		if (naextents <= fork_recs)
>>  			xchk_ino_set_corrupt(sc, ino);
>>  		break;
>>  	default:
>> -		if (nextents != 0)
>> +		if (naextents != 0)
>>  			xchk_ino_set_corrupt(sc, ino);
>>  	}
>>
>> @@ -487,6 +489,7 @@ xchk_inode_xref_bmap(
>>  	struct xfs_scrub	*sc,
>>  	struct xfs_dinode	*dip)
>>  {
>> +	struct xfs_mount	*mp = sc->mp;
>>  	xfs_extnum_t		nextents;
>>  	xfs_filblks_t		count;
>>  	xfs_filblks_t		acount;
>> @@ -500,14 +503,14 @@ xchk_inode_xref_bmap(
>>  			&nextents, &count);
>>  	if (!xchk_should_check_xref(sc, &error, NULL))
>>  		return;
>> -	if (nextents < be32_to_cpu(dip->di_nextents))
>> +	if (nextents < xfs_dfork_nextents(mp, dip, XFS_DATA_FORK))
>>  		xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
>>
>>  	error = xfs_bmap_count_blocks(sc->tp, sc->ip, XFS_ATTR_FORK,
>>  			&nextents, &acount);
>>  	if (!xchk_should_check_xref(sc, &error, NULL))
>>  		return;
>> -	if (nextents != be16_to_cpu(dip->di_anextents))
>> +	if (nextents != xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK))
>>  		xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
>>
>>  	/* Check nblocks against the inode. */
>> diff --git a/fs/xfs/scrub/inode_repair.c b/fs/xfs/scrub/inode_repair.c
>> index 042c7d0bc0f5..bdb4685923c0 100644
>> --- a/fs/xfs/scrub/inode_repair.c
>> +++ b/fs/xfs/scrub/inode_repair.c
>
> Hey waitaminute, is this based off of djwong-dev??

Increasing data extent counter width also causes the maximum height of BMBT to
increase. This requires that the macro XFS_BTREE_MAXLEVELS be updated with a
larger value. However such a change causes the value of mp->m_rmap_maxlevels
to increase which in turn causes log reservation sizes to increase and hence a
modified XFS driver will fail to mount filesystems created by older versions
of mkfs.xfs.

I based this patchset on top of btree-dynamic-depth branch since it removes
the macro XFS_BTREE_MAXLEVELS and computes mp->m_rmap_maxlevels based on the
size of an AG.

>
>> @@ -602,7 +602,7 @@ xrep_dinode_bad_extents_fork(
>>  	int			i;
>>  	int			fork_size;
>>
>> -	nex = XFS_DFORK_NEXTENTS(dip, whichfork);
>> +	nex = xfs_dfork_nextents(sc->mp, dip, whichfork);
>>  	fork_size = nex * sizeof(struct xfs_bmbt_rec);
>>  	if (fork_size < 0 || fork_size > dfork_size)
>>  		return true;
>> @@ -636,7 +636,7 @@ xrep_dinode_bad_btree_fork(
>>  	int			nrecs;
>>  	int			level;
>>
>> -	if (XFS_DFORK_NEXTENTS(dip, whichfork) <=
>> +	if (xfs_dfork_nextents(sc->mp, dip, whichfork) <=
>>  			dfork_size / sizeof(struct xfs_bmbt_rec))
>>  		return true;
>>
>> @@ -831,12 +831,16 @@ xrep_dinode_ensure_forkoff(
>>  	struct xrep_dinode_stats	*dis)
>>  {
>>  	struct xfs_bmdr_block		*bmdr;
>> +	xfs_extnum_t			anextents, dnextents;
>>  	size_t				bmdr_minsz = xfs_bmdr_space_calc(1);
>>  	unsigned int			lit_sz = XFS_LITINO(sc->mp);
>>  	unsigned int			afork_min, dfork_min;
>>
>>  	trace_xrep_dinode_ensure_forkoff(sc, dip);
>>
>> +	dnextents = xfs_dfork_nextents(sc->mp, dip, XFS_DATA_FORK);
>> +	anextents = xfs_dfork_nextents(sc->mp, dip, XFS_ATTR_FORK);
>> +
>>  	/*
>>  	 * Before calling this function, xrep_dinode_core ensured that both
>>  	 * forks actually fit inside their respective literal areas.  If this
>> @@ -857,15 +861,14 @@ xrep_dinode_ensure_forkoff(
>>  		afork_min = XFS_DFORK_SIZE(dip, sc->mp, XFS_ATTR_FORK);
>>  		break;
>>  	case XFS_DINODE_FMT_EXTENTS:
>> -		if (dip->di_anextents) {
>> +		if (anextents) {
>>  			/*
>>  			 * We must maintain sufficient space to hold the entire
>>  			 * extent map array in the data fork.  Note that we
>>  			 * previously zapped the fork if it had no chance of
>>  			 * fitting in the inode.
>>  			 */
>> -			afork_min = sizeof(struct xfs_bmbt_rec) *
>> -						be16_to_cpu(dip->di_anextents);
>> +			afork_min = sizeof(struct xfs_bmbt_rec) * anextents;
>>  		} else if (dis->attr_extents > 0) {
>>  			/*
>>  			 * The attr fork thinks it has zero extents, but we
>> @@ -908,15 +911,14 @@ xrep_dinode_ensure_forkoff(
>>  		dfork_min = be64_to_cpu(dip->di_size);
>>  		break;
>>  	case XFS_DINODE_FMT_EXTENTS:
>> -		if (dip->di_nextents) {
>> +		if (dnextents) {
>>  			/*
>>  			 * We must maintain sufficient space to hold the entire
>>  			 * extent map array in the data fork.  Note that we
>>  			 * previously zapped the fork if it had no chance of
>>  			 * fitting in the inode.
>>  			 */
>> -			dfork_min = sizeof(struct xfs_bmbt_rec) *
>> -						be32_to_cpu(dip->di_nextents);
>> +			dfork_min = sizeof(struct xfs_bmbt_rec) * dnextents;
>>  		} else if (dis->data_extents > 0 || dis->rt_extents > 0) {
>>  			/*
>>  			 * The data fork thinks it has zero extents, but we
>> @@ -956,7 +958,7 @@ xrep_dinode_ensure_forkoff(
>>  	 * recovery fork, move the attr fork up.
>>  	 */
>>  	if (dip->di_format == XFS_DINODE_FMT_EXTENTS &&
>> -	    dip->di_nextents == 0 &&
>> +	    dnextents == 0 &&
>>  	    (dis->data_extents > 0 || dis->rt_extents > 0) &&
>>  	    bmdr_minsz > XFS_DFORK_DSIZE(dip, sc->mp)) {
>>  		if (bmdr_minsz + afork_min > lit_sz) {
>> @@ -982,7 +984,7 @@ xrep_dinode_ensure_forkoff(
>>  	 * recovery fork, move the attr fork down.
>>  	 */
>>  	if (dip->di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> -	    dip->di_anextents == 0 &&
>> +	    anextents == 0 &&
>>  	    dis->attr_extents > 0 &&
>>  	    bmdr_minsz > XFS_DFORK_ASIZE(dip, sc->mp)) {
>>  		if (dip->di_format == XFS_DINODE_FMT_BTREE) {
>> @@ -1019,6 +1021,9 @@ xrep_dinode_zap_forks(
>>  	struct xfs_dinode		*dip,
>>  	struct xrep_dinode_stats	*dis)
>>  {
>> +	uint64_t			nblocks;
>
> xfs_rfsblock_t.

Sure. I will update it.

--
chandan