Re: [PATCH V2 06/12] xfs: xfs_dfork_nextents: Return extent count via an out argument

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jul 26, 2021 at 05:15:35PM +0530, Chandan Babu R wrote:
> This commit changes xfs_dfork_nextents() to return an error code. The extent
> count itself is now returned through an out argument. This facility will be
> used by a future commit to indicate an inconsistent ondisk extent count.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_buf.c  | 29 +++++++----
>  fs/xfs/libxfs/xfs_inode_buf.h  |  4 +-
>  fs/xfs/libxfs/xfs_inode_fork.c | 22 ++++++--
>  fs/xfs/scrub/inode.c           | 94 +++++++++++++++++++++-------------
>  fs/xfs/scrub/inode_repair.c    | 34 ++++++++----
>  5 files changed, 119 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 6bef0757fca4..9ed04da2e2b1 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -345,7 +345,8 @@ xfs_dinode_verify_fork(
>  	xfs_extnum_t		di_nextents;
>  	xfs_extnum_t		max_extents;
>  
> -	di_nextents = xfs_dfork_nextents(mp, dip, whichfork);
> +	if (xfs_dfork_nextents(mp, dip, whichfork, &di_nextents))
> +		return __this_address;
>  
>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>  	case XFS_DINODE_FMT_LOCAL:
> @@ -377,29 +378,31 @@ xfs_dinode_verify_fork(
>  	return NULL;
>  }
>  
> -xfs_extnum_t
> +int
>  xfs_dfork_nextents(
>  	struct xfs_mount	*mp,
>  	struct xfs_dinode	*dip,
> -	int			whichfork)
> +	int			whichfork,
> +	xfs_extnum_t		*nextents)
>  {
> -	xfs_extnum_t		nextents = 0;
> +	int			error = 0;
>  
>  	switch (whichfork) {
>  	case XFS_DATA_FORK:
> -		nextents = be32_to_cpu(dip->di_nextents);
> +		*nextents = be32_to_cpu(dip->di_nextents);
>  		break;
>  
>  	case XFS_ATTR_FORK:
> -		nextents = be16_to_cpu(dip->di_anextents);
> +		*nextents = be16_to_cpu(dip->di_anextents);
>  		break;
>  
>  	default:
>  		ASSERT(0);
> +		error = -EINVAL;

-EFSCORRUPTED?  We don't have a specific code for "your darn software
screwed up, hyuck!!" but I guess this will at least get peoples'
attention.

>  		break;
>  	}
>  
> -	return nextents;
> +	return error;
>  }
>  
>  static xfs_failaddr_t
> @@ -502,6 +505,7 @@ xfs_dinode_verify(
>  	uint64_t		flags2;
>  	uint64_t		di_size;
>  	xfs_extnum_t            nextents;
> +	xfs_extnum_t            naextents;
>  	int64_t			nblocks;
>  
>  	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
> @@ -533,8 +537,13 @@ 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);
> +	if (xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents))
> +		return __this_address;
> +
> +	if (xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &naextents))
> +		return __this_address;
> +
> +	nextents += naextents;
>  	nblocks = be64_to_cpu(dip->di_nblocks);
>  
>  	/* Fork checks carried over from xfs_iformat_fork */
> @@ -595,7 +604,7 @@ xfs_dinode_verify(
>  		default:
>  			return __this_address;
>  		}
> -		if (xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK))
> +		if (naextents)
>  			return __this_address;
>  	}
>  
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index ea2c35091609..20f796610d46 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -36,8 +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);
> +int xfs_dfork_nextents(struct xfs_mount *mp, struct xfs_dinode *dip,
> +		int whichfork, xfs_extnum_t *nextents);
>  
>  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 38dd2dfc31fa..7f7ffe29436d 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -107,13 +107,20 @@ 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(mp, dip, whichfork);
> -	int			size = nex * sizeof(xfs_bmbt_rec_t);
> +	xfs_extnum_t		nex;
> +	int			size;
>  	struct xfs_iext_cursor	icur;
>  	struct xfs_bmbt_rec	*dp;
>  	struct xfs_bmbt_irec	new;
> +	int			error;
>  	int			i;
>  
> +	error = xfs_dfork_nextents(mp, dip, whichfork, &nex);
> +	if (error)
> +		return error;
> +
> +	size = nex * sizeof(xfs_bmbt_rec_t);

