diff mbox series

[V3,06/12] xfs: xfs_dfork_nextents: Return extent count via an out argument

Message ID 20210916100647.176018-7-chandan.babu@oracle.com (mailing list archive)
State New, archived
Headers show
Series xfs: Extend per-inode extent counters | expand

Commit Message

Chandan Babu R Sept. 16, 2021, 10:06 a.m. UTC 
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 <chandan.babu@oracle.com>
---
 fs/xfs/libxfs/xfs_format.h     | 14 ++---
 fs/xfs/libxfs/xfs_inode_buf.c  | 16 ++++--
 fs/xfs/libxfs/xfs_inode_fork.c | 21 ++++++--
 fs/xfs/scrub/inode.c           | 94 +++++++++++++++++++++-------------
 fs/xfs/scrub/inode_repair.c    | 34 ++++++++----
 5 files changed, 118 insertions(+), 61 deletions(-)

Comments

Dave Chinner Sept. 30, 2021, 1:19 a.m. UTC | #1 
On Thu, Sep 16, 2021 at 03:36:41PM +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 <chandan.babu@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h     | 14 ++---
>  fs/xfs/libxfs/xfs_inode_buf.c  | 16 ++++--
>  fs/xfs/libxfs/xfs_inode_fork.c | 21 ++++++--
>  fs/xfs/scrub/inode.c           | 94 +++++++++++++++++++++-------------
>  fs/xfs/scrub/inode_repair.c    | 34 ++++++++----
>  5 files changed, 118 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index b4638052801f..dba868f2c3e3 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -931,28 +931,30 @@ enum xfs_dinode_fmt {
>  		(dip)->di_format : \
>  		(dip)->di_aformat)
>  
> -static inline xfs_extnum_t
> +static inline int
>  xfs_dfork_nextents(
>  	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 = -EFSCORRUPTED;
>  		break;
>  	}
>  
> -	return nextents;
> +	return error;
>  }

So why do we need to do this? AFAICT, the only check that can return
errors that is added by the ned of the patch series is a
on-disk-format check that does:

	if (inode_has_nrext64 && dip->di_nextents16 != 0)
		return -EFSCORRUPTED;

This doesn't belong here - it is conflating verification with
extraction. Verfication of the on-disk format belongs in the
verifiers or verification code, not in the function that extracts

>  /*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 176c98798aa4..dc511630cc7a 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(dip, whichfork);
> +	if (xfs_dfork_nextents(dip, whichfork, &di_nextents))
> +		return __this_address;
>  
>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>  	case XFS_DINODE_FMT_LOCAL:
> @@ -477,6 +478,7 @@ xfs_dinode_verify(
>  	uint64_t		flags2;
>  	uint64_t		di_size;
>  	xfs_extnum_t            nextents;
> +	xfs_extnum_t            naextents;
>  	xfs_rfsblock_t		nblocks;
>  
>  	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
> @@ -508,8 +510,13 @@ xfs_dinode_verify(
>  	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
>  		return __this_address;
>  
> -	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
> -	nextents += xfs_dfork_nextents(dip, XFS_ATTR_FORK);
> +	if (xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents))
> +		return __this_address;
> +
> +	if (xfs_dfork_nextents(dip, XFS_ATTR_FORK, &naextents))
> +		return __this_address;

Yeah, so this should end up being:

xfs_failaddr_t
xfs_dfork_nextents_verify(
	... )
{
	if (ip->di_flags2 & NREXT64) {
		if (!xfs_has_nrext64(mp))
			return __this_address;
		if (dip->di_nextents16 != 0)
			return __this_address;
	} else if (dip->di_nextents64 != 0)
		return __this_address;
	}
	return NULL;
}

and
	faddr = xfs_dfork_nextents_verify(dip, mp);
	if (faddr)
		return faddr;
	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
	naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);

Now all the verification can be removed from xfs_dfork_nextents(),
and anything that needs to verify that the extent counts are in a
valid format can call xfs_dfork_nextents_verify() to perform this
check (i.e. the dinode verifiers and scrub checking code).

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index b4638052801f..dba868f2c3e3 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -931,28 +931,30 @@  enum xfs_dinode_fmt {
 		(dip)->di_format : \
 		(dip)->di_aformat)
 
-static inline xfs_extnum_t
+static inline int
 xfs_dfork_nextents(
 	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 = -EFSCORRUPTED;
 		break;
 	}
 
-	return nextents;
+	return error;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 176c98798aa4..dc511630cc7a 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(dip, whichfork);
+	if (xfs_dfork_nextents(dip, whichfork, &di_nextents))
+		return __this_address;
 
 	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
 	case XFS_DINODE_FMT_LOCAL:
@@ -477,6 +478,7 @@  xfs_dinode_verify(
 	uint64_t		flags2;
 	uint64_t		di_size;
 	xfs_extnum_t            nextents;
+	xfs_extnum_t            naextents;
 	xfs_rfsblock_t		nblocks;
 
 	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
@@ -508,8 +510,13 @@  xfs_dinode_verify(
 	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
 		return __this_address;
 
-	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
-	nextents += xfs_dfork_nextents(dip, XFS_ATTR_FORK);
+	if (xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents))
+		return __this_address;
+
+	if (xfs_dfork_nextents(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 */
@@ -570,7 +577,8 @@  xfs_dinode_verify(
 		default:
 			return __this_address;
 		}
