On Fri, May 01, 2020 at 10:14:16AM +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@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 54 ++++++++++++-----------------------
> 1 file changed, 18 insertions(+), 36 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 983beb680e81a..b136f29f7d9d3 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -198,6 +198,21 @@ xfs_inode_from_disk(
> ASSERT(ip->i_cowfp == NULL);
> ASSERT(ip->i_afp == NULL);
>
> + /*
> + * Get the truly permanent information first that is not overwritten by
> + * xfs_ialloc first. This also includes i_mode so that a newly read
> + * in inode structure for an allocation is marked as already free.
> + */
The first sentence has a wording issue and the second is kind of
confusing. I think this can be simplified and condensed further (see the
diff below for example). Otherwise looks good.
Brian
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index b136f29f7d9d..ed02649138aa 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -199,17 +199,14 @@ xfs_inode_from_disk(
ASSERT(ip->i_afp == NULL);
/*
- * Get the truly permanent information first that is not overwritten by
- * xfs_ialloc first. This also includes i_mode so that a newly read
- * in inode structure for an allocation is marked as already free.
+ * 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);
- to->di_flushiter = be16_to_cpu(from->di_flushiter);
- /*
- * Only copy the rest if the inode is actually allocated.
- */
if (!inode->i_mode)
return 0;
> + inode->i_generation = be32_to_cpu(from->di_gen);
> + inode->i_mode = be16_to_cpu(from->di_mode);
> + to->di_flushiter = be16_to_cpu(from->di_flushiter);
> +
> + /*
> + * Only copy the rest if the inode is actually allocated.
> + */
> + 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.
> @@ -215,7 +230,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
> @@ -229,8 +243,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);
> @@ -659,39 +671,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
>