sizeof(struct xfs_bmbt_rec);

(Please convert the old typedef usage when possible.)

> +
>  	/*
>  	 * If the number of extents is unreasonable, then something is wrong and
>  	 * we just bail out rather than crash in kmem_alloc() or memcpy() below.
> @@ -235,7 +242,10 @@ xfs_iformat_data_fork(
>  	 * depend on it.
>  	 */
>  	ip->i_df.if_format = dip->di_format;
> -	ip->i_df.if_nextents = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK);
> +	error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK,
> +			&ip->i_df.if_nextents);
> +	if (error)
> +		return error;
>  
>  	switch (inode->i_mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -304,9 +314,11 @@ xfs_iformat_attr_fork(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	xfs_extnum_t		nextents;
> -	int			error = 0;
> +	int			error;
>  
> -	nextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
> +	error = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &nextents);
> +	if (error)
> +		return error;
>  
>  	/*
>  	 * Initialize the extent count early, as the per-format routines may
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index a161dac31a6f..e9dc3749ea08 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -208,6 +208,44 @@ xchk_dinode_nsec(
>  		xchk_ino_set_corrupt(sc, ino);
>  }
>  
> +STATIC void
> +xchk_dinode_fork_recs(
> +	struct xfs_scrub	*sc,
> +	struct xfs_dinode	*dip,
> +	xfs_ino_t		ino,
> +	xfs_extnum_t		nextents,
> +	int			whichfork)
> +{
> +	struct xfs_mount	*mp = sc->mp;
> +	size_t			fork_recs;
> +	unsigned char		format;
> +
> +	if (whichfork == XFS_DATA_FORK) {
> +		fork_recs =  XFS_DFORK_DSIZE(dip, mp)
> +			/ sizeof(struct xfs_bmbt_rec);
> +		format = dip->di_format;
> +	} else if (whichfork == XFS_ATTR_FORK) {
> +		fork_recs =  XFS_DFORK_ASIZE(dip, mp)
> +			/ sizeof(struct xfs_bmbt_rec);
> +		format = dip->di_aformat;
> +	}

	fork_recs = XFS_DFORK_SIZE(dip, mp, whichfork);
	format = XFS_DFORK_FORMAT(dip, whichfork);

?

> +
> +	switch (format) {
> +	case XFS_DINODE_FMT_EXTENTS:
> +		if (nextents > fork_recs)
> +			xchk_ino_set_corrupt(sc, ino);
> +		break;
> +	case XFS_DINODE_FMT_BTREE:
> +		if (nextents <= fork_recs)
> +			xchk_ino_set_corrupt(sc, ino);
> +		break;
> +	default:
> +		if (nextents != 0)
> +			xchk_ino_set_corrupt(sc, ino);
> +		break;
> +	}
> +}
> +
>  /* Scrub all the ondisk inode fields. */
>  STATIC void
>  xchk_dinode(
> @@ -216,7 +254,6 @@ xchk_dinode(
>  	xfs_ino_t		ino)
>  {
>  	struct xfs_mount	*mp = sc->mp;
> -	size_t			fork_recs;
>  	unsigned long long	isize;
>  	uint64_t		flags2;
>  	xfs_extnum_t		nextents;
> @@ -224,6 +261,7 @@ xchk_dinode(
>  	prid_t			prid;
>  	uint16_t		flags;
>  	uint16_t		mode;
> +	int			error;
>  
>  	flags = be16_to_cpu(dip->di_flags);
>  	if (dip->di_version >= 3)
> @@ -379,33 +417,22 @@ xchk_dinode(
>  	xchk_inode_extsize(sc, dip, ino, mode, flags);
>  
>  	/* 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:
> -		if (nextents > fork_recs)
> -			xchk_ino_set_corrupt(sc, ino);
> -		break;
> -	case XFS_DINODE_FMT_BTREE:
> -		if (nextents <= fork_recs)
> -			xchk_ino_set_corrupt(sc, ino);
> -		break;
> -	default:
> -		if (nextents != 0)
> -			xchk_ino_set_corrupt(sc, ino);
> -		break;
> -	}
> -
> -	naextents = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK);
> +	error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents);
> +	if (error)
> +		xchk_ino_set_corrupt(sc, ino);
> +	else

	error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &nextents);
	if (error) {
		xchk_ino_set_corrupt(sc, ino);
		return;
	}
	xchk_dinode_fork_recs(sc, dip, ino, nextents, XFS_DATA_FORK);

At this point you might as well return; you have sufficient information
to generate the corruption report for userspace.

> +		xchk_dinode_fork_recs(sc, dip, ino, nextents, XFS_DATA_FORK);
>  
>  	/* di_forkoff */
>  	if (XFS_DFORK_APTR(dip) >= (char *)dip + mp->m_sb.sb_inodesize)
>  		xchk_ino_set_corrupt(sc, ino);
> -	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);
>  
> +	error = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &naextents);
> +	if (error || (naextents != 0 && dip->di_forkoff == 0))
> +		xchk_ino_set_corrupt(sc, ino);

Please keep these separate so that the debug tracepoints can capture
them as separate corruption sources.  Also, if xfs_dfork_nextents
returns an error, you might as well return since we have enough data to
generate the corruption report.

(The rest of the scrub and repair code changes look good, btw.)

--D

> +
>  	/* di_aformat */
>  	if (dip->di_aformat != XFS_DINODE_FMT_LOCAL &&
>  	    dip->di_aformat != XFS_DINODE_FMT_EXTENTS &&
> @@ -413,20 +440,8 @@ xchk_dinode(
>  		xchk_ino_set_corrupt(sc, ino);
>  
>  	/* di_anextents */
> -	fork_recs =  XFS_DFORK_ASIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
> -	switch (dip->di_aformat) {
> -	case XFS_DINODE_FMT_EXTENTS:
> -		if (naextents > fork_recs)
> -			xchk_ino_set_corrupt(sc, ino);
> -		break;
> -	case XFS_DINODE_FMT_BTREE:
> -		if (naextents <= fork_recs)
> -			xchk_ino_set_corrupt(sc, ino);
> -		break;
> -	default:
> -		if (naextents != 0)
> -			xchk_ino_set_corrupt(sc, ino);
> -	}
> +	if (!error)
> +		xchk_dinode_fork_recs(sc, dip, ino, naextents, XFS_ATTR_FORK);
>  
>  	if (dip->di_version >= 3) {
>  		xchk_dinode_nsec(sc, ino, dip, dip->di_crtime);
> @@ -490,6 +505,7 @@ xchk_inode_xref_bmap(
>  	struct xfs_dinode	*dip)
>  {
>  	struct xfs_mount	*mp = sc->mp;
> +	xfs_extnum_t		dip_nextents;
>  	xfs_extnum_t		nextents;
>  	xfs_filblks_t		count;
>  	xfs_filblks_t		acount;
> @@ -503,14 +519,18 @@ xchk_inode_xref_bmap(
>  			&nextents, &count);
>  	if (!xchk_should_check_xref(sc, &error, NULL))
>  		return;
> -	if (nextents < xfs_dfork_nextents(mp, dip, XFS_DATA_FORK))
> +
> +	error = xfs_dfork_nextents(mp, dip, XFS_DATA_FORK, &dip_nextents);
> +	if (error || nextents < dip_nextents)
>  		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 != xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK))
> +
> +	error = xfs_dfork_nextents(mp, dip, XFS_ATTR_FORK, &dip_nextents);
> +	if (error || nextents < dip_nextents)
>  		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 bdb4685923c0..521c8df00990 100644
> --- a/fs/xfs/scrub/inode_repair.c
> +++ b/fs/xfs/scrub/inode_repair.c
> @@ -602,7 +602,9 @@ xrep_dinode_bad_extents_fork(
>  	int			i;
>  	int			fork_size;
>  
> -	nex = xfs_dfork_nextents(sc->mp, dip, whichfork);
> +	if (xfs_dfork_nextents(sc->mp, dip, whichfork, &nex))
> +		return true;
> +
>  	fork_size = nex * sizeof(struct xfs_bmbt_rec);
>  	if (fork_size < 0 || fork_size > dfork_size)
>  		return true;
> @@ -633,11 +635,14 @@ xrep_dinode_bad_btree_fork(
>  	int			whichfork)
>  {
>  	struct xfs_bmdr_block	*dfp;
> +	xfs_extnum_t		nextents;
>  	int			nrecs;
>  	int			level;
>  
> -	if (xfs_dfork_nextents(sc->mp, dip, whichfork) <=
> -			dfork_size / sizeof(struct xfs_bmbt_rec))
> +	if (xfs_dfork_nextents(sc->mp, dip, whichfork, &nextents))
> +		return true;
> +
> +	if (nextents <= dfork_size / sizeof(struct xfs_bmbt_rec))
>  		return true;
>  
>  	if (dfork_size < sizeof(struct xfs_bmdr_block))
> @@ -774,11 +779,15 @@ xrep_dinode_check_afork(
>  	struct xfs_dinode		*dip)
>  {
>  	struct xfs_attr_shortform	*sfp;
> +	xfs_extnum_t			nextents;
>  	int				size;
>  
> +	if (xfs_dfork_nextents(sc->mp, dip, XFS_ATTR_FORK, &nextents))
> +		return true;
> +
>  	if (XFS_DFORK_BOFF(dip) == 0)
>  		return dip->di_aformat != XFS_DINODE_FMT_EXTENTS ||
> -		       dip->di_anextents != 0;
> +		       nextents != 0;
>  
>  	size = XFS_DFORK_SIZE(dip, sc->mp, XFS_ATTR_FORK);
>  	switch (XFS_DFORK_FORMAT(dip, XFS_ATTR_FORK)) {
> @@ -835,11 +844,15 @@ xrep_dinode_ensure_forkoff(
>  	size_t				bmdr_minsz = xfs_bmdr_space_calc(1);
>  	unsigned int			lit_sz = XFS_LITINO(sc->mp);
>  	unsigned int			afork_min, dfork_min;
> +	int				error;
>  
>  	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);
> +	error = xfs_dfork_nextents(sc->mp, dip, XFS_DATA_FORK, &dnextents);
> +	ASSERT(error == 0);
> +
> +	error = xfs_dfork_nextents(sc->mp, dip, XFS_ATTR_FORK, &anextents);
> +	ASSERT(error == 0);
>  
>  	/*
>  	 * Before calling this function, xrep_dinode_core ensured that both
> @@ -1027,6 +1040,7 @@ xrep_dinode_zap_forks(
>  	uint16_t			mode;
>  	bool				zap_datafork = false;
>  	bool				zap_attrfork = false;
> +	int				error;
>  
>  	trace_xrep_dinode_zap_forks(sc, dip);
>  
> @@ -1035,12 +1049,12 @@ xrep_dinode_zap_forks(
>  	/* Inode counters don't make sense? */
>  	nblocks = be64_to_cpu(dip->di_nblocks);
>  
> -	nextents = xfs_dfork_nextents(sc->mp, dip, XFS_DATA_FORK);
> -	if (nextents > nblocks)
> +	error = xfs_dfork_nextents(sc->mp, dip, XFS_DATA_FORK, &nextents);
> +	if (error || nextents > nblocks)
>  		zap_datafork = true;
>  
> -	naextents = xfs_dfork_nextents(sc->mp, dip, XFS_ATTR_FORK);
> -	if (naextents > nblocks)
> +	error = xfs_dfork_nextents(sc->mp, dip, XFS_ATTR_FORK, &naextents);
> +	if (error || naextents > nblocks)
>  		zap_attrfork = true;
>  
>  	if (nextents + naextents > nblocks)
> -- 
> 2.30.2
>