Re: [PATCH 1/6] mkfs: check root inode location

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Jan 30, 2020 at 01:32:30PM -0600, Eric Sandeen wrote:
> On 1/23/20 6:17 PM, 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.
> 
> Actual mkfs-time location calculation is still completely separate from 
> the code in xfs_ialloc_calc_rootino though, right?  Maybe there's nothing
> to do about that.

Correct, because proto.c uses the regular inode allocation routines to
create the root inode, and mkfs doesn't have the ability to compute the
root inode and Make It So.

> I mostly find myself wondering what a user will do next if this check fails.

Complain. :)

To be fair, if there was a mismatch prior to this patch, the user would
end up with a filesystem that formats fine, mounts ok, and explodes in
xfs_repair.  Better we fail early than have repair shred the filesystem
after they've loaded up their production data and deleted the backups.

--D

> Assuming we trust xfs_ialloc_calc_rootino though, this seems fine.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> -Eric
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/libxfs_api_defs.h |    1 +
> >  mkfs/xfs_mkfs.c          |   39 +++++++++++++++++++++++++++++++++------
> >  2 files changed, 34 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > index cc7304ad..9ede0125 100644
> > --- a/libxfs/libxfs_api_defs.h
> > +++ b/libxfs/libxfs_api_defs.h
> > @@ -172,6 +172,7 @@
> >  
> >  #define xfs_ag_init_headers		libxfs_ag_init_headers
> >  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> > +#define xfs_ialloc_calc_rootino		libxfs_ialloc_calc_rootino
> >  
> >  #define xfs_refcountbt_calc_reserves	libxfs_refcountbt_calc_reserves
> >  #define xfs_finobt_calc_reserves	libxfs_finobt_calc_reserves
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 784fe6a9..91a25bf5 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3549,6 +3549,38 @@ rewrite_secondary_superblocks(
> >  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> >  }
> >  
> > +static void
> > +check_root_ino(
> > +	struct xfs_mount	*mp)
> > +{
> > +	xfs_ino_t		ino;
> > +
> > +	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);
> > +	}
> > +
> > +	/*
> > +	 * The superblock points to the root directory inode, but xfs_repair
> > +	 * expects to find the root inode in a very specific location computed
> > +	 * from the filesystem geometry for an extra level of verification.
> > +	 *
> > +	 * Fail the format immediately if those assumptions ever break, because
> > +	 * repair will toss the root directory.
> > +	 */
> > +	ino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
> > +	if (mp->m_sb.sb_rootino != ino) {
> > +		fprintf(stderr,
> > +	_("%s: root inode (%llu) not allocated in expected location (%llu)\n"),
> > +			progname,
> > +			(unsigned long long)mp->m_sb.sb_rootino,
> > +			(unsigned long long)ino);
> > +		exit(1);
> > +	}
> > +}
> > +
> >  int
> >  main(
> >  	int			argc,
> > @@ -3835,12 +3867,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
> >