dinode reading cleanups v2

dinode reading cleanups v2

[Christoph Hellwig]

Hi all,

while dusting off my series to move the per-fork extent count and format
into the xfs_ifork structure I found that we added a few more hacks in
the area it touched.  This series has some of the prep patch combined with
a restructure of the dinode reading path that cleans up handling of early
errors during dinode reading, and allows droppign a workaround in
xfs_bmapi_read.

Another side effect is that we can share more code with xfsprogs.

Changes since v1:
 - remove the repair hack in the dir2 verifier
 - keep a NULL ifork safety check in xfs_bmapi_read
 - handle a NULL attr fork in xfs_ifork_verify_local_attr
 - cleanup various comments

Git tree:

    git://git.infradead.org/users/hch/xfs.git xfs-inode-read-cleanup.2

Gitweb:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-inode-read-cleanup.2

[PATCH 01/12] xfs: xfs_bmapi_read doesn't take a fork id as the last argument

[Christoph Hellwig]

The last argument to xfs_bmapi_raad contains XFS_BMAPI_* flags, not the
fork.  Given that XFS_DATA_FORK evaluates to 0 no real harm is done,
but let's fix this anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index f42c74cb8be53..9498ced947be9 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -66,7 +66,7 @@ xfs_rtbuf_get(
 
 	ip = issum ? mp->m_rsumip : mp->m_rbmip;
 
-	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
+	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, 0);
 	if (error)
 		return error;
 
-- 
2.26.2

Re: [PATCH 01/12] xfs: xfs_bmapi_read doesn't take a fork id as the last argument

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:12AM +0200, Christoph Hellwig wrote:
> The last argument to xfs_bmapi_raad contains XFS_BMAPI_* flags, not the
> fork.  Given that XFS_DATA_FORK evaluates to 0 no real harm is done,
> but let's fix this anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Heh.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index f42c74cb8be53..9498ced947be9 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -66,7 +66,7 @@ xfs_rtbuf_get(
>  
>  	ip = issum ? mp->m_rsumip : mp->m_rbmip;
>  
> -	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
> +	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, 0);
>  	if (error)
>  		return error;
>  
> -- 
> 2.26.2
> 

[PATCH 02/12] xfs: call xfs_iformat_fork from xfs_inode_from_disk

[Christoph Hellwig]

We always need to fill out the fork structures when reading the inode,
so call xfs_iformat_fork from the tail of xfs_inode_from_disk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 7 ++++---
 fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
 fs/xfs/xfs_log_recover.c      | 4 +---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 81a010422bea3..dc00ce6fc4a2f 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -180,7 +180,7 @@ xfs_imap_to_bp(
 	return 0;
 }
 
-void
+int
 xfs_inode_from_disk(
 	struct xfs_inode	*ip,
 	struct xfs_dinode	*from)
@@ -241,6 +241,8 @@ xfs_inode_from_disk(
 		to->di_flags2 = be64_to_cpu(from->di_flags2);
 		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
 	}
+
+	return xfs_iformat_fork(ip, from);
 }
 
 void