-		if (xfs_dfork_nextents(dip, XFS_ATTR_FORK))
+
+		if (naextents)
 			return __this_address;
 	}
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 7d1efccfea59..435c343612e2 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(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(dip, whichfork, &nex);
+	if (error)
+		return error;
+
+	size = nex * sizeof(struct xfs_bmbt_rec);
+
 	/*
 	 * 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.
@@ -234,7 +241,9 @@  xfs_iformat_data_fork(
 	 * depend on it.
 	 */
 	ip->i_df.if_format = dip->di_format;
-	ip->i_df.if_nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
+	error = xfs_dfork_nextents(dip, XFS_DATA_FORK, &ip->i_df.if_nextents);
+	if (error)
+		return error;
 
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFIFO:
@@ -302,9 +311,11 @@  xfs_iformat_attr_fork(
 	struct xfs_dinode	*dip)
 {
 	xfs_extnum_t		naextents;
-	int			error = 0;
+	int			error;
 
-	naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);
+	error = xfs_dfork_nextents(dip, XFS_ATTR_FORK, &naextents);
+	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 4177b85c941d..be43bd6be1ed 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -221,6 +221,38 @@  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;
+
+	fork_recs = XFS_DFORK_SIZE(dip, mp, whichfork) /
+		sizeof(struct xfs_bmbt_rec);
+	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(
@@ -229,7 +261,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;
@@ -237,6 +268,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)
@@ -392,33 +424,30 @@  xchk_dinode(
 	xchk_inode_extsize(sc, dip, ino, mode, flags);
 
 	/* di_nextents */
-	nextents = xfs_dfork_nextents(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;
+	error = xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents);
+	if (error) {
+		xchk_ino_set_corrupt(sc, ino);
+		return;
 	}
-
-	naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);
+	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(dip, XFS_ATTR_FORK, &naextents);
+	if (error) {
+		xchk_ino_set_corrupt(sc, ino);
+		return;
+	}
+
+	if (naextents != 0 && dip->di_forkoff == 0) {
+		xchk_ino_set_corrupt(sc, ino);
+		return;
+	}
+
 	/* di_aformat */
 	if (dip->di_aformat != XFS_DINODE_FMT_LOCAL &&
 	    dip->di_aformat != XFS_DINODE_FMT_EXTENTS &&
@@ -426,20 +455,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);
@@ -502,6 +519,7 @@  xchk_inode_xref_bmap(
 	struct xfs_scrub	*sc,
 	struct xfs_dinode	*dip)
 {
+	xfs_extnum_t		dip_nextents;
 	xfs_extnum_t		nextents;
 	xfs_filblks_t		count;
 	xfs_filblks_t		acount;
@@ -515,14 +533,18 @@  xchk_inode_xref_bmap(
 			&nextents, &count);
 	if (!xchk_should_check_xref(sc, &error, NULL))
 		return;
-	if (nextents < xfs_dfork_nextents(dip, XFS_DATA_FORK))
+
+	error = xfs_dfork_nextents(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(dip, XFS_ATTR_FORK))
+
+	error = xfs_dfork_nextents(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 ec8360b3b13b..4133a91c9a57 100644
--- a/fs/xfs/scrub/inode_repair.c
+++ b/fs/xfs/scrub/inode_repair.c
@@ -606,7 +606,9 @@  xrep_dinode_bad_extents_fork(
 	xfs_extnum_t		nex;
 	int			fork_size;
 
-	nex = xfs_dfork_nextents(dip, whichfork);
+	if (xfs_dfork_nextents(dip, whichfork, &nex))
+		return true;
+
 	fork_size = nex * sizeof(struct xfs_bmbt_rec);
 	if (fork_size < 0 || fork_size > dfork_size)
 		return true;
@@ -637,11 +639,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(dip, whichfork) <=
-			dfork_size / sizeof(struct xfs_bmbt_rec))
+	if (xfs_dfork_nextents(dip, whichfork, &nextents))
+		return true;
+
+	if (nextents <= dfork_size / sizeof(struct xfs_bmbt_rec))
 		return true;
 
 	if (dfork_size < sizeof(struct xfs_bmdr_block))
@@ -778,11 +783,15 @@  xrep_dinode_check_afork(
 	struct xfs_dinode		*dip)
 {
 	struct xfs_attr_shortform	*sfp;
+	xfs_extnum_t			nextents;
 	int				size;
 
+	if (xfs_dfork_nextents(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)) {
@@ -839,11 +848,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(dip, XFS_DATA_FORK);
-	anextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);
+	error = xfs_dfork_nextents(dip, XFS_DATA_FORK, &dnextents);
+	ASSERT(error == 0);
+
+	error = xfs_dfork_nextents(dip, XFS_ATTR_FORK, &anextents);
+	ASSERT(error == 0);
 
 	/*
 	 * Before calling this function, xrep_dinode_core ensured that both
@@ -1031,6 +1044,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);
 
@@ -1039,12 +1053,12 @@  xrep_dinode_zap_forks(
 	/* Inode counters don't make sense? */
 	nblocks = be64_to_cpu(dip->di_nblocks);
 
-	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
-	if (nextents > nblocks)
+	error = xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents);
+	if (error || nextents > nblocks)
 		zap_datafork = true;
 
-	naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);
-	if (naextents > nblocks)
+	error = xfs_dfork_nextents(dip, XFS_ATTR_FORK, &naextents);
+	if (error || naextents > nblocks)
 		zap_attrfork = true;
 
 	if (nextents + naextents > nblocks)