diff mbox series

[1/6] mkfs: check root inode location

Message ID 157982505230.2765631.2328249334657581135.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs_repair: do not trash valid root dirs | expand

Commit Message

Darrick J. Wong Jan. 24, 2020, 12:17 a.m. UTC 
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          |   39 +++++++++++++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 6 deletions(-)

Comments

Eric Sandeen Jan. 30, 2020, 7:32 p.m. UTC | #1 
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.

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

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
>
Darrick J. Wong Jan. 30, 2020, 8:19 p.m. UTC | #2 
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
> >
Eric Sandeen Jan. 30, 2020, 8:34 p.m. UTC | #3 
On 1/30/20 2:19 PM, Darrick J. Wong wrote:
> 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.

Oh, for sure.  But it's still:

"haha something went wrong, HAND PLONK"

I guess the savvy user would come to the list or file a bug.

(Hm, you know, a routine to dump cfg-> out for debugging would be neat.)

-Eric
diff mbox series

Patch

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