@@ -641,8 +643,7 @@ xfs_iread(
 	 * Otherwise, just get the truly permanent information.
 	 */
 	if (dip->di_mode) {
-		xfs_inode_from_disk(ip, dip);
-		error = xfs_iformat_fork(ip, dip);
+		error = xfs_inode_from_disk(ip, dip);
 		if (error)  {
 #ifdef DEBUG
 			xfs_alert(mp, "%s: xfs_iformat() returned error %d",
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index d9b4781ac9fd4..0fbb99224ec73 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -54,7 +54,7 @@ int	xfs_iread(struct xfs_mount *, struct xfs_trans *,
 void	xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
 void	xfs_inode_to_disk(struct xfs_inode *ip, struct xfs_dinode *to,
 			  xfs_lsn_t lsn);
-void	xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
+int	xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
 void	xfs_log_dinode_to_disk(struct xfs_log_dinode *from,
 			       struct xfs_dinode *to);
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 3207851158332..3960caf51c9f7 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2874,9 +2874,7 @@ xfs_recover_inode_owner_change(
 
 	/* instantiate the inode */
 	ASSERT(dip->di_version >= 3);
-	xfs_inode_from_disk(ip, dip);
-
-	error = xfs_iformat_fork(ip, dip);
+	error = xfs_inode_from_disk(ip, dip);
 	if (error)
 		goto out_free_ip;
 
-- 
2.26.2

Re: [PATCH 02/12] xfs: call xfs_iformat_fork from xfs_inode_from_disk

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:13AM +0200, Christoph Hellwig wrote:
> We always need to fill out the fork structures when reading the inode,
> so call xfs_iformat_fork from the tail of xfs_inode_from_disk.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 7 ++++---
>  fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
>  fs/xfs/xfs_log_recover.c      | 4 +---
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 81a010422bea3..dc00ce6fc4a2f 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -180,7 +180,7 @@ xfs_imap_to_bp(
>  	return 0;
>  }
>  
> -void
> +int
>  xfs_inode_from_disk(
>  	struct xfs_inode	*ip,
>  	struct xfs_dinode	*from)
> @@ -241,6 +241,8 @@ xfs_inode_from_disk(
>  		to->di_flags2 = be64_to_cpu(from->di_flags2);
>  		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
>  	}
> +
> +	return xfs_iformat_fork(ip, from);
>  }
>  
>  void
> @@ -641,8 +643,7 @@ xfs_iread(
>  	 * Otherwise, just get the truly permanent information.
>  	 */
>  	if (dip->di_mode) {
> -		xfs_inode_from_disk(ip, dip);
> -		error = xfs_iformat_fork(ip, dip);
> +		error = xfs_inode_from_disk(ip, dip);
>  		if (error)  {
>  #ifdef DEBUG
>  			xfs_alert(mp, "%s: xfs_iformat() returned error %d",
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index d9b4781ac9fd4..0fbb99224ec73 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -54,7 +54,7 @@ int	xfs_iread(struct xfs_mount *, struct xfs_trans *,
>  void	xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
>  void	xfs_inode_to_disk(struct xfs_inode *ip, struct xfs_dinode *to,
>  			  xfs_lsn_t lsn);
> -void	xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
> +int	xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
>  void	xfs_log_dinode_to_disk(struct xfs_log_dinode *from,
>  			       struct xfs_dinode *to);
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 3207851158332..3960caf51c9f7 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2874,9 +2874,7 @@ xfs_recover_inode_owner_change(
>  
>  	/* instantiate the inode */
>  	ASSERT(dip->di_version >= 3);
> -	xfs_inode_from_disk(ip, dip);
> -
> -	error = xfs_iformat_fork(ip, dip);
> +	error = xfs_inode_from_disk(ip, dip);
>  	if (error)
>  		goto out_free_ip;
>  
> -- 
> 2.26.2
> 

[PATCH 03/12] xfs: split xfs_iformat_fork

[Christoph Hellwig]

xfs_iformat_fork is a weird catchall.  Split it into one helper for
the data fork and one for the attr fork, and then call both helper
as well as the COW fork initialization from xfs_inode_from_disk.  Order
the COW fork initialization after the attr fork initialization given
that it can't fail to simplify the error handling.

Note that the newly split helpers are moved down the file in
xfs_inode_fork.c to avoid the need for forward declarations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_buf.c  |  20 +++-
 fs/xfs/libxfs/xfs_inode_fork.c | 186 +++++++++++++++------------------
 fs/xfs/libxfs/xfs_inode_fork.h |   3 +-
 3 files changed, 103 insertions(+), 106 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index dc00ce6fc4a2f..abdecc80579e3 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -187,6 +187,10 @@ xfs_inode_from_disk(
 {
 	struct xfs_icdinode	*to = &ip->i_d;
 	struct inode		*inode = VFS_I(ip);
+	int			error;
+
+	ASSERT(ip->i_cowfp == NULL);
+	ASSERT(ip->i_afp == NULL);
 
 	/*
 	 * Convert v1 inodes immediately to v2 inode format as this is the
@@ -242,7 +246,21 @@ xfs_inode_from_disk(
 		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
 	}
 
-	return xfs_iformat_fork(ip, from);
+	error = xfs_iformat_data_fork(ip, from);
+	if (error)
+		return error;
+	if (XFS_DFORK_Q(from)) {
+		error = xfs_iformat_attr_fork(ip, from);
+		if (error)
+			goto out_destroy_data_fork;
+	}
+	if (xfs_is_reflink_inode(ip))
+		xfs_ifork_init_cow(ip);
+	return 0;
+
+out_destroy_data_fork:
+	xfs_idestroy_fork(ip, XFS_DATA_FORK);
+	return error;
 }
 
 void
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 3e9a42f1e23b9..5fadfa9a17eb9 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -26,110 +26,6 @@
 
 kmem_zone_t *xfs_ifork_zone;
 
-STATIC int xfs_iformat_local(xfs_inode_t *, xfs_dinode_t *, int, int);
-STATIC int xfs_iformat_extents(xfs_inode_t *, xfs_dinode_t *, int);
-STATIC int xfs_iformat_btree(xfs_inode_t *, xfs_dinode_t *, int);
-
-/*
- * Copy inode type and data and attr format specific information from the
- * on-disk inode to the in-core inode and fork structures.  For fifos, devices,
- * and sockets this means set i_rdev to the proper value.  For files,
- * directories, and symlinks this means to bring in the in-line data or extent
- * pointers as well as the attribute fork.  For a fork in B-tree format, only
- * the root is immediately brought in-core.  The rest will be read in later when
- * first referenced (see xfs_iread_extents()).
- */
-int
-xfs_iformat_fork(
-	struct xfs_inode	*ip,
-	struct xfs_dinode	*dip)
-{
-	struct inode		*inode = VFS_I(ip);
-	struct xfs_attr_shortform *atp;
-	int			size;
-	int			error = 0;
-	xfs_fsize_t             di_size;
-
-	switch (inode->i_mode & S_IFMT) {
-	case S_IFIFO:
-	case S_IFCHR:
-	case S_IFBLK:
-	case S_IFSOCK:
-		ip->i_d.di_size = 0;
-		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
-		break;
-
-	case S_IFREG:
-	case S_IFLNK:
-	case S_IFDIR:
-		switch (dip->di_format) {
-		case XFS_DINODE_FMT_LOCAL:
-			di_size = be64_to_cpu(dip->di_size);
-			size = (int)di_size;
-			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK, size);
-			break;
-		case XFS_DINODE_FMT_EXTENTS:
-			error = xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
-			break;
-		case XFS_DINODE_FMT_BTREE:
-			error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
-			break;
-		default:
-			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
-					dip, sizeof(*dip), __this_address);
-			return -EFSCORRUPTED;
-		}
-		break;
-
-	default:
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
-				sizeof(*dip), __this_address);
-		return -EFSCORRUPTED;
-	}
-	if (error)
-		return error;
-
-	if (xfs_is_reflink_inode(ip)) {
-		ASSERT(ip->i_cowfp == NULL);
-		xfs_ifork_init_cow(ip);
-	}
-
-	if (!XFS_DFORK_Q(dip))
-		return 0;
-
-	ASSERT(ip->i_afp == NULL);
-	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
-
-	switch (dip->di_aformat) {
-	case XFS_DINODE_FMT_LOCAL:
-		atp = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip);
-		size = be16_to_cpu(atp->hdr.totsize);
-
-		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK, size);
-		break;
-	case XFS_DINODE_FMT_EXTENTS:
-		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
-		break;
-	case XFS_DINODE_FMT_BTREE:
-		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
-		break;
-	default:
-		xfs_inode_verifier_error(ip, error, __func__, dip,
-				sizeof(*dip), __this_address);
-		error = -EFSCORRUPTED;
-		break;
-	}
-	if (error) {
-		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
-		ip->i_afp = NULL;
-		if (ip->i_cowfp)
-			kmem_cache_free(xfs_ifork_zone, ip->i_cowfp);
-		ip->i_cowfp = NULL;
-		xfs_idestroy_fork(ip, XFS_DATA_FORK);
-	}
-	return error;
-}
-
 void
 xfs_init_local_fork(
 	struct xfs_inode	*ip,
@@ -325,6 +221,88 @@ xfs_iformat_btree(
 	return 0;
 }
 
+int
+xfs_iformat_data_fork(
+	struct xfs_inode	*ip,
+	struct xfs_dinode	*dip)
+{
+	struct inode		*inode = VFS_I(ip);
+
+	switch (inode->i_mode & S_IFMT) {
+	case S_IFIFO:
+	case S_IFCHR:
+	case S_IFBLK:
+	case S_IFSOCK:
+		ip->i_d.di_size = 0;
+		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
+		return 0;
+	case S_IFREG:
+	case S_IFLNK:
+	case S_IFDIR:
+		switch (dip->di_format) {
+		case XFS_DINODE_FMT_LOCAL:
+			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
+					be64_to_cpu(dip->di_size));
+		case XFS_DINODE_FMT_EXTENTS:
+			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
+		case XFS_DINODE_FMT_BTREE:
+			return xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
+		default:
+			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
+					dip, sizeof(*dip), __this_address);
+			return -EFSCORRUPTED;
+		}
+		break;
+	default:
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
+				sizeof(*dip), __this_address);
+		return -EFSCORRUPTED;
+	}
+}
+
+static uint16_t
+xfs_dfork_attr_shortform_size(
+	struct xfs_dinode		*dip)
+{
+	struct xfs_attr_shortform	*atp =
+		(struct xfs_attr_shortform *)XFS_DFORK_APTR(dip);
+
+	return be16_to_cpu(atp->hdr.totsize);
+}
+
+int
+xfs_iformat_attr_fork(
+	struct xfs_inode	*ip,
+	struct xfs_dinode	*dip)
+{
+	int			error = 0;
+
+	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
+	switch (dip->di_aformat) {
+	case XFS_DINODE_FMT_LOCAL:
+		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
+				xfs_dfork_attr_shortform_size(dip));
+		break;
+	case XFS_DINODE_FMT_EXTENTS:
+		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
+		break;
+	case XFS_DINODE_FMT_BTREE:
+		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
+		break;
+	default:
+		xfs_inode_verifier_error(ip, error, __func__, dip,
+				sizeof(*dip), __this_address);
+		error = -EFSCORRUPTED;
+		break;
+	}
+
+	if (error) {
+		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
+		ip->i_afp = NULL;
+	}
+	return error;
+}
+
 /*
  * Reallocate the space for if_broot based on the number of records
  * being added or deleted as indicated in rec_diff.  Move the records
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 668ee942be224..8487b0c88a75e 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -88,7 +88,8 @@ struct xfs_ifork {
 
 struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
 
-int		xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
+int		xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
+int		xfs_iformat_attr_fork(struct xfs_inode *, struct xfs_dinode *);
 void		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
 				struct xfs_inode_log_item *, int);
 void		xfs_idestroy_fork(struct xfs_inode *, int);
-- 
2.26.2

Re: [PATCH 03/12] xfs: split xfs_iformat_fork

[Brian Foster]

On Fri, May 08, 2020 at 08:34:14AM +0200, Christoph Hellwig wrote:
> xfs_iformat_fork is a weird catchall.  Split it into one helper for
> the data fork and one for the attr fork, and then call both helper
> as well as the COW fork initialization from xfs_inode_from_disk.  Order
> the COW fork initialization after the attr fork initialization given
> that it can't fail to simplify the error handling.
> 
> Note that the newly split helpers are moved down the file in
> xfs_inode_fork.c to avoid the need for forward declarations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_buf.c  |  20 +++-
>  fs/xfs/libxfs/xfs_inode_fork.c | 186 +++++++++++++++------------------
>  fs/xfs/libxfs/xfs_inode_fork.h |   3 +-
>  3 files changed, 103 insertions(+), 106 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index dc00ce6fc4a2f..abdecc80579e3 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -187,6 +187,10 @@ xfs_inode_from_disk(
>  {
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
> +	int			error;
> +
> +	ASSERT(ip->i_cowfp == NULL);
> +	ASSERT(ip->i_afp == NULL);
>  
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
> @@ -242,7 +246,21 @@ xfs_inode_from_disk(
>  		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
>  	}
>  
> -	return xfs_iformat_fork(ip, from);
> +	error = xfs_iformat_data_fork(ip, from);
> +	if (error)
> +		return error;
> +	if (XFS_DFORK_Q(from)) {
> +		error = xfs_iformat_attr_fork(ip, from);
> +		if (error)
> +			goto out_destroy_data_fork;
> +	}
> +	if (xfs_is_reflink_inode(ip))
> +		xfs_ifork_init_cow(ip);
> +	return 0;
> +
> +out_destroy_data_fork:
> +	xfs_idestroy_fork(ip, XFS_DATA_FORK);
> +	return error;
>  }
>  
>  void
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 3e9a42f1e23b9..5fadfa9a17eb9 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -26,110 +26,6 @@
>  
>  kmem_zone_t *xfs_ifork_zone;
>  
> -STATIC int xfs_iformat_local(xfs_inode_t *, xfs_dinode_t *, int, int);
> -STATIC int xfs_iformat_extents(xfs_inode_t *, xfs_dinode_t *, int);
> -STATIC int xfs_iformat_btree(xfs_inode_t *, xfs_dinode_t *, int);
> -
> -/*
> - * Copy inode type and data and attr format specific information from the
> - * on-disk inode to the in-core inode and fork structures.  For fifos, devices,
> - * and sockets this means set i_rdev to the proper value.  For files,
> - * directories, and symlinks this means to bring in the in-line data or extent
> - * pointers as well as the attribute fork.  For a fork in B-tree format, only
> - * the root is immediately brought in-core.  The rest will be read in later when
> - * first referenced (see xfs_iread_extents()).
> - */
> -int
> -xfs_iformat_fork(
> -	struct xfs_inode	*ip,
> -	struct xfs_dinode	*dip)
> -{
> -	struct inode		*inode = VFS_I(ip);
> -	struct xfs_attr_shortform *atp;
> -	int			size;
> -	int			error = 0;
> -	xfs_fsize_t             di_size;
> -
> -	switch (inode->i_mode & S_IFMT) {
> -	case S_IFIFO:
> -	case S_IFCHR:
> -	case S_IFBLK:
> -	case S_IFSOCK:
> -		ip->i_d.di_size = 0;
> -		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
> -		break;
> -
> -	case S_IFREG:
> -	case S_IFLNK:
> -	case S_IFDIR:
> -		switch (dip->di_format) {
> -		case XFS_DINODE_FMT_LOCAL:
> -			di_size = be64_to_cpu(dip->di_size);
> -			size = (int)di_size;
> -			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK, size);
> -			break;
> -		case XFS_DINODE_FMT_EXTENTS:
> -			error = xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
> -			break;
> -		case XFS_DINODE_FMT_BTREE:
> -			error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
> -			break;
> -		default:
> -			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
> -					dip, sizeof(*dip), __this_address);
> -			return -EFSCORRUPTED;
> -		}
> -		break;
> -
> -	default:
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
> -				sizeof(*dip), __this_address);
> -		return -EFSCORRUPTED;
> -	}
> -	if (error)
> -		return error;
> -
> -	if (xfs_is_reflink_inode(ip)) {
> -		ASSERT(ip->i_cowfp == NULL);
> -		xfs_ifork_init_cow(ip);
> -	}
> -
> -	if (!XFS_DFORK_Q(dip))
> -		return 0;
> -
> -	ASSERT(ip->i_afp == NULL);
> -	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
> -
> -	switch (dip->di_aformat) {
> -	case XFS_DINODE_FMT_LOCAL:
> -		atp = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip);
> -		size = be16_to_cpu(atp->hdr.totsize);
> -
> -		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK, size);
> -		break;
> -	case XFS_DINODE_FMT_EXTENTS:
> -		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> -		break;
> -	case XFS_DINODE_FMT_BTREE:
> -		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
> -		break;
> -	default:
> -		xfs_inode_verifier_error(ip, error, __func__, dip,
> -				sizeof(*dip), __this_address);
> -		error = -EFSCORRUPTED;
> -		break;
> -	}
> -	if (error) {
> -		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> -		ip->i_afp = NULL;
> -		if (ip->i_cowfp)
> -			kmem_cache_free(xfs_ifork_zone, ip->i_cowfp);
> -		ip->i_cowfp = NULL;
> -		xfs_idestroy_fork(ip, XFS_DATA_FORK);
> -	}
> -	return error;
> -}
> -
>  void
>  xfs_init_local_fork(
>  	struct xfs_inode	*ip,
> @@ -325,6 +221,88 @@ xfs_iformat_btree(
>  	return 0;
>  }
>  
> +int
> +xfs_iformat_data_fork(
> +	struct xfs_inode	*ip,
> +	struct xfs_dinode	*dip)
> +{
> +	struct inode		*inode = VFS_I(ip);
> +
> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFIFO:
> +	case S_IFCHR:
> +	case S_IFBLK:
> +	case S_IFSOCK:
> +		ip->i_d.di_size = 0;
> +		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
> +		return 0;
> +	case S_IFREG:
> +	case S_IFLNK:
> +	case S_IFDIR:
> +		switch (dip->di_format) {
> +		case XFS_DINODE_FMT_LOCAL:
> +			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
> +					be64_to_cpu(dip->di_size));
> +		case XFS_DINODE_FMT_EXTENTS:
> +			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
> +		case XFS_DINODE_FMT_BTREE:
> +			return xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
> +		default:
> +			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
> +					dip, sizeof(*dip), __this_address);
> +			return -EFSCORRUPTED;
> +		}
> +		break;
> +	default:
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
> +				sizeof(*dip), __this_address);
> +		return -EFSCORRUPTED;
> +	}
> +}
> +
> +static uint16_t
> +xfs_dfork_attr_shortform_size(
> +	struct xfs_dinode		*dip)
> +{
> +	struct xfs_attr_shortform	*atp =
> +		(struct xfs_attr_shortform *)XFS_DFORK_APTR(dip);
> +
> +	return be16_to_cpu(atp->hdr.totsize);
> +}
> +
> +int
> +xfs_iformat_attr_fork(
> +	struct xfs_inode	*ip,
> +	struct xfs_dinode	*dip)
> +{
> +	int			error = 0;
> +
> +	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
> +	switch (dip->di_aformat) {
> +	case XFS_DINODE_FMT_LOCAL:
> +		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
> +				xfs_dfork_attr_shortform_size(dip));
> +		break;
> +	case XFS_DINODE_FMT_EXTENTS:
> +		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> +		break;
> +	case XFS_DINODE_FMT_BTREE:
> +		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
> +		break;
> +	default:
> +		xfs_inode_verifier_error(ip, error, __func__, dip,
> +				sizeof(*dip), __this_address);
> +		error = -EFSCORRUPTED;
> +		break;
> +	}
> +
> +	if (error) {
> +		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> +		ip->i_afp = NULL;
> +	}
> +	return error;
> +}
> +
>  /*
>   * Reallocate the space for if_broot based on the number of records
>   * being added or deleted as indicated in rec_diff.  Move the records
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 668ee942be224..8487b0c88a75e 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -88,7 +88,8 @@ struct xfs_ifork {
>  
>  struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
>  
> -int		xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> +int		xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
> +int		xfs_iformat_attr_fork(struct xfs_inode *, struct xfs_dinode *);
>  void		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
>  				struct xfs_inode_log_item *, int);
>  void		xfs_idestroy_fork(struct xfs_inode *, int);
> -- 
> 2.26.2
> 

Re: [PATCH 03/12] xfs: split xfs_iformat_fork

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:14AM +0200, Christoph Hellwig wrote:
> xfs_iformat_fork is a weird catchall.  Split it into one helper for
> the data fork and one for the attr fork, and then call both helper
> as well as the COW fork initialization from xfs_inode_from_disk.  Order
> the COW fork initialization after the attr fork initialization given
> that it can't fail to simplify the error handling.
> 
> Note that the newly split helpers are moved down the file in
> xfs_inode_fork.c to avoid the need for forward declarations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems reasonable,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c  |  20 +++-
>  fs/xfs/libxfs/xfs_inode_fork.c | 186 +++++++++++++++------------------
>  fs/xfs/libxfs/xfs_inode_fork.h |   3 +-
>  3 files changed, 103 insertions(+), 106 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index dc00ce6fc4a2f..abdecc80579e3 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -187,6 +187,10 @@ xfs_inode_from_disk(
>  {
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
> +	int			error;
> +
> +	ASSERT(ip->i_cowfp == NULL);
> +	ASSERT(ip->i_afp == NULL);
>  
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
> @@ -242,7 +246,21 @@ xfs_inode_from_disk(
>  		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
>  	}
>  
> -	return xfs_iformat_fork(ip, from);
> +	error = xfs_iformat_data_fork(ip, from);
> +	if (error)
> +		return error;
> +	if (XFS_DFORK_Q(from)) {
> +		error = xfs_iformat_attr_fork(ip, from);
> +		if (error)
> +			goto out_destroy_data_fork;
> +	}
> +	if (xfs_is_reflink_inode(ip))
> +		xfs_ifork_init_cow(ip);
> +	return 0;
> +
> +out_destroy_data_fork:
> +	xfs_idestroy_fork(ip, XFS_DATA_FORK);
> +	return error;
>  }
>  
>  void
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 3e9a42f1e23b9..5fadfa9a17eb9 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -26,110 +26,6 @@
>  
>  kmem_zone_t *xfs_ifork_zone;
>  
> -STATIC int xfs_iformat_local(xfs_inode_t *, xfs_dinode_t *, int, int);
> -STATIC int xfs_iformat_extents(xfs_inode_t *, xfs_dinode_t *, int);
> -STATIC int xfs_iformat_btree(xfs_inode_t *, xfs_dinode_t *, int);
> -
> -/*
> - * Copy inode type and data and attr format specific information from the
> - * on-disk inode to the in-core inode and fork structures.  For fifos, devices,
> - * and sockets this means set i_rdev to the proper value.  For files,
> - * directories, and symlinks this means to bring in the in-line data or extent
> - * pointers as well as the attribute fork.  For a fork in B-tree format, only
> - * the root is immediately brought in-core.  The rest will be read in later when
> - * first referenced (see xfs_iread_extents()).
> - */
> -int
> -xfs_iformat_fork(
> -	struct xfs_inode	*ip,
> -	struct xfs_dinode	*dip)
> -{
> -	struct inode		*inode = VFS_I(ip);
> -	struct xfs_attr_shortform *atp;
> -	int			size;
> -	int			error = 0;
> -	xfs_fsize_t             di_size;
> -
> -	switch (inode->i_mode & S_IFMT) {
> -	case S_IFIFO:
> -	case S_IFCHR:
> -	case S_IFBLK:
> -	case S_IFSOCK:
> -		ip->i_d.di_size = 0;
> -		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
> -		break;
> -
> -	case S_IFREG:
> -	case S_IFLNK:
> -	case S_IFDIR:
> -		switch (dip->di_format) {
> -		case XFS_DINODE_FMT_LOCAL:
> -			di_size = be64_to_cpu(dip->di_size);
> -			size = (int)di_size;
> -			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK, size);
> -			break;
> -		case XFS_DINODE_FMT_EXTENTS:
> -			error = xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
> -			break;
> -		case XFS_DINODE_FMT_BTREE:
> -			error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
> -			break;
> -		default:
> -			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
> -					dip, sizeof(*dip), __this_address);
> -			return -EFSCORRUPTED;
> -		}
> -		break;
> -
> -	default:
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
> -				sizeof(*dip), __this_address);
> -		return -EFSCORRUPTED;
> -	}
> -	if (error)
> -		return error;
> -
> -	if (xfs_is_reflink_inode(ip)) {
> -		ASSERT(ip->i_cowfp == NULL);
> -		xfs_ifork_init_cow(ip);
> -	}
> -
> -	if (!XFS_DFORK_Q(dip))
> -		return 0;
> -
> -	ASSERT(ip->i_afp == NULL);
> -	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
> -
> -	switch (dip->di_aformat) {
> -	case XFS_DINODE_FMT_LOCAL:
> -		atp = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip);
> -		size = be16_to_cpu(atp->hdr.totsize);
> -
> -		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK, size);
> -		break;
> -	case XFS_DINODE_FMT_EXTENTS:
> -		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> -		break;
> -	case XFS_DINODE_FMT_BTREE:
> -		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
> -		break;
> -	default:
> -		xfs_inode_verifier_error(ip, error, __func__, dip,
> -				sizeof(*dip), __this_address);
> -		error = -EFSCORRUPTED;
> -		break;
> -	}
> -	if (error) {
> -		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> -		ip->i_afp = NULL;
> -		if (ip->i_cowfp)
> -			kmem_cache_free(xfs_ifork_zone, ip->i_cowfp);
> -		ip->i_cowfp = NULL;
> -		xfs_idestroy_fork(ip, XFS_DATA_FORK);
> -	}
> -	return error;
> -}
> -
>  void
>  xfs_init_local_fork(
>  	struct xfs_inode	*ip,
> @@ -325,6 +221,88 @@ xfs_iformat_btree(
>  	return 0;
>  }
>  
> +int
> +xfs_iformat_data_fork(
> +	struct xfs_inode	*ip,
> +	struct xfs_dinode	*dip)
> +{
> +	struct inode		*inode = VFS_I(ip);
> +
> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFIFO:
> +	case S_IFCHR:
> +	case S_IFBLK:
> +	case S_IFSOCK:
> +		ip->i_d.di_size = 0;
> +		inode->i_rdev = xfs_to_linux_dev_t(xfs_dinode_get_rdev(dip));
> +		return 0;
> +	case S_IFREG:
> +	case S_IFLNK:
> +	case S_IFDIR:
> +		switch (dip->di_format) {
> +		case XFS_DINODE_FMT_LOCAL:
> +			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
> +					be64_to_cpu(dip->di_size));
> +		case XFS_DINODE_FMT_EXTENTS:
> +			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
> +		case XFS_DINODE_FMT_BTREE:
> +			return xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
> +		default:
> +			xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__,
> +					dip, sizeof(*dip), __this_address);
> +			return -EFSCORRUPTED;
> +		}
> +		break;
> +	default:
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
> +				sizeof(*dip), __this_address);
> +		return -EFSCORRUPTED;
> +	}
> +}
> +
> +static uint16_t
> +xfs_dfork_attr_shortform_size(
> +	struct xfs_dinode		*dip)
> +{
> +	struct xfs_attr_shortform	*atp =
> +		(struct xfs_attr_shortform *)XFS_DFORK_APTR(dip);
> +
> +	return be16_to_cpu(atp->hdr.totsize);
> +}
> +
> +int
> +xfs_iformat_attr_fork(
> +	struct xfs_inode	*ip,
> +	struct xfs_dinode	*dip)
> +{
> +	int			error = 0;
> +
> +	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS);
> +	switch (dip->di_aformat) {
> +	case XFS_DINODE_FMT_LOCAL:
> +		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
> +				xfs_dfork_attr_shortform_size(dip));
> +		break;
> +	case XFS_DINODE_FMT_EXTENTS:
> +		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> +		break;
> +	case XFS_DINODE_FMT_BTREE:
> +		error = xfs_iformat_btree(ip, dip, XFS_ATTR_FORK);
> +		break;
> +	default:
> +		xfs_inode_verifier_error(ip, error, __func__, dip,
> +				sizeof(*dip), __this_address);
> +		error = -EFSCORRUPTED;
> +		break;
> +	}
> +
> +	if (error) {
> +		kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> +		ip->i_afp = NULL;
> +	}
> +	return error;
> +}
> +
>  /*
>   * Reallocate the space for if_broot based on the number of records
>   * being added or deleted as indicated in rec_diff.  Move the records
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 668ee942be224..8487b0c88a75e 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -88,7 +88,8 @@ struct xfs_ifork {
>  
>  struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
>  
> -int		xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> +int		xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
> +int		xfs_iformat_attr_fork(struct xfs_inode *, struct xfs_dinode *);
>  void		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
>  				struct xfs_inode_log_item *, int);
>  void		xfs_idestroy_fork(struct xfs_inode *, int);
> -- 
> 2.26.2
> 

[PATCH 04/12] xfs: handle unallocated inodes in xfs_inode_from_disk

[Christoph Hellwig]

Handle inodes with a 0 di_mode in xfs_inode_from_disk, instead of partially
duplicating inode reading in xfs_iread.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 50 ++++++++++-------------------------
 1 file changed, 14 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index abdecc80579e3..686a026b5f6ed 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -192,6 +192,17 @@ xfs_inode_from_disk(
 	ASSERT(ip->i_cowfp == NULL);
 	ASSERT(ip->i_afp == NULL);
 
+	/*
+	 * First get the permanent information that is needed to allocate an
+	 * inode. If the inode is unused, mode is zero and we shouldn't mess
+	 * with the unitialized part of it.
+	 */
+	to->di_flushiter = be16_to_cpu(from->di_flushiter);
+	inode->i_generation = be32_to_cpu(from->di_gen);
+	inode->i_mode = be16_to_cpu(from->di_mode);
+	if (!inode->i_mode)
+		return 0;
+
 	/*
 	 * Convert v1 inodes immediately to v2 inode format as this is the
 	 * minimum inode version format we support in the rest of the code.
@@ -209,7 +220,6 @@ xfs_inode_from_disk(
 	to->di_format = from->di_format;
 	i_uid_write(inode, be32_to_cpu(from->di_uid));
 	i_gid_write(inode, be32_to_cpu(from->di_gid));
-	to->di_flushiter = be16_to_cpu(from->di_flushiter);
 
 	/*
 	 * Time is signed, so need to convert to signed 32 bit before
@@ -223,8 +233,6 @@ xfs_inode_from_disk(
 	inode->i_mtime.tv_nsec = (int)be32_to_cpu(from->di_mtime.t_nsec);
 	inode->i_ctime.tv_sec = (int)be32_to_cpu(from->di_ctime.t_sec);
 	inode->i_ctime.tv_nsec = (int)be32_to_cpu(from->di_ctime.t_nsec);
-	inode->i_generation = be32_to_cpu(from->di_gen);
-	inode->i_mode = be16_to_cpu(from->di_mode);
 
 	to->di_size = be64_to_cpu(from->di_size);
 	to->di_nblocks = be64_to_cpu(from->di_nblocks);
@@ -653,39 +661,9 @@ xfs_iread(
 		goto out_brelse;
 	}
 
-	/*
-	 * If the on-disk inode is already linked to a directory
-	 * entry, copy all of the inode into the in-core inode.
-	 * xfs_iformat_fork() handles copying in the inode format
-	 * specific information.
-	 * Otherwise, just get the truly permanent information.
-	 */
-	if (dip->di_mode) {
-		error = xfs_inode_from_disk(ip, dip);
-		if (error)  {
-#ifdef DEBUG
-			xfs_alert(mp, "%s: xfs_iformat() returned error %d",
-				__func__, error);
-#endif /* DEBUG */
-			goto out_brelse;
-		}
-	} else {
-		/*
-		 * Partial initialisation of the in-core inode. Just the bits
-		 * that xfs_ialloc won't overwrite or relies on being correct.
-		 */
-		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
-		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
-
-		/*
-		 * Make sure to pull in the mode here as well in
-		 * case the inode is released without being used.
-		 * This ensures that xfs_inactive() will see that
-		 * the inode is already free and not try to mess
-		 * with the uninitialized part of it.
-		 */
-		VFS_I(ip)->i_mode = 0;
-	}
+	error = xfs_inode_from_disk(ip, dip);
+	if (error)
+		goto out_brelse;
 
 	ip->i_delayed_blks = 0;
 
