Re: [PATCH 2/4] mkfs: check root inode location

[Brian Foster <bfoster@redhat.com>]

On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure the root inode gets created where repair thinks it should be
> created.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/libxfs_api_defs.h |    1 +
>  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 645c9b1b..8f6b9fc2 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -156,5 +156,6 @@
>  
>  #define xfs_ag_init_headers		libxfs_ag_init_headers
>  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
>  

Perhaps this should be in the previous patch..?


>  #endif /* __LIBXFS_API_DEFS_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 18338a61..5143d9b4 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
>  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
>  }
>  
> +static void
> +check_root_ino(
> +	struct xfs_mount	*mp)
> +{
> +	xfs_agino_t		first, last;
> +
> +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> +		fprintf(stderr,
> +			_("%s: root inode created in AG %u, not AG 0\n"),
> +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> +		exit(1);
> +	}
> +
> +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> +		fprintf(stderr,
> +			_("%s: root inode (%llu) not created in first chunk\n"),
> +			progname, (unsigned long long)mp->m_sb.sb_rootino);

If the root inode ended up somewhere in the middle of the first chunk,
we'd fail (rightly), but with a misleading error message. Perhaps
something like "root inode (..) not allocated in expected location"
would be better? I'd also like to see a comment somewhere in here to
explain why we have this check. For example:

"The superblock refers directly to the root inode, but repair makes
hardcoded assumptions about its location based on filesystem geometry
for an extra level of verification. If this assumption ever breaks, we
should flag it immediately and fail the mkfs. Otherwise repair may
consider the filesystem corrupt and toss the root inode."

Feel free to reword that however appropriate (given the behavior change
in subsequent patches), of course..

Brian

> +		exit(1);
> +	}
> +}
> +
>  int
>  main(
>  	int			argc,
> @@ -3807,12 +3829,7 @@ main(
>  	/*
>  	 * Protect ourselves against possible stupidity
>  	 */
> -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> -		fprintf(stderr,
> -			_("%s: root inode created in AG %u, not AG 0\n"),
> -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> -		exit(1);
> -	}
> +	check_root_ino(mp);
>  
>  	/*
>  	 * Re-write multiple secondary superblocks with rootinode field set
>