-- 
2.26.2

Re: [PATCH 04/12] xfs: handle unallocated inodes in xfs_inode_from_disk

[Brian Foster]

On Fri, May 08, 2020 at 08:34:15AM +0200, Christoph Hellwig wrote:
> Handle inodes with a 0 di_mode in xfs_inode_from_disk, instead of partially
> duplicating inode reading in xfs_iread.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_buf.c | 50 ++++++++++-------------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index abdecc80579e3..686a026b5f6ed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -192,6 +192,17 @@ xfs_inode_from_disk(
>  	ASSERT(ip->i_cowfp == NULL);
>  	ASSERT(ip->i_afp == NULL);
>  
> +	/*
> +	 * First get the permanent information that is needed to allocate an
> +	 * inode. If the inode is unused, mode is zero and we shouldn't mess
> +	 * with the unitialized part of it.
> +	 */
> +	to->di_flushiter = be16_to_cpu(from->di_flushiter);
> +	inode->i_generation = be32_to_cpu(from->di_gen);
> +	inode->i_mode = be16_to_cpu(from->di_mode);
> +	if (!inode->i_mode)
> +		return 0;
> +
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
>  	 * minimum inode version format we support in the rest of the code.
> @@ -209,7 +220,6 @@ xfs_inode_from_disk(
>  	to->di_format = from->di_format;
>  	i_uid_write(inode, be32_to_cpu(from->di_uid));
>  	i_gid_write(inode, be32_to_cpu(from->di_gid));
> -	to->di_flushiter = be16_to_cpu(from->di_flushiter);
>  
>  	/*
>  	 * Time is signed, so need to convert to signed 32 bit before
> @@ -223,8 +233,6 @@ xfs_inode_from_disk(
>  	inode->i_mtime.tv_nsec = (int)be32_to_cpu(from->di_mtime.t_nsec);
>  	inode->i_ctime.tv_sec = (int)be32_to_cpu(from->di_ctime.t_sec);
>  	inode->i_ctime.tv_nsec = (int)be32_to_cpu(from->di_ctime.t_nsec);
> -	inode->i_generation = be32_to_cpu(from->di_gen);
> -	inode->i_mode = be16_to_cpu(from->di_mode);
>  
>  	to->di_size = be64_to_cpu(from->di_size);
>  	to->di_nblocks = be64_to_cpu(from->di_nblocks);
> @@ -653,39 +661,9 @@ xfs_iread(
>  		goto out_brelse;
>  	}
>  
> -	/*
> -	 * If the on-disk inode is already linked to a directory
> -	 * entry, copy all of the inode into the in-core inode.
> -	 * xfs_iformat_fork() handles copying in the inode format
> -	 * specific information.
> -	 * Otherwise, just get the truly permanent information.
> -	 */
> -	if (dip->di_mode) {
> -		error = xfs_inode_from_disk(ip, dip);
> -		if (error)  {
> -#ifdef DEBUG
> -			xfs_alert(mp, "%s: xfs_iformat() returned error %d",
> -				__func__, error);
> -#endif /* DEBUG */
> -			goto out_brelse;
> -		}
> -	} else {
> -		/*
> -		 * Partial initialisation of the in-core inode. Just the bits
> -		 * that xfs_ialloc won't overwrite or relies on being correct.
> -		 */
> -		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
> -		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
> -
> -		/*
> -		 * Make sure to pull in the mode here as well in
> -		 * case the inode is released without being used.
> -		 * This ensures that xfs_inactive() will see that
> -		 * the inode is already free and not try to mess
> -		 * with the uninitialized part of it.
> -		 */
> -		VFS_I(ip)->i_mode = 0;
> -	}
> +	error = xfs_inode_from_disk(ip, dip);
> +	if (error)
> +		goto out_brelse;
>  
>  	ip->i_delayed_blks = 0;
>  
> -- 
> 2.26.2
> 

Re: [PATCH 04/12] xfs: handle unallocated inodes in xfs_inode_from_disk

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:15AM +0200, Christoph Hellwig wrote:
> Handle inodes with a 0 di_mode in xfs_inode_from_disk, instead of partially
> duplicating inode reading in xfs_iread.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 50 ++++++++++-------------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index abdecc80579e3..686a026b5f6ed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -192,6 +192,17 @@ xfs_inode_from_disk(
>  	ASSERT(ip->i_cowfp == NULL);
>  	ASSERT(ip->i_afp == NULL);
>  
> +	/*
> +	 * First get the permanent information that is needed to allocate an
> +	 * inode. If the inode is unused, mode is zero and we shouldn't mess
> +	 * with the unitialized part of it.
> +	 */
> +	to->di_flushiter = be16_to_cpu(from->di_flushiter);
> +	inode->i_generation = be32_to_cpu(from->di_gen);
> +	inode->i_mode = be16_to_cpu(from->di_mode);
> +	if (!inode->i_mode)
> +		return 0;
> +
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
>  	 * minimum inode version format we support in the rest of the code.
> @@ -209,7 +220,6 @@ xfs_inode_from_disk(
>  	to->di_format = from->di_format;
>  	i_uid_write(inode, be32_to_cpu(from->di_uid));
>  	i_gid_write(inode, be32_to_cpu(from->di_gid));
> -	to->di_flushiter = be16_to_cpu(from->di_flushiter);
>  
>  	/*
>  	 * Time is signed, so need to convert to signed 32 bit before
> @@ -223,8 +233,6 @@ xfs_inode_from_disk(
>  	inode->i_mtime.tv_nsec = (int)be32_to_cpu(from->di_mtime.t_nsec);
>  	inode->i_ctime.tv_sec = (int)be32_to_cpu(from->di_ctime.t_sec);
>  	inode->i_ctime.tv_nsec = (int)be32_to_cpu(from->di_ctime.t_nsec);
> -	inode->i_generation = be32_to_cpu(from->di_gen);
> -	inode->i_mode = be16_to_cpu(from->di_mode);
>  
>  	to->di_size = be64_to_cpu(from->di_size);
>  	to->di_nblocks = be64_to_cpu(from->di_nblocks);
> @@ -653,39 +661,9 @@ xfs_iread(
>  		goto out_brelse;
>  	}
>  
> -	/*
> -	 * If the on-disk inode is already linked to a directory
> -	 * entry, copy all of the inode into the in-core inode.
> -	 * xfs_iformat_fork() handles copying in the inode format
> -	 * specific information.
> -	 * Otherwise, just get the truly permanent information.
> -	 */
> -	if (dip->di_mode) {
> -		error = xfs_inode_from_disk(ip, dip);
> -		if (error)  {
> -#ifdef DEBUG
> -			xfs_alert(mp, "%s: xfs_iformat() returned error %d",
> -				__func__, error);
> -#endif /* DEBUG */
> -			goto out_brelse;
> -		}
> -	} else {
> -		/*
> -		 * Partial initialisation of the in-core inode. Just the bits
> -		 * that xfs_ialloc won't overwrite or relies on being correct.
> -		 */
> -		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
> -		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
> -
> -		/*
> -		 * Make sure to pull in the mode here as well in
> -		 * case the inode is released without being used.
> -		 * This ensures that xfs_inactive() will see that
> -		 * the inode is already free and not try to mess
> -		 * with the uninitialized part of it.
> -		 */
> -		VFS_I(ip)->i_mode = 0;
> -	}
> +	error = xfs_inode_from_disk(ip, dip);
> +	if (error)
> +		goto out_brelse;
>  
>  	ip->i_delayed_blks = 0;
>  
> -- 
> 2.26.2
> 

[PATCH 05/12] xfs: call xfs_dinode_verify from xfs_inode_from_disk

[Christoph Hellwig]

Keep the code dealing with the dinode together, and also ensure we verify
the dinode in the owner change log recovery case as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 .../xfs-self-describing-metadata.txt           | 10 +++++-----
 fs/xfs/libxfs/xfs_inode_buf.c                  | 18 ++++++++----------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/filesystems/xfs-self-describing-metadata.txt b/Documentation/filesystems/xfs-self-describing-metadata.txt
index 8db0121d0980c..e912699d74301 100644
--- a/Documentation/filesystems/xfs-self-describing-metadata.txt
+++ b/Documentation/filesystems/xfs-self-describing-metadata.txt
@@ -337,11 +337,11 @@ buffer.
 
 The structure of the verifiers and the identifiers checks is very similar to the
 buffer code described above. The only difference is where they are called. For
-example, inode read verification is done in xfs_iread() when the inode is first
-read out of the buffer and the struct xfs_inode is instantiated. The inode is
-already extensively verified during writeback in xfs_iflush_int, so the only
-addition here is to add the LSN and CRC to the inode as it is copied back into
-the buffer.
+example, inode read verification is done in xfs_inode_from_disk() when the inode
+is first read out of the buffer and the struct xfs_inode is instantiated. The
+inode is already extensively verified during writeback in xfs_iflush_int, so the
+only addition here is to add the LSN and CRC to the inode as it is copied back
+into the buffer.
 
 XXX: inode unlinked list modification doesn't recalculate the inode CRC! None of
 the unlinked list modifications check or update CRCs, neither during unlink nor
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 686a026b5f6ed..3aac22e892985 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -188,10 +188,18 @@ xfs_inode_from_disk(
 	struct xfs_icdinode	*to = &ip->i_d;
 	struct inode		*inode = VFS_I(ip);
 	int			error;
+	xfs_failaddr_t		fa;
 
 	ASSERT(ip->i_cowfp == NULL);
 	ASSERT(ip->i_afp == NULL);
 
+	fa = xfs_dinode_verify(ip->i_mount, ip->i_ino, from);
+	if (fa) {
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", from,
+				sizeof(*from), fa);
+		return -EFSCORRUPTED;
+	}
+
 	/*
 	 * First get the permanent information that is needed to allocate an
 	 * inode. If the inode is unused, mode is zero and we shouldn't mess
@@ -627,7 +635,6 @@ xfs_iread(
 {
 	xfs_buf_t	*bp;
 	xfs_dinode_t	*dip;
-	xfs_failaddr_t	fa;
 	int		error;
 
 	/*
@@ -652,15 +659,6 @@ xfs_iread(
 	if (error)
 		return error;
 
-	/* even unallocated inodes are verified */
-	fa = xfs_dinode_verify(mp, ip->i_ino, dip);
-	if (fa) {
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", dip,
-				sizeof(*dip), fa);
-		error = -EFSCORRUPTED;
-		goto out_brelse;
-	}
-
 	error = xfs_inode_from_disk(ip, dip);
 	if (error)
 		goto out_brelse;
-- 
2.26.2

Re: [PATCH 05/12] xfs: call xfs_dinode_verify from xfs_inode_from_disk

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:16AM +0200, Christoph Hellwig wrote:
> Keep the code dealing with the dinode together, and also ensure we verify
> the dinode in the owner change log recovery case as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Seems reasonable to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  .../xfs-self-describing-metadata.txt           | 10 +++++-----
>  fs/xfs/libxfs/xfs_inode_buf.c                  | 18 ++++++++----------
>  2 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/filesystems/xfs-self-describing-metadata.txt b/Documentation/filesystems/xfs-self-describing-metadata.txt
> index 8db0121d0980c..e912699d74301 100644
> --- a/Documentation/filesystems/xfs-self-describing-metadata.txt
> +++ b/Documentation/filesystems/xfs-self-describing-metadata.txt
> @@ -337,11 +337,11 @@ buffer.
>  
>  The structure of the verifiers and the identifiers checks is very similar to the
>  buffer code described above. The only difference is where they are called. For
> -example, inode read verification is done in xfs_iread() when the inode is first
> -read out of the buffer and the struct xfs_inode is instantiated. The inode is
> -already extensively verified during writeback in xfs_iflush_int, so the only
> -addition here is to add the LSN and CRC to the inode as it is copied back into
> -the buffer.
> +example, inode read verification is done in xfs_inode_from_disk() when the inode
> +is first read out of the buffer and the struct xfs_inode is instantiated. The
> +inode is already extensively verified during writeback in xfs_iflush_int, so the
> +only addition here is to add the LSN and CRC to the inode as it is copied back
> +into the buffer.
>  
>  XXX: inode unlinked list modification doesn't recalculate the inode CRC! None of
>  the unlinked list modifications check or update CRCs, neither during unlink nor
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 686a026b5f6ed..3aac22e892985 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -188,10 +188,18 @@ xfs_inode_from_disk(
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
>  	int			error;
> +	xfs_failaddr_t		fa;
>  
>  	ASSERT(ip->i_cowfp == NULL);
>  	ASSERT(ip->i_afp == NULL);
>  
> +	fa = xfs_dinode_verify(ip->i_mount, ip->i_ino, from);
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", from,
> +				sizeof(*from), fa);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	/*
>  	 * First get the permanent information that is needed to allocate an
>  	 * inode. If the inode is unused, mode is zero and we shouldn't mess
> @@ -627,7 +635,6 @@ xfs_iread(
>  {
>  	xfs_buf_t	*bp;
>  	xfs_dinode_t	*dip;
> -	xfs_failaddr_t	fa;
>  	int		error;
>  
>  	/*
> @@ -652,15 +659,6 @@ xfs_iread(
>  	if (error)
>  		return error;
>  
> -	/* even unallocated inodes are verified */
> -	fa = xfs_dinode_verify(mp, ip->i_ino, dip);
> -	if (fa) {
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", dip,
> -				sizeof(*dip), fa);
> -		error = -EFSCORRUPTED;
> -		goto out_brelse;
> -	}
> -
>  	error = xfs_inode_from_disk(ip, dip);
>  	if (error)
>  		goto out_brelse;
> -- 
> 2.26.2
> 

[PATCH 06/12] xfs: don't reset i_delayed_blks in xfs_iread

[Christoph Hellwig]

i_delayed_blks is set to 0 in xfs_inode_alloc and can't have anything
assigned to it until the inode is visible to the VFS.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 3aac22e892985..329534eebbdcc 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -663,8 +663,6 @@ xfs_iread(
 	if (error)
 		goto out_brelse;
 
-	ip->i_delayed_blks = 0;
-
 	/*
 	 * Mark the buffer containing the inode as something to keep
 	 * around for a while.  This helps to keep recently accessed
-- 
2.26.2

Re: [PATCH 06/12] xfs: don't reset i_delayed_blks in xfs_iread

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:17AM +0200, Christoph Hellwig wrote:
> i_delayed_blks is set to 0 in xfs_inode_alloc and can't have anything
> assigned to it until the inode is visible to the VFS.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Seems fine to me...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 3aac22e892985..329534eebbdcc 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -663,8 +663,6 @@ xfs_iread(
>  	if (error)
>  		goto out_brelse;
>  
> -	ip->i_delayed_blks = 0;
> -
>  	/*
>  	 * Mark the buffer containing the inode as something to keep
>  	 * around for a while.  This helps to keep recently accessed
> -- 
> 2.26.2
> 

[PATCH 07/12] xfs: remove xfs_iread

[Christoph Hellwig]

There is not much point in the xfs_iread function, as it has a single
caller and not a whole lot of code.  Move it into the only caller,
and trim down the overdocumentation to just documenting the important
"why" instead of a lot of redundant "what".

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 73 -----------------------------------
 fs/xfs/libxfs/xfs_inode_buf.h |  2 -
 fs/xfs/xfs_icache.c           | 33 +++++++++++++++-
 3 files changed, 32 insertions(+), 76 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 329534eebbdcc..05f939adea944 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -614,79 +614,6 @@ xfs_dinode_calc_crc(
 	dip->di_crc = xfs_end_cksum(crc);
 }
 
-/*
- * Read the disk inode attributes into the in-core inode structure.
- *
- * For version 5 superblocks, if we are initialising a new inode and we are not
- * utilising the XFS_MOUNT_IKEEP inode cluster mode, we can simple build the new
- * inode core with a random generation number. If we are keeping inodes around,
- * we need to read the inode cluster to get the existing generation number off
- * disk. Further, if we are using version 4 superblocks (i.e. v1/v2 inode
- * format) then log recovery is dependent on the di_flushiter field being
- * initialised from the current on-disk value and hence we must also read the
- * inode off disk.
- */
-int
-xfs_iread(
-	xfs_mount_t	*mp,
-	xfs_trans_t	*tp,
-	xfs_inode_t	*ip,
-	uint		iget_flags)
-{
-	xfs_buf_t	*bp;
-	xfs_dinode_t	*dip;
-	int		error;
-
-	/*
-	 * Fill in the location information in the in-core inode.
-	 */
-	error = xfs_imap(mp, tp, ip->i_ino, &ip->i_imap, iget_flags);
-	if (error)
-		return error;
-
-	/* shortcut IO on inode allocation if possible */
-	if ((iget_flags & XFS_IGET_CREATE) &&
-	    xfs_sb_version_has_v3inode(&mp->m_sb) &&
-	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
-		VFS_I(ip)->i_generation = prandom_u32();
-		return 0;
-	}
-
-	/*
-	 * Get pointers to the on-disk inode and the buffer containing it.
-	 */
-	error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
-	if (error)
-		return error;
-
-	error = xfs_inode_from_disk(ip, dip);
-	if (error)
-		goto out_brelse;
-
-	/*
-	 * Mark the buffer containing the inode as something to keep
-	 * around for a while.  This helps to keep recently accessed
-	 * meta-data in-core longer.
-	 */
-	xfs_buf_set_ref(bp, XFS_INO_REF);
-
-	/*
-	 * Use xfs_trans_brelse() to release the buffer containing the on-disk
-	 * inode, because it was acquired with xfs_trans_read_buf() in
-	 * xfs_imap_to_bp() above.  If tp is NULL, this is just a normal
-	 * brelse().  If we're within a transaction, then xfs_trans_brelse()
-	 * will only release the buffer if it is not dirty within the
-	 * transaction.  It will be OK to release the buffer in this case,
-	 * because inodes on disk are never destroyed and we will be locking the
-	 * new in-core inode before putting it in the cache where other
-	 * processes can find it.  Thus we don't have to worry about the inode
-	 * being changed just because we released the buffer.
-	 */
- out_brelse:
-	xfs_trans_brelse(tp, bp);
-	return error;
-}
-
 /*
  * Validate di_extsize hint.
  *
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 0fbb99224ec73..e4cbcaf62a32b 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -49,8 +49,6 @@ struct xfs_imap {
 int	xfs_imap_to_bp(struct xfs_mount *, struct xfs_trans *,
 		       struct xfs_imap *, struct xfs_dinode **,
 		       struct xfs_buf **, uint);
-int	xfs_iread(struct xfs_mount *, struct xfs_trans *,
-		  struct xfs_inode *, uint);
 void	xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
 void	xfs_inode_to_disk(struct xfs_inode *ip, struct xfs_dinode *to,
 			  xfs_lsn_t lsn);
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 922a29032e374..af5748f5d9271 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -22,6 +22,7 @@
 #include "xfs_dquot_item.h"
 #include "xfs_dquot.h"
 #include "xfs_reflink.h"
+#include "xfs_ialloc.h"
 
 #include <linux/iversion.h>
 
@@ -508,10 +509,40 @@ xfs_iget_cache_miss(
 	if (!ip)
 		return -ENOMEM;
 
-	error = xfs_iread(mp, tp, ip, flags);
+	error = xfs_imap(mp, tp, ip->i_ino, &ip->i_imap, flags);
 	if (error)
 		goto out_destroy;
 
+	/*
+	 * For version 5 superblocks, if we are initialising a new inode and we
+	 * are not utilising the XFS_MOUNT_IKEEP inode cluster mode, we can
+	 * simply build the new inode core with a random generation number.
+	 *
+	 * For version 4 (and older) superblocks, log recovery is dependent on
+	 * the di_flushiter field being initialised from the current on-disk
+	 * value and hence we must also read the inode off disk even when
+	 * initializing new inodes.
+	 */
+	if (xfs_sb_version_has_v3inode(&mp->m_sb) &&
+	    (flags & XFS_IGET_CREATE) && !(mp->m_flags & XFS_MOUNT_IKEEP)) {
+		VFS_I(ip)->i_generation = prandom_u32();
+	} else {
+		struct xfs_dinode	*dip;
+		struct xfs_buf		*bp;
+
+		error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
+		if (error)
+			goto out_destroy;
+
+		error = xfs_inode_from_disk(ip, dip);
+		if (!error)
+			xfs_buf_set_ref(bp, XFS_INO_REF);
+		xfs_trans_brelse(tp, bp);
+
+		if (error)
+			goto out_destroy;
+	}
+
 	if (!xfs_inode_verify_forks(ip)) {
 		error = -EFSCORRUPTED;
 		goto out_destroy;
-- 
2.26.2

Re: [PATCH 07/12] xfs: remove xfs_iread

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:18AM +0200, Christoph Hellwig wrote:
> There is not much point in the xfs_iread function, as it has a single
> caller and not a whole lot of code.  Move it into the only caller,
> and trim down the overdocumentation to just documenting the important
> "why" instead of a lot of redundant "what".
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks reasonable,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 73 -----------------------------------
>  fs/xfs/libxfs/xfs_inode_buf.h |  2 -
>  fs/xfs/xfs_icache.c           | 33 +++++++++++++++-
>  3 files changed, 32 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 329534eebbdcc..05f939adea944 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -614,79 +614,6 @@ xfs_dinode_calc_crc(
>  	dip->di_crc = xfs_end_cksum(crc);
>  }
>  
> -/*
> - * Read the disk inode attributes into the in-core inode structure.
> - *
> - * For version 5 superblocks, if we are initialising a new inode and we are not
> - * utilising the XFS_MOUNT_IKEEP inode cluster mode, we can simple build the new
> - * inode core with a random generation number. If we are keeping inodes around,
> - * we need to read the inode cluster to get the existing generation number off
> - * disk. Further, if we are using version 4 superblocks (i.e. v1/v2 inode
> - * format) then log recovery is dependent on the di_flushiter field being
> - * initialised from the current on-disk value and hence we must also read the
> - * inode off disk.
> - */
> -int
> -xfs_iread(
> -	xfs_mount_t	*mp,
> -	xfs_trans_t	*tp,
> -	xfs_inode_t	*ip,
> -	uint		iget_flags)
> -{
> -	xfs_buf_t	*bp;
> -	xfs_dinode_t	*dip;
> -	int		error;
> -
> -	/*
> -	 * Fill in the location information in the in-core inode.
> -	 */
> -	error = xfs_imap(mp, tp, ip->i_ino, &ip->i_imap, iget_flags);
> -	if (error)
> -		return error;
> -
> -	/* shortcut IO on inode allocation if possible */
> -	if ((iget_flags & XFS_IGET_CREATE) &&
> -	    xfs_sb_version_has_v3inode(&mp->m_sb) &&
> -	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> -		VFS_I(ip)->i_generation = prandom_u32();
> -		return 0;
> -	}
> -
> -	/*
> -	 * Get pointers to the on-disk inode and the buffer containing it.
> -	 */
> -	error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
> -	if (error)
> -		return error;
> -
> -	error = xfs_inode_from_disk(ip, dip);
> -	if (error)
> -		goto out_brelse;
> -
> -	/*
> -	 * Mark the buffer containing the inode as something to keep
> -	 * around for a while.  This helps to keep recently accessed
> -	 * meta-data in-core longer.
> -	 */
> -	xfs_buf_set_ref(bp, XFS_INO_REF);
> -
> -	/*
> -	 * Use xfs_trans_brelse() to release the buffer containing the on-disk
> -	 * inode, because it was acquired with xfs_trans_read_buf() in
> -	 * xfs_imap_to_bp() above.  If tp is NULL, this is just a normal
> -	 * brelse().  If we're within a transaction, then xfs_trans_brelse()
> -	 * will only release the buffer if it is not dirty within the
> -	 * transaction.  It will be OK to release the buffer in this case,
> -	 * because inodes on disk are never destroyed and we will be locking the
> -	 * new in-core inode before putting it in the cache where other
> -	 * processes can find it.  Thus we don't have to worry about the inode
> -	 * being changed just because we released the buffer.
> -	 */
> - out_brelse:
> -	xfs_trans_brelse(tp, bp);
> -	return error;
> -}
> -
>  /*
>   * Validate di_extsize hint.
>   *
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 0fbb99224ec73..e4cbcaf62a32b 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -49,8 +49,6 @@ struct xfs_imap {
>  int	xfs_imap_to_bp(struct xfs_mount *, struct xfs_trans *,
>  		       struct xfs_imap *, struct xfs_dinode **,
>  		       struct xfs_buf **, uint);
> -int	xfs_iread(struct xfs_mount *, struct xfs_trans *,
> -		  struct xfs_inode *, uint);
>  void	xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
>  void	xfs_inode_to_disk(struct xfs_inode *ip, struct xfs_dinode *to,
>  			  xfs_lsn_t lsn);
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 922a29032e374..af5748f5d9271 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -22,6 +22,7 @@
>  #include "xfs_dquot_item.h"
>  #include "xfs_dquot.h"
>  #include "xfs_reflink.h"
> +#include "xfs_ialloc.h"
>  
>  #include <linux/iversion.h>
>  
> @@ -508,10 +509,40 @@ xfs_iget_cache_miss(
>  	if (!ip)
>  		return -ENOMEM;
>  
> -	error = xfs_iread(mp, tp, ip, flags);
> +	error = xfs_imap(mp, tp, ip->i_ino, &ip->i_imap, flags);
>  	if (error)
>  		goto out_destroy;
>  
> +	/*
> +	 * For version 5 superblocks, if we are initialising a new inode and we
> +	 * are not utilising the XFS_MOUNT_IKEEP inode cluster mode, we can
> +	 * simply build the new inode core with a random generation number.
> +	 *
> +	 * For version 4 (and older) superblocks, log recovery is dependent on
> +	 * the di_flushiter field being initialised from the current on-disk
> +	 * value and hence we must also read the inode off disk even when
> +	 * initializing new inodes.
> +	 */
> +	if (xfs_sb_version_has_v3inode(&mp->m_sb) &&
> +	    (flags & XFS_IGET_CREATE) && !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> +		VFS_I(ip)->i_generation = prandom_u32();
> +	} else {
> +		struct xfs_dinode	*dip;
> +		struct xfs_buf		*bp;
> +
> +		error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
> +		if (error)
> +			goto out_destroy;
> +
> +		error = xfs_inode_from_disk(ip, dip);
> +		if (!error)
> +			xfs_buf_set_ref(bp, XFS_INO_REF);
> +		xfs_trans_brelse(tp, bp);
> +
> +		if (error)
> +			goto out_destroy;
> +	}
> +
>  	if (!xfs_inode_verify_forks(ip)) {
>  		error = -EFSCORRUPTED;
>  		goto out_destroy;
> -- 
> 2.26.2
> 

[PATCH 08/12] xfs: remove xfs_ifork_ops

[Christoph Hellwig]

xfs_ifork_ops add up to two indirect calls per inode read and flush,
despite just having a single instance in the kernel.  In xfsprogs
phase6 in xfs_repair overrides the verify_dir method to deal with inodes
that do not have a valid parent, but that can be fixed pretty easily
by ensuring they always have a valid looking parent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 19 +++++--------------
 fs/xfs/libxfs/xfs_inode_fork.h | 15 ++-------------
 fs/xfs/xfs_inode.c             |  4 ++--
 3 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 5fadfa9a17eb9..e346e143f1053 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -673,18 +673,10 @@ xfs_ifork_init_cow(
 	ip->i_cnextents = 0;
 }
 
-/* Default fork content verifiers. */
-struct xfs_ifork_ops xfs_default_ifork_ops = {
-	.verify_attr	= xfs_attr_shortform_verify,
-	.verify_dir	= xfs_dir2_sf_verify,
-	.verify_symlink	= xfs_symlink_shortform_verify,
-};
-
 /* Verify the inline contents of the data fork of an inode. */
 xfs_failaddr_t
 xfs_ifork_verify_data(
-	struct xfs_inode	*ip,
-	struct xfs_ifork_ops	*ops)
+	struct xfs_inode	*ip)
 {
 	/* Non-local data fork, we're done. */
 	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
@@ -693,9 +685,9 @@ xfs_ifork_verify_data(
 	/* Check the inline data fork if there is one. */
 	switch (VFS_I(ip)->i_mode & S_IFMT) {
 	case S_IFDIR:
-		return ops->verify_dir(ip);
+		return xfs_dir2_sf_verify(ip);
 	case S_IFLNK:
-		return ops->verify_symlink(ip);
+		return xfs_symlink_shortform_verify(ip);
 	default:
 		return NULL;
 	}
@@ -704,13 +696,12 @@ xfs_ifork_verify_data(
 /* Verify the inline contents of the attr fork of an inode. */
 xfs_failaddr_t
 xfs_ifork_verify_attr(
-	struct xfs_inode	*ip,
-	struct xfs_ifork_ops	*ops)
+	struct xfs_inode	*ip)
 {
 	/* There has to be an attr fork allocated if aformat is local. */
 	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
 		return NULL;
 	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
 		return __this_address;
-	return ops->verify_attr(ip);
+	return xfs_attr_shortform_verify(ip);
 }
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 8487b0c88a75e..3f84d33abd3b7 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -176,18 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
 
 extern void xfs_ifork_init_cow(struct xfs_inode *ip);
 
-typedef xfs_failaddr_t (*xfs_ifork_verifier_t)(struct xfs_inode *);
-
-struct xfs_ifork_ops {
-	xfs_ifork_verifier_t	verify_symlink;
-	xfs_ifork_verifier_t	verify_dir;
-	xfs_ifork_verifier_t	verify_attr;
-};
-extern struct xfs_ifork_ops	xfs_default_ifork_ops;
-
-xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip,
-		struct xfs_ifork_ops *ops);
-xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip,
-		struct xfs_ifork_ops *ops);
+xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
+xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
 
 #endif	/* __XFS_INODE_FORK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ab31a5dec7aab..25c00ffe18409 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3718,7 +3718,7 @@ xfs_inode_verify_forks(
 	struct xfs_ifork	*ifp;
 	xfs_failaddr_t		fa;
 
-	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
+	fa = xfs_ifork_verify_data(ip);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
@@ -3726,7 +3726,7 @@ xfs_inode_verify_forks(
 		return false;
 	}
 
-	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
+	fa = xfs_ifork_verify_attr(ip);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
-- 
2.26.2

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Brian Foster]

On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> xfs_ifork_ops add up to two indirect calls per inode read and flush,
> despite just having a single instance in the kernel.  In xfsprogs
> phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> that do not have a valid parent, but that can be fixed pretty easily
> by ensuring they always have a valid looking parent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Code looks fine, but I assume we'll want a repair fix completed and
merged before wiping this out:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_fork.c | 19 +++++--------------
>  fs/xfs/libxfs/xfs_inode_fork.h | 15 ++-------------
>  fs/xfs/xfs_inode.c             |  4 ++--
>  3 files changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 5fadfa9a17eb9..e346e143f1053 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -673,18 +673,10 @@ xfs_ifork_init_cow(
>  	ip->i_cnextents = 0;
>  }
>  
> -/* Default fork content verifiers. */
> -struct xfs_ifork_ops xfs_default_ifork_ops = {
> -	.verify_attr	= xfs_attr_shortform_verify,
> -	.verify_dir	= xfs_dir2_sf_verify,
> -	.verify_symlink	= xfs_symlink_shortform_verify,
> -};
> -
>  /* Verify the inline contents of the data fork of an inode. */
>  xfs_failaddr_t
>  xfs_ifork_verify_data(
> -	struct xfs_inode	*ip,
> -	struct xfs_ifork_ops	*ops)
> +	struct xfs_inode	*ip)
>  {
>  	/* Non-local data fork, we're done. */
>  	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> @@ -693,9 +685,9 @@ xfs_ifork_verify_data(
>  	/* Check the inline data fork if there is one. */
>  	switch (VFS_I(ip)->i_mode & S_IFMT) {
>  	case S_IFDIR:
> -		return ops->verify_dir(ip);
> +		return xfs_dir2_sf_verify(ip);
>  	case S_IFLNK:
> -		return ops->verify_symlink(ip);
> +		return xfs_symlink_shortform_verify(ip);
>  	default:
>  		return NULL;
>  	}
> @@ -704,13 +696,12 @@ xfs_ifork_verify_data(
>  /* Verify the inline contents of the attr fork of an inode. */
>  xfs_failaddr_t
>  xfs_ifork_verify_attr(
> -	struct xfs_inode	*ip,
> -	struct xfs_ifork_ops	*ops)
> +	struct xfs_inode	*ip)
>  {
>  	/* There has to be an attr fork allocated if aformat is local. */
>  	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
>  		return NULL;
>  	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
>  		return __this_address;
> -	return ops->verify_attr(ip);
> +	return xfs_attr_shortform_verify(ip);
>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 8487b0c88a75e..3f84d33abd3b7 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,18 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
>  
>  extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
> -typedef xfs_failaddr_t (*xfs_ifork_verifier_t)(struct xfs_inode *);
> -
> -struct xfs_ifork_ops {
> -	xfs_ifork_verifier_t	verify_symlink;
> -	xfs_ifork_verifier_t	verify_dir;
> -	xfs_ifork_verifier_t	verify_attr;
> -};
> -extern struct xfs_ifork_ops	xfs_default_ifork_ops;
> -
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip,
> -		struct xfs_ifork_ops *ops);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip,
> -		struct xfs_ifork_ops *ops);
> +xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> +xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ab31a5dec7aab..25c00ffe18409 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3718,7 +3718,7 @@ xfs_inode_verify_forks(
>  	struct xfs_ifork	*ifp;
>  	xfs_failaddr_t		fa;
>  
> -	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
> +	fa = xfs_ifork_verify_data(ip);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> @@ -3726,7 +3726,7 @@ xfs_inode_verify_forks(
>  		return false;
>  	}
>  
> -	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
> +	fa = xfs_ifork_verify_attr(ip);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -- 
> 2.26.2
> 

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Christoph Hellwig]

On Fri, May 08, 2020 at 11:05:43AM -0400, Brian Foster wrote:
> On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> > xfs_ifork_ops add up to two indirect calls per inode read and flush,
> > despite just having a single instance in the kernel.  In xfsprogs
> > phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> > that do not have a valid parent, but that can be fixed pretty easily
> > by ensuring they always have a valid looking parent.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> 
> Code looks fine, but I assume we'll want a repair fix completed and
> merged before wiping this out:

With the xfsprogs merge delays I'm not sure merged will work, but I'll
happily take your patch and get it in shape for submission.

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Brian Foster]

On Sat, May 09, 2020 at 10:17:15AM +0200, Christoph Hellwig wrote:
> On Fri, May 08, 2020 at 11:05:43AM -0400, Brian Foster wrote:
> > On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> > > xfs_ifork_ops add up to two indirect calls per inode read and flush,
> > > despite just having a single instance in the kernel.  In xfsprogs
> > > phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> > > that do not have a valid parent, but that can be fixed pretty easily
> > > by ensuring they always have a valid looking parent.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > 
> > Code looks fine, but I assume we'll want a repair fix completed and
> > merged before wiping this out:
> 
> With the xfsprogs merge delays I'm not sure merged will work, but I'll
> happily take your patch and get it in shape for submission.
> 

The critical bit is that repair is fixed before this lands in xfsprogs,
otherwise we just reintroduce the regression the callback mechanism was
designed to fix. The repair change is not huge, but it's not necessarily
trivial so it's probably worth making sure the repair change is at least
reviewed before putting this into the kernel pipeline.

BTW, I played with this a bit more yesterday and made some tweaks that I
think make it a little cleaner. Namely instead of processing the parent
bits in phases 3 and 4 and setting the parent in the internal structures
in phase 4, to do everything in phase 3 and skip the repeat checks in
phase 4. This has the side effect of eliminating some duplicate error
messages where repair complains about the original bogus value in phase
3, sets it to zero, and then complains about the zero value again in
phase 4. This still needs some auditing to assess whether we're losing
any extra verification by setting the parent in phase 3, however. It
also might be worth looking at giving the other dir formats the same
treatment. Squashed diff of my local tree below...

Brian

diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 6685a4d2..96ed6a5b 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -859,14 +859,7 @@ next_readbuf:
 		 */
 		if (isa_dir)  {
 			set_inode_isadir(ino_rec, irec_offset);
-			/*
-			 * we always set the parent but
-			 * we may as well wait until
-			 * phase 4 (no inode discovery)
-			 * because the parent info will
-			 * be solid then.
-			 */
-			if (!ino_discovery)  {
+			if (ino_discovery)  {
 				ASSERT(parent != 0);
 				set_inode_parent(ino_rec, irec_offset, parent);
 				ASSERT(parent ==
diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..9c789b4a 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,6 @@ process_sf_dir2(
 	int			tmp_elen;
 	int			tmp_len;
 	xfs_dir2_sf_entry_t	*tmp_sfep;
-	xfs_ino_t		zero = 0;
 
 	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
 	max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -480,6 +479,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
 	 * check parent (..) entry
 	 */
 	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
+	if (!ino_discovery)
+		return 0;
+
 
 	/*
 	 * if parent entry is bogus, null it out.  we'll fix it later .
@@ -494,7 +496,7 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
@@ -529,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
diff --git a/repair/phase6.c b/repair/phase6.c
index beceea9a..43bcea50 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
 						1,
 						XFS_DIR3_FT_DIR};
 
-/*
- * When we're checking directory inodes, we're allowed to set a directory's
- * dotdot entry to zero to signal that the parent needs to be reconnected
- * during phase 6.  If we're handling a shortform directory the ifork
- * verifiers will fail, so temporarily patch out this canary so that we can
- * verify the rest of the fork and move on to fixing the dir.
- */
-static xfs_failaddr_t
-phase6_verify_dir(
-	struct xfs_inode		*ip)
-{
-	struct xfs_mount		*mp = ip->i_mount;
-	struct xfs_ifork		*ifp;
-	struct xfs_dir2_sf_hdr		*sfp;
-	xfs_failaddr_t			fa;
-	xfs_ino_t			old_parent;
-	bool				parent_bypass = false;
-	int				size;
-
-	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
-	size = ifp->if_bytes;
-
-	/*
-	 * If this is a shortform directory, phase4 may have set the parent
-	 * inode to zero to indicate that it must be fixed.  Temporarily
-	 * set a valid parent so that the directory verifier will pass.
-	 */
-	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
-	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
-		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
-		if (old_parent == 0) {
-			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
-			parent_bypass = true;
-		}
-	}
-
-	fa = libxfs_default_ifork_ops.verify_dir(ip);
-
-	/* Put it back. */
-	if (parent_bypass)
-		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
-
-	return fa;
-}
-
-static struct xfs_ifork_ops phase6_ifork_ops = {
-	.verify_attr	= xfs_attr_shortform_verify,
-	.verify_dir	= phase6_verify_dir,
-	.verify_symlink	= xfs_symlink_shortform_verify,
-};
-
 /*
  * Data structures used to keep track of directories where the ".."
  * entries are updated. These must be rebuilt after the initial pass
@@ -1104,7 +1052,7 @@ mv_orphanage(
 					(unsigned long long)ino, ++incr);
 
 	/* Orphans may not have a proper parent, so use custom ops here */
-	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
+	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
 	if (err)
 		do_error(_("%d - couldn't iget disconnected inode\n"), err);
 
@@ -2875,7 +2823,7 @@ process_dir_inode(
 
 	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
 	if (error) {
 		if (!no_modify)
 			do_error(

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:13:44AM -0400, Brian Foster wrote:
> On Sat, May 09, 2020 at 10:17:15AM +0200, Christoph Hellwig wrote:
> > On Fri, May 08, 2020 at 11:05:43AM -0400, Brian Foster wrote:
> > > On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> > > > xfs_ifork_ops add up to two indirect calls per inode read and flush,
> > > > despite just having a single instance in the kernel.  In xfsprogs
> > > > phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> > > > that do not have a valid parent, but that can be fixed pretty easily
> > > > by ensuring they always have a valid looking parent.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > 
> > > Code looks fine, but I assume we'll want a repair fix completed and
> > > merged before wiping this out:
> > 
> > With the xfsprogs merge delays I'm not sure merged will work, but I'll
> > happily take your patch and get it in shape for submission.
> > 
> 
> The critical bit is that repair is fixed before this lands in xfsprogs,
> otherwise we just reintroduce the regression the callback mechanism was
> designed to fix. The repair change is not huge, but it's not necessarily
> trivial so it's probably worth making sure the repair change is at least
> reviewed before putting this into the kernel pipeline.
> 
> BTW, I played with this a bit more yesterday and made some tweaks that I
> think make it a little cleaner. Namely instead of processing the parent
> bits in phases 3 and 4 and setting the parent in the internal structures
> in phase 4, to do everything in phase 3 and skip the repeat checks in
> phase 4. This has the side effect of eliminating some duplicate error
> messages where repair complains about the original bogus value in phase
> 3, sets it to zero, and then complains about the zero value again in
> phase 4. This still needs some auditing to assess whether we're losing
> any extra verification by setting the parent in phase 3, however. It
> also might be worth looking at giving the other dir formats the same
> treatment. Squashed diff of my local tree below...
> 
> Brian
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 6685a4d2..96ed6a5b 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -859,14 +859,7 @@ next_readbuf:
>  		 */
>  		if (isa_dir)  {
>  			set_inode_isadir(ino_rec, irec_offset);
> -			/*
> -			 * we always set the parent but
> -			 * we may as well wait until
> -			 * phase 4 (no inode discovery)
> -			 * because the parent info will
> -			 * be solid then.
> -			 */
> -			if (!ino_discovery)  {
> +			if (ino_discovery)  {
>  				ASSERT(parent != 0);
>  				set_inode_parent(ino_rec, irec_offset, parent);
>  				ASSERT(parent ==
> diff --git a/repair/dir2.c b/repair/dir2.c
> index cbbce601..9c789b4a 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -165,7 +165,6 @@ process_sf_dir2(
>  	int			tmp_elen;
>  	int			tmp_len;
>  	xfs_dir2_sf_entry_t	*tmp_sfep;
> -	xfs_ino_t		zero = 0;
>  
>  	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
>  	max_size = XFS_DFORK_DSIZE(dip, mp);
> @@ -480,6 +479,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
>  	 * check parent (..) entry
>  	 */
>  	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
> +	if (!ino_discovery)
> +		return 0;
> +
>  
>  	/*
>  	 * if parent entry is bogus, null it out.  we'll fix it later .
> @@ -494,7 +496,7 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
>  		if (!no_modify)  {
>  			do_warn(_("clearing inode number\n"));
>  
> -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
>  			*dino_dirty = 1;
>  			*repair = 1;
>  		} else  {
> @@ -529,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
>  		if (!no_modify)  {
>  			do_warn(_("clearing inode number\n"));
>  
> -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
>  			*dino_dirty = 1;
>  			*repair = 1;
>  		} else  {
> diff --git a/repair/phase6.c b/repair/phase6.c
> index beceea9a..43bcea50 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
>  						1,
>  						XFS_DIR3_FT_DIR};
>  
> -/*
> - * When we're checking directory inodes, we're allowed to set a directory's
> - * dotdot entry to zero to signal that the parent needs to be reconnected
> - * during phase 6.  If we're handling a shortform directory the ifork
> - * verifiers will fail, so temporarily patch out this canary so that we can
> - * verify the rest of the fork and move on to fixing the dir.
> - */
> -static xfs_failaddr_t
> -phase6_verify_dir(
> -	struct xfs_inode		*ip)
> -{
> -	struct xfs_mount		*mp = ip->i_mount;
> -	struct xfs_ifork		*ifp;
> -	struct xfs_dir2_sf_hdr		*sfp;
> -	xfs_failaddr_t			fa;
> -	xfs_ino_t			old_parent;
> -	bool				parent_bypass = false;
> -	int				size;
> -
> -	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> -	size = ifp->if_bytes;
> -
> -	/*
> -	 * If this is a shortform directory, phase4 may have set the parent
> -	 * inode to zero to indicate that it must be fixed.  Temporarily
> -	 * set a valid parent so that the directory verifier will pass.
> -	 */
> -	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
> -	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
> -		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
> -		if (old_parent == 0) {
> -			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> -			parent_bypass = true;
> -		}
> -	}
> -
> -	fa = libxfs_default_ifork_ops.verify_dir(ip);
> -
> -	/* Put it back. */
> -	if (parent_bypass)
> -		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
> -
> -	return fa;
> -}
> -
> -static struct xfs_ifork_ops phase6_ifork_ops = {
> -	.verify_attr	= xfs_attr_shortform_verify,
> -	.verify_dir	= phase6_verify_dir,
> -	.verify_symlink	= xfs_symlink_shortform_verify,
> -};
> -
>  /*
>   * Data structures used to keep track of directories where the ".."
>   * entries are updated. These must be rebuilt after the initial pass
> @@ -1104,7 +1052,7 @@ mv_orphanage(
>  					(unsigned long long)ino, ++incr);
>  
>  	/* Orphans may not have a proper parent, so use custom ops here */
> -	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
> +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);

Hmm.  I'll have to look at this more thoroughly on a non-weekend, but I
think I like this approach, since it removes the weird quirk that if
repair fails after writing out a sf directory with parent==0, we'll have
transformed an fs with bad directory parent pointers to an fs with a
directory that totally fails the verifier.

So for the kernel patch, provisionally:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  	if (err)
>  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
>  
> @@ -2875,7 +2823,7 @@ process_dir_inode(
>  
>  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
>  
> -	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
> +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
>  	if (error) {
>  		if (!no_modify)
>  			do_error(
> 

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Brian Foster]

On Sat, May 16, 2020 at 10:48:02AM -0700, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 07:13:44AM -0400, Brian Foster wrote:
> > On Sat, May 09, 2020 at 10:17:15AM +0200, Christoph Hellwig wrote:
> > > On Fri, May 08, 2020 at 11:05:43AM -0400, Brian Foster wrote:
> > > > On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> > > > > xfs_ifork_ops add up to two indirect calls per inode read and flush,
> > > > > despite just having a single instance in the kernel.  In xfsprogs
> > > > > phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> > > > > that do not have a valid parent, but that can be fixed pretty easily
> > > > > by ensuring they always have a valid looking parent.
> > > > > 
> > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > ---
> > > > 
> > > > Code looks fine, but I assume we'll want a repair fix completed and
> > > > merged before wiping this out:
> > > 
> > > With the xfsprogs merge delays I'm not sure merged will work, but I'll
> > > happily take your patch and get it in shape for submission.
> > > 
> > 
> > The critical bit is that repair is fixed before this lands in xfsprogs,
> > otherwise we just reintroduce the regression the callback mechanism was
> > designed to fix. The repair change is not huge, but it's not necessarily
> > trivial so it's probably worth making sure the repair change is at least
> > reviewed before putting this into the kernel pipeline.
> > 
> > BTW, I played with this a bit more yesterday and made some tweaks that I
> > think make it a little cleaner. Namely instead of processing the parent
> > bits in phases 3 and 4 and setting the parent in the internal structures
> > in phase 4, to do everything in phase 3 and skip the repeat checks in
> > phase 4. This has the side effect of eliminating some duplicate error
> > messages where repair complains about the original bogus value in phase
> > 3, sets it to zero, and then complains about the zero value again in
> > phase 4. This still needs some auditing to assess whether we're losing
> > any extra verification by setting the parent in phase 3, however. It
> > also might be worth looking at giving the other dir formats the same
> > treatment. Squashed diff of my local tree below...
> > 
> > Brian
> > 
> > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > index 6685a4d2..96ed6a5b 100644
> > --- a/repair/dino_chunks.c
> > +++ b/repair/dino_chunks.c
> > @@ -859,14 +859,7 @@ next_readbuf:
> >  		 */
> >  		if (isa_dir)  {
> >  			set_inode_isadir(ino_rec, irec_offset);
> > -			/*
> > -			 * we always set the parent but
> > -			 * we may as well wait until
> > -			 * phase 4 (no inode discovery)
> > -			 * because the parent info will
> > -			 * be solid then.
> > -			 */
> > -			if (!ino_discovery)  {
> > +			if (ino_discovery)  {
> >  				ASSERT(parent != 0);
> >  				set_inode_parent(ino_rec, irec_offset, parent);
> >  				ASSERT(parent ==
> > diff --git a/repair/dir2.c b/repair/dir2.c
> > index cbbce601..9c789b4a 100644
> > --- a/repair/dir2.c
> > +++ b/repair/dir2.c
> > @@ -165,7 +165,6 @@ process_sf_dir2(
> >  	int			tmp_elen;
> >  	int			tmp_len;
> >  	xfs_dir2_sf_entry_t	*tmp_sfep;
> > -	xfs_ino_t		zero = 0;
> >  
> >  	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
> >  	max_size = XFS_DFORK_DSIZE(dip, mp);
> > @@ -480,6 +479,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
> >  	 * check parent (..) entry
> >  	 */
> >  	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
> > +	if (!ino_discovery)
> > +		return 0;
> > +
> >  
> >  	/*
> >  	 * if parent entry is bogus, null it out.  we'll fix it later .
> > @@ -494,7 +496,7 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
> >  		if (!no_modify)  {
> >  			do_warn(_("clearing inode number\n"));
> >  
> > -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> > +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> >  			*dino_dirty = 1;
> >  			*repair = 1;
> >  		} else  {
> > @@ -529,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
> >  		if (!no_modify)  {
> >  			do_warn(_("clearing inode number\n"));
> >  
> > -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> > +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> >  			*dino_dirty = 1;
> >  			*repair = 1;
> >  		} else  {
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index beceea9a..43bcea50 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
> >  						1,
> >  						XFS_DIR3_FT_DIR};
> >  
> > -/*
> > - * When we're checking directory inodes, we're allowed to set a directory's
> > - * dotdot entry to zero to signal that the parent needs to be reconnected
> > - * during phase 6.  If we're handling a shortform directory the ifork
> > - * verifiers will fail, so temporarily patch out this canary so that we can
> > - * verify the rest of the fork and move on to fixing the dir.
> > - */
> > -static xfs_failaddr_t
> > -phase6_verify_dir(
> > -	struct xfs_inode		*ip)
> > -{
> > -	struct xfs_mount		*mp = ip->i_mount;
> > -	struct xfs_ifork		*ifp;
> > -	struct xfs_dir2_sf_hdr		*sfp;
> > -	xfs_failaddr_t			fa;
> > -	xfs_ino_t			old_parent;
> > -	bool				parent_bypass = false;
> > -	int				size;
> > -
> > -	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > -	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > -	size = ifp->if_bytes;
> > -
> > -	/*
> > -	 * If this is a shortform directory, phase4 may have set the parent
> > -	 * inode to zero to indicate that it must be fixed.  Temporarily
> > -	 * set a valid parent so that the directory verifier will pass.
> > -	 */
> > -	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
> > -	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
> > -		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
> > -		if (old_parent == 0) {
> > -			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> > -			parent_bypass = true;
> > -		}
> > -	}
> > -
> > -	fa = libxfs_default_ifork_ops.verify_dir(ip);
> > -
> > -	/* Put it back. */
> > -	if (parent_bypass)
> > -		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
> > -
> > -	return fa;
> > -}
> > -
> > -static struct xfs_ifork_ops phase6_ifork_ops = {
> > -	.verify_attr	= xfs_attr_shortform_verify,
> > -	.verify_dir	= phase6_verify_dir,
> > -	.verify_symlink	= xfs_symlink_shortform_verify,
> > -};
> > -
> >  /*
> >   * Data structures used to keep track of directories where the ".."
> >   * entries are updated. These must be rebuilt after the initial pass
> > @@ -1104,7 +1052,7 @@ mv_orphanage(
> >  					(unsigned long long)ino, ++incr);
> >  
> >  	/* Orphans may not have a proper parent, so use custom ops here */
> > -	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
> > +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
> 
> Hmm.  I'll have to look at this more thoroughly on a non-weekend, but I
> think I like this approach, since it removes the weird quirk that if
> repair fails after writing out a sf directory with parent==0, we'll have
> transformed an fs with bad directory parent pointers to an fs with a
> directory that totally fails the verifier.
> 

I don't _think_ xfs_repair should ever write back the parent == 0
condition it creates. IIUC it's intended to be a transient state and the
directory should eventually be fixed up or junked (or maybe lost+found?)
in phase 6. However there is a similar quirk within xfs_repair itself in
that phase 3 sets the parent from whatever bad value it is to zero, then
phase 4 repeats the same general scan and complains about parent == 0
because that is also invalid. ;P

Brian

> So for the kernel patch, provisionally:
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
> >  	if (err)
> >  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
> >  
> > @@ -2875,7 +2823,7 @@ process_dir_inode(
> >  
> >  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
> >  
> > -	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
> > +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
> >  	if (error) {
> >  		if (!no_modify)
> >  			do_error(
> > 
> 

Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Darrick J. Wong]

On Mon, May 18, 2020 at 09:35:46AM -0400, Brian Foster wrote:
> On Sat, May 16, 2020 at 10:48:02AM -0700, Darrick J. Wong wrote:
> > On Sat, May 09, 2020 at 07:13:44AM -0400, Brian Foster wrote:
> > > On Sat, May 09, 2020 at 10:17:15AM +0200, Christoph Hellwig wrote:
> > > > On Fri, May 08, 2020 at 11:05:43AM -0400, Brian Foster wrote:
> > > > > On Fri, May 08, 2020 at 08:34:19AM +0200, Christoph Hellwig wrote:
> > > > > > xfs_ifork_ops add up to two indirect calls per inode read and flush,
> > > > > > despite just having a single instance in the kernel.  In xfsprogs
> > > > > > phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> > > > > > that do not have a valid parent, but that can be fixed pretty easily
> > > > > > by ensuring they always have a valid looking parent.
> > > > > > 
> > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > > ---
> > > > > 
> > > > > Code looks fine, but I assume we'll want a repair fix completed and
> > > > > merged before wiping this out:
> > > > 
> > > > With the xfsprogs merge delays I'm not sure merged will work, but I'll
> > > > happily take your patch and get it in shape for submission.
> > > > 
> > > 
> > > The critical bit is that repair is fixed before this lands in xfsprogs,
> > > otherwise we just reintroduce the regression the callback mechanism was
> > > designed to fix. The repair change is not huge, but it's not necessarily
> > > trivial so it's probably worth making sure the repair change is at least
> > > reviewed before putting this into the kernel pipeline.
> > > 
> > > BTW, I played with this a bit more yesterday and made some tweaks that I
> > > think make it a little cleaner. Namely instead of processing the parent
> > > bits in phases 3 and 4 and setting the parent in the internal structures
> > > in phase 4, to do everything in phase 3 and skip the repeat checks in
> > > phase 4. This has the side effect of eliminating some duplicate error
> > > messages where repair complains about the original bogus value in phase
> > > 3, sets it to zero, and then complains about the zero value again in
> > > phase 4. This still needs some auditing to assess whether we're losing
> > > any extra verification by setting the parent in phase 3, however. It
> > > also might be worth looking at giving the other dir formats the same
> > > treatment. Squashed diff of my local tree below...
> > > 
> > > Brian
> > > 
> > > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > > index 6685a4d2..96ed6a5b 100644
> > > --- a/repair/dino_chunks.c
> > > +++ b/repair/dino_chunks.c
> > > @@ -859,14 +859,7 @@ next_readbuf:
> > >  		 */
> > >  		if (isa_dir)  {
> > >  			set_inode_isadir(ino_rec, irec_offset);
> > > -			/*
> > > -			 * we always set the parent but
> > > -			 * we may as well wait until
> > > -			 * phase 4 (no inode discovery)
> > > -			 * because the parent info will
> > > -			 * be solid then.
> > > -			 */
> > > -			if (!ino_discovery)  {
> > > +			if (ino_discovery)  {
> > >  				ASSERT(parent != 0);
> > >  				set_inode_parent(ino_rec, irec_offset, parent);
> > >  				ASSERT(parent ==
> > > diff --git a/repair/dir2.c b/repair/dir2.c
> > > index cbbce601..9c789b4a 100644
> > > --- a/repair/dir2.c
> > > +++ b/repair/dir2.c
> > > @@ -165,7 +165,6 @@ process_sf_dir2(
> > >  	int			tmp_elen;
> > >  	int			tmp_len;
> > >  	xfs_dir2_sf_entry_t	*tmp_sfep;
> > > -	xfs_ino_t		zero = 0;
> > >  
> > >  	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
> > >  	max_size = XFS_DFORK_DSIZE(dip, mp);
> > > @@ -480,6 +479,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
> > >  	 * check parent (..) entry
> > >  	 */
> > >  	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
> > > +	if (!ino_discovery)
> > > +		return 0;
> > > +
> > >  
> > >  	/*
> > >  	 * if parent entry is bogus, null it out.  we'll fix it later .
> > > @@ -494,7 +496,7 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
> > >  		if (!no_modify)  {
> > >  			do_warn(_("clearing inode number\n"));
> > >  
> > > -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> > > +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> > >  			*dino_dirty = 1;
> > >  			*repair = 1;
> > >  		} else  {
> > > @@ -529,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
> > >  		if (!no_modify)  {
> > >  			do_warn(_("clearing inode number\n"));
> > >  
> > > -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> > > +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> > >  			*dino_dirty = 1;
> > >  			*repair = 1;
> > >  		} else  {
> > > diff --git a/repair/phase6.c b/repair/phase6.c
> > > index beceea9a..43bcea50 100644
> > > --- a/repair/phase6.c
> > > +++ b/repair/phase6.c
> > > @@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
> > >  						1,
> > >  						XFS_DIR3_FT_DIR};
> > >  
> > > -/*
> > > - * When we're checking directory inodes, we're allowed to set a directory's
> > > - * dotdot entry to zero to signal that the parent needs to be reconnected
> > > - * during phase 6.  If we're handling a shortform directory the ifork
> > > - * verifiers will fail, so temporarily patch out this canary so that we can
> > > - * verify the rest of the fork and move on to fixing the dir.
> > > - */
> > > -static xfs_failaddr_t
> > > -phase6_verify_dir(
> > > -	struct xfs_inode		*ip)
> > > -{
> > > -	struct xfs_mount		*mp = ip->i_mount;
> > > -	struct xfs_ifork		*ifp;
> > > -	struct xfs_dir2_sf_hdr		*sfp;
> > > -	xfs_failaddr_t			fa;
> > > -	xfs_ino_t			old_parent;
> > > -	bool				parent_bypass = false;
> > > -	int				size;
> > > -
> > > -	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > -	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > > -	size = ifp->if_bytes;
> > > -
> > > -	/*
> > > -	 * If this is a shortform directory, phase4 may have set the parent
> > > -	 * inode to zero to indicate that it must be fixed.  Temporarily
> > > -	 * set a valid parent so that the directory verifier will pass.
> > > -	 */
> > > -	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
> > > -	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
> > > -		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
> > > -		if (old_parent == 0) {
> > > -			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> > > -			parent_bypass = true;
> > > -		}
> > > -	}
> > > -
> > > -	fa = libxfs_default_ifork_ops.verify_dir(ip);
> > > -
> > > -	/* Put it back. */
> > > -	if (parent_bypass)
> > > -		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
> > > -
> > > -	return fa;
> > > -}
> > > -
> > > -static struct xfs_ifork_ops phase6_ifork_ops = {
> > > -	.verify_attr	= xfs_attr_shortform_verify,
> > > -	.verify_dir	= phase6_verify_dir,
> > > -	.verify_symlink	= xfs_symlink_shortform_verify,
> > > -};
> > > -
> > >  /*
> > >   * Data structures used to keep track of directories where the ".."
> > >   * entries are updated. These must be rebuilt after the initial pass
> > > @@ -1104,7 +1052,7 @@ mv_orphanage(
> > >  					(unsigned long long)ino, ++incr);
> > >  
> > >  	/* Orphans may not have a proper parent, so use custom ops here */
> > > -	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
> > > +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
> > 
> > Hmm.  I'll have to look at this more thoroughly on a non-weekend, but I
> > think I like this approach, since it removes the weird quirk that if
> > repair fails after writing out a sf directory with parent==0, we'll have
> > transformed an fs with bad directory parent pointers to an fs with a
> > directory that totally fails the verifier.
> > 
> 
> I don't _think_ xfs_repair should ever write back the parent == 0
> condition it creates. IIUC it's intended to be a transient state and the
> directory should eventually be fixed up or junked (or maybe lost+found?)

I've found those zeroes on disk in the past.  I think that happens if
you tweak the buffer cache in just the right way that a subsequent
cache_node_lookup causes a cache shake that starts writing things back
to disk to shove buffers onto the MRU.  In that particular instance, the
cache behavior was due to some other stupid bug that I'd accidentally
put in my dev tree, but at the time I recall noticing that it is
theoretically possible for buffers to get written out before the
explicit flush at the end.

> in phase 6. However there is a similar quirk within xfs_repair itself in
> that phase 3 sets the parent from whatever bad value it is to zero, then
> phase 4 repeats the same general scan and complains about parent == 0
> because that is also invalid. ;P

Yeah, that's also superfluous behavior. :)

--D

> Brian
> 
> > So for the kernel patch, provisionally:
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> > >  	if (err)
> > >  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
> > >  
> > > @@ -2875,7 +2823,7 @@ process_dir_inode(
> > >  
> > >  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
> > >  
> > > -	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
> > > +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
> > >  	if (error) {
> > >  		if (!no_modify)
> > >  			do_error(
> > > 
> > 
> 

[PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Christoph Hellwig]

The split between xfs_inode_verify_forks and the two helpers
implementing the actual functionality is a little strange.  Reshuffle
it so that xfs_inode_verify_forks verifies if the data and attr forks
are actually in local format and only call the low-level helpers if
that is the case.  Handle the actual error reporting in the low-level
handlers to streamline the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 51 ++++++++++++++++++++++------------
 fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
 fs/xfs/xfs_inode.c             | 21 +++-----------
 3 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index e346e143f1053..401921975d75b 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -674,34 +674,51 @@ xfs_ifork_init_cow(
 }
 
 /* Verify the inline contents of the data fork of an inode. */
-xfs_failaddr_t
-xfs_ifork_verify_data(
+int
+xfs_ifork_verify_local_data(
 	struct xfs_inode	*ip)
 {
-	/* Non-local data fork, we're done. */
-	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
-		return NULL;
+	xfs_failaddr_t		fa = NULL;
 
-	/* Check the inline data fork if there is one. */
 	switch (VFS_I(ip)->i_mode & S_IFMT) {
 	case S_IFDIR:
-		return xfs_dir2_sf_verify(ip);
+		fa = xfs_dir2_sf_verify(ip);
+		break;
 	case S_IFLNK:
-		return xfs_symlink_shortform_verify(ip);
+		fa = xfs_symlink_shortform_verify(ip);
+		break;
 	default:
-		return NULL;
+		break;
 	}
+
+	if (fa) {
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
+			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);
+		return -EFSCORRUPTED;
+	}
+
+	return 0;
 }
 
 /* Verify the inline contents of the attr fork of an inode. */
-xfs_failaddr_t
-xfs_ifork_verify_attr(
+int
+xfs_ifork_verify_local_attr(
 	struct xfs_inode	*ip)
 {
-	/* There has to be an attr fork allocated if aformat is local. */
-	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
-		return NULL;
-	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
-		return __this_address;
-	return xfs_attr_shortform_verify(ip);
+	struct xfs_ifork	*ifp = ip->i_afp;
+	xfs_failaddr_t		fa;
+
+	if (!ifp)
+		fa = __this_address;
+	else
+		fa = xfs_attr_shortform_verify(ip);
+
+	if (fa) {
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
+			ifp ? ifp->if_u1.if_data : NULL,
+			ifp ? ifp->if_bytes : 0, fa);
+		return -EFSCORRUPTED;
+	}
+
+	return 0;
 }
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 3f84d33abd3b7..f46a8c1db5964 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
 
 extern void xfs_ifork_init_cow(struct xfs_inode *ip);
 
-xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
-xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
+int xfs_ifork_verify_local_data(struct xfs_inode *ip);
+int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
 
 #endif	/* __XFS_INODE_FORK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 25c00ffe18409..c8abdefe00377 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3715,25 +3715,12 @@ bool
 xfs_inode_verify_forks(
 	struct xfs_inode	*ip)
 {
-	struct xfs_ifork	*ifp;
-	xfs_failaddr_t		fa;
-
-	fa = xfs_ifork_verify_data(ip);
-	if (fa) {
-		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
-				ifp->if_u1.if_data, ifp->if_bytes, fa);
+	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_data(ip))
 		return false;
-	}
-
-	fa = xfs_ifork_verify_attr(ip);
-	if (fa) {
-		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
-				ifp ? ifp->if_u1.if_data : NULL,
-				ifp ? ifp->if_bytes : 0, fa);
+	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_attr(ip))
 		return false;
-	}
 	return true;
 }
 
-- 
2.26.2

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Brian Foster]

On Fri, May 08, 2020 at 08:34:20AM +0200, Christoph Hellwig wrote:
> The split between xfs_inode_verify_forks and the two helpers
> implementing the actual functionality is a little strange.  Reshuffle
> it so that xfs_inode_verify_forks verifies if the data and attr forks
> are actually in local format and only call the low-level helpers if
> that is the case.  Handle the actual error reporting in the low-level
> handlers to streamline the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_fork.c | 51 ++++++++++++++++++++++------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
>  fs/xfs/xfs_inode.c             | 21 +++-----------
>  3 files changed, 40 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index e346e143f1053..401921975d75b 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -674,34 +674,51 @@ xfs_ifork_init_cow(
>  }
>  
>  /* Verify the inline contents of the data fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_data(
> +int
> +xfs_ifork_verify_local_data(
>  	struct xfs_inode	*ip)
>  {
> -	/* Non-local data fork, we're done. */
> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> +	xfs_failaddr_t		fa = NULL;
>  
> -	/* Check the inline data fork if there is one. */
>  	switch (VFS_I(ip)->i_mode & S_IFMT) {
>  	case S_IFDIR:
> -		return xfs_dir2_sf_verify(ip);
> +		fa = xfs_dir2_sf_verify(ip);
> +		break;
>  	case S_IFLNK:
> -		return xfs_symlink_shortform_verify(ip);
> +		fa = xfs_symlink_shortform_verify(ip);
> +		break;
>  	default:
> -		return NULL;
> +		break;
>  	}
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> +			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
>  
>  /* Verify the inline contents of the attr fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_attr(
> +int
> +xfs_ifork_verify_local_attr(
>  	struct xfs_inode	*ip)
>  {
> -	/* There has to be an attr fork allocated if aformat is local. */
> -	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> -	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> -		return __this_address;
> -	return xfs_attr_shortform_verify(ip);
> +	struct xfs_ifork	*ifp = ip->i_afp;
> +	xfs_failaddr_t		fa;
> +
> +	if (!ifp)
> +		fa = __this_address;
> +	else
> +		fa = xfs_attr_shortform_verify(ip);
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> +			ifp ? ifp->if_u1.if_data : NULL,
> +			ifp ? ifp->if_bytes : 0, fa);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 3f84d33abd3b7..f46a8c1db5964 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
>  
>  extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 25c00ffe18409..c8abdefe00377 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3715,25 +3715,12 @@ bool
>  xfs_inode_verify_forks(
>  	struct xfs_inode	*ip)
>  {
> -	struct xfs_ifork	*ifp;
> -	xfs_failaddr_t		fa;
> -
> -	fa = xfs_ifork_verify_data(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> -				ifp->if_u1.if_data, ifp->if_bytes, fa);
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
>  		return false;
> -	}
> -
> -	fa = xfs_ifork_verify_attr(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -				ifp ? ifp->if_u1.if_data : NULL,
> -				ifp ? ifp->if_bytes : 0, fa);
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		return false;
> -	}
>  	return true;
>  }
>  
> -- 
> 2.26.2
> 

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:20AM +0200, Christoph Hellwig wrote:
> The split between xfs_inode_verify_forks and the two helpers
> implementing the actual functionality is a little strange.  Reshuffle
> it so that xfs_inode_verify_forks verifies if the data and attr forks
> are actually in local format and only call the low-level helpers if
> that is the case.  Handle the actual error reporting in the low-level
> handlers to streamline the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c | 51 ++++++++++++++++++++++------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
>  fs/xfs/xfs_inode.c             | 21 +++-----------
>  3 files changed, 40 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index e346e143f1053..401921975d75b 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -674,34 +674,51 @@ xfs_ifork_init_cow(
>  }
>  
>  /* Verify the inline contents of the data fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_data(
> +int
> +xfs_ifork_verify_local_data(
>  	struct xfs_inode	*ip)
>  {
> -	/* Non-local data fork, we're done. */
> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> +	xfs_failaddr_t		fa = NULL;
>  
> -	/* Check the inline data fork if there is one. */
>  	switch (VFS_I(ip)->i_mode & S_IFMT) {
>  	case S_IFDIR:
> -		return xfs_dir2_sf_verify(ip);
> +		fa = xfs_dir2_sf_verify(ip);
> +		break;
>  	case S_IFLNK:
> -		return xfs_symlink_shortform_verify(ip);
> +		fa = xfs_symlink_shortform_verify(ip);
> +		break;
>  	default:
> -		return NULL;
> +		break;
>  	}
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> +			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);

Needs more indent, but I could fix this on merge...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
>  
>  /* Verify the inline contents of the attr fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_attr(
> +int
> +xfs_ifork_verify_local_attr(
>  	struct xfs_inode	*ip)
>  {
> -	/* There has to be an attr fork allocated if aformat is local. */
> -	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> -	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> -		return __this_address;
> -	return xfs_attr_shortform_verify(ip);
> +	struct xfs_ifork	*ifp = ip->i_afp;
> +	xfs_failaddr_t		fa;
> +
> +	if (!ifp)
> +		fa = __this_address;
> +	else
> +		fa = xfs_attr_shortform_verify(ip);
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> +			ifp ? ifp->if_u1.if_data : NULL,
> +			ifp ? ifp->if_bytes : 0, fa);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 3f84d33abd3b7..f46a8c1db5964 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
>  
>  extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 25c00ffe18409..c8abdefe00377 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3715,25 +3715,12 @@ bool
>  xfs_inode_verify_forks(
>  	struct xfs_inode	*ip)
>  {
> -	struct xfs_ifork	*ifp;
> -	xfs_failaddr_t		fa;
> -
> -	fa = xfs_ifork_verify_data(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> -				ifp->if_u1.if_data, ifp->if_bytes, fa);
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
>  		return false;
> -	}
> -
> -	fa = xfs_ifork_verify_attr(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -				ifp ? ifp->if_u1.if_data : NULL,
> -				ifp ? ifp->if_bytes : 0, fa);
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		return false;
> -	}
>  	return true;
>  }
>  
> -- 
> 2.26.2
> 

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Christoph Hellwig]

On Sat, May 16, 2020 at 10:49:27AM -0700, Darrick J. Wong wrote:
> > +int
> > +xfs_ifork_verify_local_data(
> >  	struct xfs_inode	*ip)
> >  {

> > +	if (fa) {
> > +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> > +			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);
> 
> Needs more indent, but I could fix this on merge...

Sure.  It seems like that is the only outstanding comment on this series,
so maybe you should just pick it up and I shouldn't bother to resend it?

[PATCH 10/12] xfs: improve local fork verification

[Christoph Hellwig]

Call the data/attr local fork verifies as soon as we are ready for them.
This keeps them close to the code setting up the forks, and avoids a
few branches later on.  Also open code xfs_inode_verify_forks in the
only remaining caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c |  8 +++++++-
 fs/xfs/xfs_icache.c            |  6 ------
 fs/xfs/xfs_inode.c             | 28 +++++++++-------------------
 fs/xfs/xfs_inode.h             |  2 --
 fs/xfs/xfs_log_recover.c       |  5 -----
 5 files changed, 16 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 401921975d75b..2fe325e38fd88 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -227,6 +227,7 @@ xfs_iformat_data_fork(
 	struct xfs_dinode	*dip)
 {
 	struct inode		*inode = VFS_I(ip);
+	int			error;
 
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFIFO:
@@ -241,8 +242,11 @@ xfs_iformat_data_fork(
 	case S_IFDIR:
 		switch (dip->di_format) {
 		case XFS_DINODE_FMT_LOCAL:
-			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
+			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK,
 					be64_to_cpu(dip->di_size));
+			if (!error)
+				error = xfs_ifork_verify_local_data(ip);
+			return error;
 		case XFS_DINODE_FMT_EXTENTS:
 			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
 		case XFS_DINODE_FMT_BTREE:
@@ -282,6 +286,8 @@ xfs_iformat_attr_fork(
 	case XFS_DINODE_FMT_LOCAL:
 		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
 				xfs_dfork_attr_shortform_size(dip));
+		if (!error)
+			error = xfs_ifork_verify_local_attr(ip);
 		break;
 	case XFS_DINODE_FMT_EXTENTS:
 		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index af5748f5d9271..5a3a520b95288 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -543,14 +543,8 @@ xfs_iget_cache_miss(
 			goto out_destroy;
 	}
 
-	if (!xfs_inode_verify_forks(ip)) {
-		error = -EFSCORRUPTED;
-		goto out_destroy;
-	}
-
 	trace_xfs_iget_miss(ip);
 
-
 	/*
 	 * Check the inode free state is valid. This also detects lookup
 	 * racing with unlinks.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c8abdefe00377..549ff468b7b60 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3707,23 +3707,6 @@ xfs_iflush(
 	return error;
 }
 
-/*
- * If there are inline format data / attr forks attached to this inode,
- * make sure they're not corrupt.
- */
-bool
-xfs_inode_verify_forks(
-	struct xfs_inode	*ip)
-{
-	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
-	    xfs_ifork_verify_local_data(ip))
-		return false;
-	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
-	    xfs_ifork_verify_local_attr(ip))
-		return false;
-	return true;
-}
-
 STATIC int
 xfs_iflush_int(
 	struct xfs_inode	*ip,
@@ -3808,8 +3791,15 @@ xfs_iflush_int(
 	if (!xfs_sb_version_has_v3inode(&mp->m_sb))
 		ip->i_d.di_flushiter++;
 
-	/* Check the inline fork data before we write out. */
-	if (!xfs_inode_verify_forks(ip))
+	/*
+	 * If there are inline format data / attr forks attached to this inode,
+	 * make sure they are not corrupt.
+	 */
+	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_data(ip))
+		goto flush_out;
+	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_attr(ip))
 		goto flush_out;
 
 	/*
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 83073c883fbf9..ff846197941e4 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -498,8 +498,6 @@ extern struct kmem_zone	*xfs_inode_zone;
 /* The default CoW extent size hint. */
 #define XFS_DEFAULT_COWEXTSZ_HINT 32
 
-bool xfs_inode_verify_forks(struct xfs_inode *ip);
-
 int xfs_iunlink_init(struct xfs_perag *pag);
 void xfs_iunlink_destroy(struct xfs_perag *pag);
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 3960caf51c9f7..87b940cb760db 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2878,11 +2878,6 @@ xfs_recover_inode_owner_change(
 	if (error)
 		goto out_free_ip;
 
-	if (!xfs_inode_verify_forks(ip)) {
-		error = -EFSCORRUPTED;
-		goto out_free_ip;
-	}
-
 	if (in_f->ilf_fields & XFS_ILOG_DOWNER) {
 		ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);
 		error = xfs_bmbt_change_owner(NULL, ip, XFS_DATA_FORK,
-- 
2.26.2

Re: [PATCH 10/12] xfs: improve local fork verification

[Brian Foster]

On Fri, May 08, 2020 at 08:34:21AM +0200, Christoph Hellwig wrote:
> Call the data/attr local fork verifies as soon as we are ready for them.
> This keeps them close to the code setting up the forks, and avoids a
> few branches later on.  Also open code xfs_inode_verify_forks in the
> only remaining caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_fork.c |  8 +++++++-
>  fs/xfs/xfs_icache.c            |  6 ------
>  fs/xfs/xfs_inode.c             | 28 +++++++++-------------------
>  fs/xfs/xfs_inode.h             |  2 --
>  fs/xfs/xfs_log_recover.c       |  5 -----
>  5 files changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 401921975d75b..2fe325e38fd88 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -227,6 +227,7 @@ xfs_iformat_data_fork(
>  	struct xfs_dinode	*dip)
>  {
>  	struct inode		*inode = VFS_I(ip);
> +	int			error;
>  
>  	switch (inode->i_mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -241,8 +242,11 @@ xfs_iformat_data_fork(
>  	case S_IFDIR:
>  		switch (dip->di_format) {
>  		case XFS_DINODE_FMT_LOCAL:
> -			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
> +			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK,
>  					be64_to_cpu(dip->di_size));
> +			if (!error)
> +				error = xfs_ifork_verify_local_data(ip);
> +			return error;
>  		case XFS_DINODE_FMT_EXTENTS:
>  			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
>  		case XFS_DINODE_FMT_BTREE:
> @@ -282,6 +286,8 @@ xfs_iformat_attr_fork(
>  	case XFS_DINODE_FMT_LOCAL:
>  		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
>  				xfs_dfork_attr_shortform_size(dip));
> +		if (!error)
> +			error = xfs_ifork_verify_local_attr(ip);
>  		break;
>  	case XFS_DINODE_FMT_EXTENTS:
>  		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index af5748f5d9271..5a3a520b95288 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -543,14 +543,8 @@ xfs_iget_cache_miss(
>  			goto out_destroy;
>  	}
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_destroy;
> -	}
> -
>  	trace_xfs_iget_miss(ip);
>  
> -
>  	/*
>  	 * Check the inode free state is valid. This also detects lookup
>  	 * racing with unlinks.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c8abdefe00377..549ff468b7b60 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3707,23 +3707,6 @@ xfs_iflush(
>  	return error;
>  }
>  
> -/*
> - * If there are inline format data / attr forks attached to this inode,
> - * make sure they're not corrupt.
> - */
> -bool
> -xfs_inode_verify_forks(
> -	struct xfs_inode	*ip)
> -{
> -	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_data(ip))
> -		return false;
> -	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_attr(ip))
> -		return false;
> -	return true;
> -}
> -
>  STATIC int
>  xfs_iflush_int(
>  	struct xfs_inode	*ip,
> @@ -3808,8 +3791,15 @@ xfs_iflush_int(
>  	if (!xfs_sb_version_has_v3inode(&mp->m_sb))
>  		ip->i_d.di_flushiter++;
>  
> -	/* Check the inline fork data before we write out. */
> -	if (!xfs_inode_verify_forks(ip))
> +	/*
> +	 * If there are inline format data / attr forks attached to this inode,
> +	 * make sure they are not corrupt.
> +	 */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
> +		goto flush_out;
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		goto flush_out;
>  
>  	/*
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 83073c883fbf9..ff846197941e4 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -498,8 +498,6 @@ extern struct kmem_zone	*xfs_inode_zone;
>  /* The default CoW extent size hint. */
>  #define XFS_DEFAULT_COWEXTSZ_HINT 32
>  
> -bool xfs_inode_verify_forks(struct xfs_inode *ip);
> -
>  int xfs_iunlink_init(struct xfs_perag *pag);
>  void xfs_iunlink_destroy(struct xfs_perag *pag);
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 3960caf51c9f7..87b940cb760db 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2878,11 +2878,6 @@ xfs_recover_inode_owner_change(
>  	if (error)
>  		goto out_free_ip;
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_free_ip;
> -	}
> -
>  	if (in_f->ilf_fields & XFS_ILOG_DOWNER) {
>  		ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);
>  		error = xfs_bmbt_change_owner(NULL, ip, XFS_DATA_FORK,
> -- 
> 2.26.2
> 

Re: [PATCH 10/12] xfs: improve local fork verification

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:21AM +0200, Christoph Hellwig wrote:
> Call the data/attr local fork verifies as soon as we are ready for them.
> This keeps them close to the code setting up the forks, and avoids a
> few branches later on.  Also open code xfs_inode_verify_forks in the
> only remaining caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_fork.c |  8 +++++++-
>  fs/xfs/xfs_icache.c            |  6 ------
>  fs/xfs/xfs_inode.c             | 28 +++++++++-------------------
>  fs/xfs/xfs_inode.h             |  2 --
>  fs/xfs/xfs_log_recover.c       |  5 -----
>  5 files changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 401921975d75b..2fe325e38fd88 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -227,6 +227,7 @@ xfs_iformat_data_fork(
>  	struct xfs_dinode	*dip)
>  {
>  	struct inode		*inode = VFS_I(ip);
> +	int			error;
>  
>  	switch (inode->i_mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -241,8 +242,11 @@ xfs_iformat_data_fork(
>  	case S_IFDIR:
>  		switch (dip->di_format) {
>  		case XFS_DINODE_FMT_LOCAL:
> -			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
> +			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK,
>  					be64_to_cpu(dip->di_size));
> +			if (!error)
> +				error = xfs_ifork_verify_local_data(ip);
> +			return error;
>  		case XFS_DINODE_FMT_EXTENTS:
>  			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
>  		case XFS_DINODE_FMT_BTREE:
> @@ -282,6 +286,8 @@ xfs_iformat_attr_fork(
>  	case XFS_DINODE_FMT_LOCAL:
>  		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
>  				xfs_dfork_attr_shortform_size(dip));
> +		if (!error)
> +			error = xfs_ifork_verify_local_attr(ip);
>  		break;
>  	case XFS_DINODE_FMT_EXTENTS:
>  		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index af5748f5d9271..5a3a520b95288 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -543,14 +543,8 @@ xfs_iget_cache_miss(
>  			goto out_destroy;
>  	}
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_destroy;
> -	}
> -
>  	trace_xfs_iget_miss(ip);
>  
> -
>  	/*
>  	 * Check the inode free state is valid. This also detects lookup
>  	 * racing with unlinks.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c8abdefe00377..549ff468b7b60 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3707,23 +3707,6 @@ xfs_iflush(
>  	return error;
>  }
>  
> -/*
> - * If there are inline format data / attr forks attached to this inode,
> - * make sure they're not corrupt.
> - */
> -bool
> -xfs_inode_verify_forks(
> -	struct xfs_inode	*ip)
> -{
> -	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_data(ip))
> -		return false;
> -	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_attr(ip))
> -		return false;
> -	return true;
> -}
> -
>  STATIC int
>  xfs_iflush_int(
>  	struct xfs_inode	*ip,
> @@ -3808,8 +3791,15 @@ xfs_iflush_int(
>  	if (!xfs_sb_version_has_v3inode(&mp->m_sb))
>  		ip->i_d.di_flushiter++;
>  
> -	/* Check the inline fork data before we write out. */
> -	if (!xfs_inode_verify_forks(ip))
> +	/*
> +	 * If there are inline format data / attr forks attached to this inode,
> +	 * make sure they are not corrupt.
> +	 */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
> +		goto flush_out;
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		goto flush_out;
>  
>  	/*
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 83073c883fbf9..ff846197941e4 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -498,8 +498,6 @@ extern struct kmem_zone	*xfs_inode_zone;
>  /* The default CoW extent size hint. */
>  #define XFS_DEFAULT_COWEXTSZ_HINT 32
>  
> -bool xfs_inode_verify_forks(struct xfs_inode *ip);
> -
>  int xfs_iunlink_init(struct xfs_perag *pag);
>  void xfs_iunlink_destroy(struct xfs_perag *pag);
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 3960caf51c9f7..87b940cb760db 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2878,11 +2878,6 @@ xfs_recover_inode_owner_change(
>  	if (error)
>  		goto out_free_ip;
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_free_ip;
> -	}
> -
>  	if (in_f->ilf_fields & XFS_ILOG_DOWNER) {
>  		ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);
>  		error = xfs_bmbt_change_owner(NULL, ip, XFS_DATA_FORK,
> -- 
> 2.26.2
> 

[PATCH 11/12] xfs: remove the special COW fork handling in xfs_bmapi_read

[Christoph Hellwig]

We don't call xfs_bmapi_read for the COW fork anymore, so remove the
special casing.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index fda13cd7add0e..76be1a18e2442 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3902,8 +3902,7 @@ xfs_bmapi_read(
 	int			whichfork = xfs_bmapi_whichfork(flags);
 
 	ASSERT(*nmap >= 1);
-	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
-			   XFS_BMAPI_COWFORK)));
+	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
 
 	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
@@ -3918,16 +3917,6 @@ xfs_bmapi_read(
 
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 	if (!ifp) {
-		/* No CoW fork?  Return a hole. */
-		if (whichfork == XFS_COW_FORK) {
-			mval->br_startoff = bno;
-			mval->br_startblock = HOLESTARTBLOCK;
-			mval->br_blockcount = len;
-			mval->br_state = XFS_EXT_NORM;
-			*nmap = 1;
-			return 0;
-		}
-
 		/*
 		 * A missing attr ifork implies that the inode says we're in
 		 * extents or btree format but failed to pass the inode fork
-- 
2.26.2

Re: [PATCH 11/12] xfs: remove the special COW fork handling in xfs_bmapi_read

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:22AM +0200, Christoph Hellwig wrote:
> We don't call xfs_bmapi_read for the COW fork anymore, so remove the
> special casing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

I was surprised this assertion, but apparently it's true, even in my dev
tree, so:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index fda13cd7add0e..76be1a18e2442 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3902,8 +3902,7 @@ xfs_bmapi_read(
>  	int			whichfork = xfs_bmapi_whichfork(flags);
>  
>  	ASSERT(*nmap >= 1);
> -	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
> -			   XFS_BMAPI_COWFORK)));
> +	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>  
>  	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> @@ -3918,16 +3917,6 @@ xfs_bmapi_read(
>  
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	if (!ifp) {
> -		/* No CoW fork?  Return a hole. */
> -		if (whichfork == XFS_COW_FORK) {
> -			mval->br_startoff = bno;
> -			mval->br_startblock = HOLESTARTBLOCK;
> -			mval->br_blockcount = len;
> -			mval->br_state = XFS_EXT_NORM;
> -			*nmap = 1;
> -			return 0;
> -		}
> -
>  		/*
>  		 * A missing attr ifork implies that the inode says we're in
>  		 * extents or btree format but failed to pass the inode fork
> -- 
> 2.26.2
> 

Re: [PATCH 11/12] xfs: remove the special COW fork handling in xfs_bmapi_read

[Christoph Hellwig]

On Sat, May 16, 2020 at 10:52:00AM -0700, Darrick J. Wong wrote:
> On Fri, May 08, 2020 at 08:34:22AM +0200, Christoph Hellwig wrote:
> > We don't call xfs_bmapi_read for the COW fork anymore, so remove the
> > special casing.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> I was surprised this assertion, but apparently it's true, even in my dev
> tree, so:

We really shouldn't add more xfs_bmapi_read callers anyway.  Going
straight to the xfs_iext_* APIs pretty much always improves the code.

[PATCH 12/12] xfs: remove the NULL fork handling in xfs_bmapi_read

[Christoph Hellwig]

Now that we fully verify the inode forks before they are added to the
inode cache, the crash reported in

  https://bugzilla.kernel.org/show_bug.cgi?id=204031

can't happen anymore, as we'll never let an inode that has inconsistent
nextents counts vs the presence of an in-core attr fork leak into the
inactivate code path.  So remove the work around to try to handle the
case, and just return an error and warn if the fork is not present.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 76be1a18e2442..34518a6dc7376 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3891,7 +3891,8 @@ xfs_bmapi_read(
 	int			flags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_ifork	*ifp;
+	int			whichfork = xfs_bmapi_whichfork(flags);
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
 	struct xfs_bmbt_irec	got;
 	xfs_fileoff_t		obno;
 	xfs_fileoff_t		end;
@@ -3899,12 +3900,14 @@ xfs_bmapi_read(
 	int			error;
 	bool			eof = false;
 	int			n = 0;
-	int			whichfork = xfs_bmapi_whichfork(flags);
 
 	ASSERT(*nmap >= 1);
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
 
+	if (WARN_ON_ONCE(!ifp))
+		return -EFSCORRUPTED;
+
 	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
 	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
 		return -EFSCORRUPTED;
@@ -3915,21 +3918,6 @@ xfs_bmapi_read(
 
 	XFS_STATS_INC(mp, xs_blk_mapr);
 
-	ifp = XFS_IFORK_PTR(ip, whichfork);
-	if (!ifp) {
-		/*
-		 * A missing attr ifork implies that the inode says we're in
-		 * extents or btree format but failed to pass the inode fork
-		 * verifier while trying to load it.  Treat that as a file
-		 * corruption too.
-		 */
-#ifdef DEBUG
-		xfs_alert(mp, "%s: inode %llu missing fork %d",
-				__func__, ip->i_ino, whichfork);
-#endif /* DEBUG */
-		return -EFSCORRUPTED;
-	}
-
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(NULL, ip, whichfork);
 		if (error)
-- 
2.26.2

Re: [PATCH 12/12] xfs: remove the NULL fork handling in xfs_bmapi_read

[Brian Foster]

On Fri, May 08, 2020 at 08:34:23AM +0200, Christoph Hellwig wrote:
> Now that we fully verify the inode forks before they are added to the
> inode cache, the crash reported in
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=204031
> 
> can't happen anymore, as we'll never let an inode that has inconsistent
> nextents counts vs the presence of an in-core attr fork leak into the
> inactivate code path.  So remove the work around to try to handle the
> case, and just return an error and warn if the fork is not present.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 76be1a18e2442..34518a6dc7376 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3891,7 +3891,8 @@ xfs_bmapi_read(
>  	int			flags)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_ifork	*ifp;
> +	int			whichfork = xfs_bmapi_whichfork(flags);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>  	struct xfs_bmbt_irec	got;
>  	xfs_fileoff_t		obno;
>  	xfs_fileoff_t		end;
> @@ -3899,12 +3900,14 @@ xfs_bmapi_read(
>  	int			error;
>  	bool			eof = false;
>  	int			n = 0;
> -	int			whichfork = xfs_bmapi_whichfork(flags);
>  
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>  
> +	if (WARN_ON_ONCE(!ifp))
> +		return -EFSCORRUPTED;
> +
>  	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
>  	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
>  		return -EFSCORRUPTED;
> @@ -3915,21 +3918,6 @@ xfs_bmapi_read(
>  
>  	XFS_STATS_INC(mp, xs_blk_mapr);
>  
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> -	if (!ifp) {
> -		/*
> -		 * A missing attr ifork implies that the inode says we're in
> -		 * extents or btree format but failed to pass the inode fork
> -		 * verifier while trying to load it.  Treat that as a file
> -		 * corruption too.
> -		 */
> -#ifdef DEBUG
> -		xfs_alert(mp, "%s: inode %llu missing fork %d",
> -				__func__, ip->i_ino, whichfork);
> -#endif /* DEBUG */
> -		return -EFSCORRUPTED;
> -	}
> -
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(NULL, ip, whichfork);
>  		if (error)
> -- 
> 2.26.2
> 

Re: [PATCH 12/12] xfs: remove the NULL fork handling in xfs_bmapi_read

[Darrick J. Wong]

On Fri, May 08, 2020 at 08:34:23AM +0200, Christoph Hellwig wrote:
> Now that we fully verify the inode forks before they are added to the
> inode cache, the crash reported in
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=204031
> 
> can't happen anymore, as we'll never let an inode that has inconsistent
> nextents counts vs the presence of an in-core attr fork leak into the
> inactivate code path.  So remove the work around to try to handle the
> case, and just return an error and warn if the fork is not present.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 76be1a18e2442..34518a6dc7376 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3891,7 +3891,8 @@ xfs_bmapi_read(
>  	int			flags)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_ifork	*ifp;
> +	int			whichfork = xfs_bmapi_whichfork(flags);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>  	struct xfs_bmbt_irec	got;
>  	xfs_fileoff_t		obno;
>  	xfs_fileoff_t		end;
> @@ -3899,12 +3900,14 @@ xfs_bmapi_read(
>  	int			error;
>  	bool			eof = false;
>  	int			n = 0;
> -	int			whichfork = xfs_bmapi_whichfork(flags);
>  
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>  
> +	if (WARN_ON_ONCE(!ifp))
> +		return -EFSCORRUPTED;
> +
>  	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
>  	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
>  		return -EFSCORRUPTED;
> @@ -3915,21 +3918,6 @@ xfs_bmapi_read(
>  
>  	XFS_STATS_INC(mp, xs_blk_mapr);
>  
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> -	if (!ifp) {
> -		/*
> -		 * A missing attr ifork implies that the inode says we're in
> -		 * extents or btree format but failed to pass the inode fork
> -		 * verifier while trying to load it.  Treat that as a file
> -		 * corruption too.
> -		 */
> -#ifdef DEBUG
> -		xfs_alert(mp, "%s: inode %llu missing fork %d",
> -				__func__, ip->i_ino, whichfork);
> -#endif /* DEBUG */
> -		return -EFSCORRUPTED;
> -	}
> -
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(NULL, ip, whichfork);
>  		if (error)
> -- 
> 2.26.2
> 

Re: dinode reading cleanups v2

[Christoph Hellwig]

As there were some conflicts due to the code moves for the log
refactoring I've pushed out a new v3 branch here:

    git://git.infradead.org/users/hch/xfs.git xfs-inode-read-cleanup.3

Gitweb:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-inode-read-cleanup.3

The only other change is extra indentation for the local fork verifiers.
I don't think it is worth reposting for that, unless you want me to.

Re: dinode reading cleanups v2

[Darrick J. Wong]

On Mon, May 18, 2020 at 08:48:59AM +0200, Christoph Hellwig wrote:
> As there were some conflicts due to the code moves for the log
> refactoring I've pushed out a new v3 branch here:
> 
>     git://git.infradead.org/users/hch/xfs.git xfs-inode-read-cleanup.3
> 
> Gitweb:
> 
>     http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-inode-read-cleanup.3
> 
> The only other change is extra indentation for the local fork verifiers.
> I don't think it is worth reposting for that, unless you want me to.

I already pulled in the v2 series (with the indentation changes); are
there any additional changes in v3?  I didn't see any changes diffing
your branch to mine, so I think everything is fine.

--D

Re: dinode reading cleanups v2

[Christoph Hellwig]

On Mon, May 18, 2020 at 10:36:22AM -0700, Darrick J. Wong wrote:
> On Mon, May 18, 2020 at 08:48:59AM +0200, Christoph Hellwig wrote:
> > As there were some conflicts due to the code moves for the log
> > refactoring I've pushed out a new v3 branch here:
> > 
> >     git://git.infradead.org/users/hch/xfs.git xfs-inode-read-cleanup.3
> > 
> > Gitweb:
> > 
> >     http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-inode-read-cleanup.3
> > 
> > The only other change is extra indentation for the local fork verifiers.
> > I don't think it is worth reposting for that, unless you want me to.
> 
> I already pulled in the v2 series (with the indentation changes); are
> there any additional changes in v3?  I didn't see any changes diffing
> your branch to mine, so I think everything is fine.

As said - just the indent change, and resolving the conflicts vs
the log recovery refactor.

dinode reading cleanups

[Christoph Hellwig]

Hi all,

while dusting off my series to move the per-fork extent count and format
into the xfs_ifork structure I found that we added a few more hacks in
the area it touched.  This series has some of the prep patch combined with
a restructure of the dinode reading path that cleans up handling of early
errors during dinode reading, and allows droppign a workaround in
xfs_bmapi_read.

Another side effect is that we can share more code with xfsprogs.

Git tree:

    git://git.infradead.org/users/hch/xfs.git xfs-inode-read-cleanup

Gitweb:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-inode-read-cleanup

[PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Christoph Hellwig]

The split between xfs_inode_verify_forks and the two helpers
implementing the actual functionality is a little strange.  Reshuffle
it so that xfs_inode_verify_forks verifies if the data and attr forks
are actually in local format and only call the low-level helpers if
that is the case.  Handle the actual error reporting in the low-level
handlers to streamline the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 47 ++++++++++++++++++++++------------
 fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
 fs/xfs/xfs_inode.c             | 21 +++------------
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index f6dcee919f59e..7e129ed2f345f 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -674,34 +674,49 @@ xfs_ifork_init_cow(
 }
 
 /* Verify the inline contents of the data fork of an inode. */
-xfs_failaddr_t
-xfs_ifork_verify_data(
+int
+xfs_ifork_verify_local_data(
 	struct xfs_inode	*ip)
 {
-	/* Non-local data fork, we're done. */
-	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
-		return NULL;
+	xfs_failaddr_t		fa = NULL;
 
-	/* Check the inline data fork if there is one. */
 	switch (VFS_I(ip)->i_mode & S_IFMT) {
 	case S_IFDIR:
-		return xfs_dir2_sf_verify(ip);
+		fa = xfs_dir2_sf_verify(ip);
+		break;
 	case S_IFLNK:
-		return xfs_symlink_shortform_verify(ip);
+		fa = xfs_symlink_shortform_verify(ip);
+		break;
 	default:
-		return NULL;
+		break;
 	}
+
+	if (fa) {
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
+			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);
+		return -EFSCORRUPTED;
+	}
+
+	return 0;
 }
 
 /* Verify the inline contents of the attr fork of an inode. */
-xfs_failaddr_t
-xfs_ifork_verify_attr(
+int
+xfs_ifork_verify_local_attr(
 	struct xfs_inode	*ip)
 {
-	/* There has to be an attr fork allocated if aformat is local. */
-	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
-		return NULL;
+	xfs_failaddr_t		fa;
+
 	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
-		return __this_address;
-	return xfs_attr_shortform_verify(ip);
+		fa = __this_address;
+	else
+		fa = xfs_attr_shortform_verify(ip);
+
+	if (fa) {
+		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
+			ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
+		return -EFSCORRUPTED;
+	}
+
+	return 0;
 }
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 3f84d33abd3b7..f46a8c1db5964 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
 
 extern void xfs_ifork_init_cow(struct xfs_inode *ip);
 
-xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
-xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
+int xfs_ifork_verify_local_data(struct xfs_inode *ip);
+int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
 
 #endif	/* __XFS_INODE_FORK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 93967278355de..2ec7789317133 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3766,25 +3766,12 @@ bool
 xfs_inode_verify_forks(
 	struct xfs_inode	*ip)
 {
-	struct xfs_ifork	*ifp;
-	xfs_failaddr_t		fa;
-
-	fa = xfs_ifork_verify_data(ip);
-	if (fa) {
-		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
-				ifp->if_u1.if_data, ifp->if_bytes, fa);
+	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_data(ip))
 		return false;
-	}
-
-	fa = xfs_ifork_verify_attr(ip);
-	if (fa) {
-		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
-		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
-				ifp ? ifp->if_u1.if_data : NULL,
-				ifp ? ifp->if_bytes : 0, fa);
+	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
+	    xfs_ifork_verify_local_attr(ip))
 		return false;
-	}
 	return true;
 }
 
-- 
2.26.2

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Brian Foster]

On Fri, May 01, 2020 at 10:14:21AM +0200, Christoph Hellwig wrote:
> The split between xfs_inode_verify_forks and the two helpers
> implementing the actual functionality is a little strange.  Reshuffle
> it so that xfs_inode_verify_forks verifies if the data and attr forks
> are actually in local format and only call the low-level helpers if
> that is the case.  Handle the actual error reporting in the low-level
> handlers to streamline the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c | 47 ++++++++++++++++++++++------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
>  fs/xfs/xfs_inode.c             | 21 +++------------
>  3 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index f6dcee919f59e..7e129ed2f345f 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -674,34 +674,49 @@ xfs_ifork_init_cow(
>  }
>  
...
>  
>  /* Verify the inline contents of the attr fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_attr(
> +int
> +xfs_ifork_verify_local_attr(
>  	struct xfs_inode	*ip)
>  {
> -	/* There has to be an attr fork allocated if aformat is local. */
> -	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> +	xfs_failaddr_t		fa;
> +
>  	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> -		return __this_address;
> -	return xfs_attr_shortform_verify(ip);
> +		fa = __this_address;
> +	else
> +		fa = xfs_attr_shortform_verify(ip);
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> +			ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
> +		return -EFSCORRUPTED;

This explicitly makes !ip->i_afp one of the handled corruption cases for
XFS_DINODE_FMT_LOCAL, but then attempts to access it anyways. Otherwise
seems Ok modulo the comments on the previous patch...

Brian

> +	}
> +
> +	return 0;
>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 3f84d33abd3b7..f46a8c1db5964 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
>  
>  extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 93967278355de..2ec7789317133 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3766,25 +3766,12 @@ bool
>  xfs_inode_verify_forks(
>  	struct xfs_inode	*ip)
>  {
> -	struct xfs_ifork	*ifp;
> -	xfs_failaddr_t		fa;
> -
> -	fa = xfs_ifork_verify_data(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> -				ifp->if_u1.if_data, ifp->if_bytes, fa);
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
>  		return false;
> -	}
> -
> -	fa = xfs_ifork_verify_attr(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -				ifp ? ifp->if_u1.if_data : NULL,
> -				ifp ? ifp->if_bytes : 0, fa);
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		return false;
> -	}
>  	return true;
>  }
>  
> -- 
> 2.26.2
> 

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Christoph Hellwig]

On Fri, May 01, 2020 at 11:57:24AM -0400, Brian Foster wrote:
> >  	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> > -		return __this_address;
> > -	return xfs_attr_shortform_verify(ip);
> > +		fa = __this_address;
> > +	else
> > +		fa = xfs_attr_shortform_verify(ip);
> > +
> > +	if (fa) {
> > +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> > +			ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
> > +		return -EFSCORRUPTED;
> 
> This explicitly makes !ip->i_afp one of the handled corruption cases for
> XFS_DINODE_FMT_LOCAL, but then attempts to access it anyways. Otherwise
> seems Ok modulo the comments on the previous patch...

No, it keeps the existing bogus behavior, just making the bug more
obvious by moving the bits closer together :(

That being said this is getting fixed later in the series.

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Brian Foster]

On Fri, May 01, 2020 at 06:40:09PM +0200, Christoph Hellwig wrote:
> On Fri, May 01, 2020 at 11:57:24AM -0400, Brian Foster wrote:
> > >  	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> > > -		return __this_address;
> > > -	return xfs_attr_shortform_verify(ip);
> > > +		fa = __this_address;
> > > +	else
> > > +		fa = xfs_attr_shortform_verify(ip);
> > > +
> > > +	if (fa) {
> > > +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> > > +			ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
> > > +		return -EFSCORRUPTED;
> > 
> > This explicitly makes !ip->i_afp one of the handled corruption cases for
> > XFS_DINODE_FMT_LOCAL, but then attempts to access it anyways. Otherwise
> > seems Ok modulo the comments on the previous patch...
> 
> No, it keeps the existing bogus behavior, just making the bug more
> obvious by moving the bits closer together :(
> 

The associated code replaced in this patch checks the attr fork pointer:

-               ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
-               xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
-                               ifp ? ifp->if_u1.if_data : NULL,
-                               ifp ? ifp->if_bytes : 0, fa);

Brian

> That being said this is getting fixed later in the series.
> 

Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Christoph Hellwig]

On Fri, May 01, 2020 at 01:02:27PM -0400, Brian Foster wrote:
> The associated code replaced in this patch checks the attr fork pointer:
> 
> -               ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> -               xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -                               ifp ? ifp->if_u1.if_data : NULL,
> -                               ifp ? ifp->if_bytes : 0, fa);

Oh, true.  I'll fix this up to keep bisectability.