[PATCH v3 0/6] xfs_repair: do not trash valid root dirs

[PATCH v3 0/6] xfs_repair: do not trash valid root dirs

[Darrick J. Wong]

Hi all,

Alex Lyakas observed that xfs_repair can accidentally trash the root
directory on a filesystem.  Specifically, if one formats a V4 filesystem
without sparse inodes but with sunit/swidth set and then mounts that
filesystem with a different sunit/swidth, the kernel will update the
values in the superblock.  This causes xfs_repair's sb_rootino
estimation to differ from the actual root directory, and it discards the
actual root directory even though there's nothing wrong with it.

Therefore, hoist the logic that computes the root inode location into
libxfs so that the kernel will avoid the sb update if the proposed
sunit/swidth changes would alter the sb_rootino estimation; and then
teach xfs_repair to retain the root directory even if the estimation
doesn't add up, as long as sb_rootino points to a directory whose '..'
entry points to itself.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-find-rootdir

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=repair-find-rootdir

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

[Darrick J. Wong]

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(-)


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

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

[Eric Sandeen]

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
> 

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

[Darrick J. Wong]

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
> > 

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

[Eric Sandeen]

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

[PATCH 2/6] xfs_repair: enforce that inode btree chunks can't point to AG headers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

xfs_repair has a very old check that evidently excuses the AG 0 inode
btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
AG headers).  mkfs never formats filesystems that way and it looks like
an error, so purge the check.  After this, we always complain if inodes
overlap with AG headers because that should never happen.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/globals.c    |    1 -
 repair/globals.h    |    1 -
 repair/scan.c       |   19 -------------------
 repair/xfs_repair.c |    7 -------
 4 files changed, 28 deletions(-)


diff --git a/repair/globals.c b/repair/globals.c
index dcd79ea4..8a60e706 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -73,7 +73,6 @@ int	lost_gquotino;
 int	lost_pquotino;
 
 xfs_agino_t	first_prealloc_ino;
-xfs_agino_t	last_prealloc_ino;
 xfs_agblock_t	bnobt_root;
 xfs_agblock_t	bcntbt_root;
 xfs_agblock_t	inobt_root;
diff --git a/repair/globals.h b/repair/globals.h
index 008bdd90..2ed5c894 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -114,7 +114,6 @@ extern int		lost_gquotino;
 extern int		lost_pquotino;
 
 extern xfs_agino_t	first_prealloc_ino;
-extern xfs_agino_t	last_prealloc_ino;
 extern xfs_agblock_t	bnobt_root;
 extern xfs_agblock_t	bcntbt_root;
 extern xfs_agblock_t	inobt_root;
diff --git a/repair/scan.c b/repair/scan.c
index c383f3aa..05707dd2 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1645,13 +1645,6 @@ scan_single_ino_chunk(
 				break;
 			case XR_E_INUSE_FS:
 			case XR_E_INUSE_FS1:
-				if (agno == 0 &&
-				    ino + j >= first_prealloc_ino &&
-				    ino + j < last_prealloc_ino) {
-					set_bmap(agno, agbno, XR_E_INO);
-					break;
-				}
-				/* fall through */
 			default:
 				/* XXX - maybe should mark block a duplicate */
 				do_warn(
@@ -1782,18 +1775,6 @@ _("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\
 				break;
 			case XR_E_INUSE_FS:
 			case XR_E_INUSE_FS1:
-				if (agno == 0 &&
-				    ino + j >= first_prealloc_ino &&
-				    ino + j < last_prealloc_ino) {
-					do_warn(
-_("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\n"),
-						agno, agbno, mp->m_sb.sb_inopblock);
-
-					set_bmap(agno, agbno, XR_E_INO);
-					suspect++;
-					break;
-				}
-				/* fall through */
 			default:
 				do_warn(
 _("inode chunk claims used block, finobt block - agno %d, bno %d, inopb %d\n"),
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9295673d..3e9059f3 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -460,13 +460,6 @@ calc_mkfs(xfs_mount_t *mp)
 		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
 	}
 
-	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
-
-	if (M_IGEO(mp)->ialloc_blks > 1)
-		last_prealloc_ino = first_prealloc_ino + XFS_INODES_PER_CHUNK;
-	else
-		last_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno + 1);
-
 	/*
 	 * now the first 3 inodes in the system
 	 */

Re: [PATCH 2/6] xfs_repair: enforce that inode btree chunks can't point to AG headers

[Eric Sandeen]

On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_repair has a very old check that evidently excuses the AG 0 inode
> btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
> AG headers).  mkfs never formats filesystems that way and it looks like
> an error, so purge the check.  After this, we always complain if inodes
> overlap with AG headers because that should never happen.

I peered back into the mists of time to see if I could find any reason for
this exception, and I couldn't.

Only question is why you removed the

-	ASSERT(M_IGEO(mp)->ialloc_blks > 0);

assert, that's still a valid assert, no?


> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/globals.c    |    1 -
>  repair/globals.h    |    1 -
>  repair/scan.c       |   19 -------------------
>  repair/xfs_repair.c |    7 -------
>  4 files changed, 28 deletions(-)
> 
> 
> diff --git a/repair/globals.c b/repair/globals.c
> index dcd79ea4..8a60e706 100644
> --- a/repair/globals.c
> +++ b/repair/globals.c
> @@ -73,7 +73,6 @@ int	lost_gquotino;
>  int	lost_pquotino;
>  
>  xfs_agino_t	first_prealloc_ino;
> -xfs_agino_t	last_prealloc_ino;
>  xfs_agblock_t	bnobt_root;
>  xfs_agblock_t	bcntbt_root;
>  xfs_agblock_t	inobt_root;
> diff --git a/repair/globals.h b/repair/globals.h
> index 008bdd90..2ed5c894 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -114,7 +114,6 @@ extern int		lost_gquotino;
>  extern int		lost_pquotino;
>  
>  extern xfs_agino_t	first_prealloc_ino;
> -extern xfs_agino_t	last_prealloc_ino;
>  extern xfs_agblock_t	bnobt_root;
>  extern xfs_agblock_t	bcntbt_root;
>  extern xfs_agblock_t	inobt_root;
> diff --git a/repair/scan.c b/repair/scan.c
> index c383f3aa..05707dd2 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -1645,13 +1645,6 @@ scan_single_ino_chunk(
>  				break;
>  			case XR_E_INUSE_FS:
>  			case XR_E_INUSE_FS1:
> -				if (agno == 0 &&
> -				    ino + j >= first_prealloc_ino &&
> -				    ino + j < last_prealloc_ino) {
> -					set_bmap(agno, agbno, XR_E_INO);
> -					break;
> -				}
> -				/* fall through */
>  			default:
>  				/* XXX - maybe should mark block a duplicate */
>  				do_warn(
> @@ -1782,18 +1775,6 @@ _("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\
>  				break;
>  			case XR_E_INUSE_FS:
>  			case XR_E_INUSE_FS1:
> -				if (agno == 0 &&
> -				    ino + j >= first_prealloc_ino &&
> -				    ino + j < last_prealloc_ino) {
> -					do_warn(
> -_("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\n"),
> -						agno, agbno, mp->m_sb.sb_inopblock);
> -
> -					set_bmap(agno, agbno, XR_E_INO);
> -					suspect++;
> -					break;
> -				}
> -				/* fall through */
>  			default:
>  				do_warn(
>  _("inode chunk claims used block, finobt block - agno %d, bno %d, inopb %d\n"),
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 9295673d..3e9059f3 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -460,13 +460,6 @@ calc_mkfs(xfs_mount_t *mp)
>  		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
>  	}
>  
> -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
> -
> -	if (M_IGEO(mp)->ialloc_blks > 1)
> -		last_prealloc_ino = first_prealloc_ino + XFS_INODES_PER_CHUNK;
> -	else
> -		last_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno + 1);
> -
>  	/*
>  	 * now the first 3 inodes in the system
>  	 */
> 

Re: [PATCH 2/6] xfs_repair: enforce that inode btree chunks can't point to AG headers

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 01:38:52PM -0600, Eric Sandeen wrote:
> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > xfs_repair has a very old check that evidently excuses the AG 0 inode
> > btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
> > AG headers).  mkfs never formats filesystems that way and it looks like
> > an error, so purge the check.  After this, we always complain if inodes
> > overlap with AG headers because that should never happen.
> 
> I peered back into the mists of time to see if I could find any reason for
> this exception, and I couldn't.
> 
> Only question is why you removed the
> 
> -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
> 
> assert, that's still a valid assert, no?

The superblock validation routines are supposed to reject all the
combinations that can result in ialloc_blks being zero.

That said, I can't think of a reason to remove the assert.  If you want
me to put it back, it I can... or pretty-please put it back in for me?
:)

--D

> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/globals.c    |    1 -
> >  repair/globals.h    |    1 -
> >  repair/scan.c       |   19 -------------------
> >  repair/xfs_repair.c |    7 -------
> >  4 files changed, 28 deletions(-)
> > 
> > 
> > diff --git a/repair/globals.c b/repair/globals.c
> > index dcd79ea4..8a60e706 100644
> > --- a/repair/globals.c
> > +++ b/repair/globals.c
> > @@ -73,7 +73,6 @@ int	lost_gquotino;
> >  int	lost_pquotino;
> >  
> >  xfs_agino_t	first_prealloc_ino;
> > -xfs_agino_t	last_prealloc_ino;
> >  xfs_agblock_t	bnobt_root;
> >  xfs_agblock_t	bcntbt_root;
> >  xfs_agblock_t	inobt_root;
> > diff --git a/repair/globals.h b/repair/globals.h
> > index 008bdd90..2ed5c894 100644
> > --- a/repair/globals.h
> > +++ b/repair/globals.h
> > @@ -114,7 +114,6 @@ extern int		lost_gquotino;
> >  extern int		lost_pquotino;
> >  
> >  extern xfs_agino_t	first_prealloc_ino;
> > -extern xfs_agino_t	last_prealloc_ino;
> >  extern xfs_agblock_t	bnobt_root;
> >  extern xfs_agblock_t	bcntbt_root;
> >  extern xfs_agblock_t	inobt_root;
> > diff --git a/repair/scan.c b/repair/scan.c
> > index c383f3aa..05707dd2 100644
> > --- a/repair/scan.c
> > +++ b/repair/scan.c
> > @@ -1645,13 +1645,6 @@ scan_single_ino_chunk(
> >  				break;
> >  			case XR_E_INUSE_FS:
> >  			case XR_E_INUSE_FS1:
> > -				if (agno == 0 &&
> > -				    ino + j >= first_prealloc_ino &&
> > -				    ino + j < last_prealloc_ino) {
> > -					set_bmap(agno, agbno, XR_E_INO);
> > -					break;
> > -				}
> > -				/* fall through */
> >  			default:
> >  				/* XXX - maybe should mark block a duplicate */
> >  				do_warn(
> > @@ -1782,18 +1775,6 @@ _("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\
> >  				break;
> >  			case XR_E_INUSE_FS:
> >  			case XR_E_INUSE_FS1:
> > -				if (agno == 0 &&
> > -				    ino + j >= first_prealloc_ino &&
> > -				    ino + j < last_prealloc_ino) {
> > -					do_warn(
> > -_("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\n"),
> > -						agno, agbno, mp->m_sb.sb_inopblock);
> > -
> > -					set_bmap(agno, agbno, XR_E_INO);
> > -					suspect++;
> > -					break;
> > -				}
> > -				/* fall through */
> >  			default:
> >  				do_warn(
> >  _("inode chunk claims used block, finobt block - agno %d, bno %d, inopb %d\n"),
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 9295673d..3e9059f3 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -460,13 +460,6 @@ calc_mkfs(xfs_mount_t *mp)
> >  		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
> >  	}
> >  
> > -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
> > -
> > -	if (M_IGEO(mp)->ialloc_blks > 1)
> > -		last_prealloc_ino = first_prealloc_ino + XFS_INODES_PER_CHUNK;
> > -	else
> > -		last_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno + 1);
> > -
> >  	/*
> >  	 * now the first 3 inodes in the system
> >  	 */
> > 

Re: [PATCH 2/6] xfs_repair: enforce that inode btree chunks can't point to AG headers

[Eric Sandeen]

On 1/30/20 2:26 PM, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 01:38:52PM -0600, Eric Sandeen wrote:
>> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> xfs_repair has a very old check that evidently excuses the AG 0 inode
>>> btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
>>> AG headers).  mkfs never formats filesystems that way and it looks like
>>> an error, so purge the check.  After this, we always complain if inodes
>>> overlap with AG headers because that should never happen.
>>
>> I peered back into the mists of time to see if I could find any reason for
>> this exception, and I couldn't.
>>
>> Only question is why you removed the
>>
>> -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
>>
>> assert, that's still a valid assert, no?
> 
> The superblock validation routines are supposed to reject all the
> combinations that can result in ialloc_blks being zero.
> 
> That said, I can't think of a reason to remove the assert.  If you want
> me to put it back, it I can... or pretty-please put it back in for me?
> :)


Sure, I'll try to remember to do that with your blessing.

It just didn't seem related to the rest of the patch, is all.  If it's only
there to protect the next calculations, and this is a spot that makes no
sense to test it, I'm ok with dropping it too.  Was just looking for a reason.

-Eric

[PATCH 3/6] xfs_repair: refactor fixed inode location checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the checking and resetting of fixed-location inodes (root,
rbmino, rsumino) into a helper function.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/xfs_repair.c |  106 ++++++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 69 deletions(-)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 3e9059f3..94673750 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -395,6 +395,37 @@ do_log(char const *msg, ...)
 	va_end(args);
 }
 
+/* Make sure a fixed-location inode is where it should be. */
+static void
+ensure_fixed_ino(
+	xfs_ino_t	*ino,
+	xfs_ino_t	expected_ino,
+	const char	*tag)
+{
+	if (*ino == expected_ino)
+		return;
+
+	do_warn(
+_("sb %s inode value %" PRIu64 " %sinconsistent with calculated value %"PRIu64"\n"),
+		tag, *ino, *ino == NULLFSINO ? "(NULLFSINO) " : "",
+		expected_ino);
+
+	if (!no_modify)
+		do_warn(
+_("resetting superblock %s inode pointer to %"PRIu64"\n"),
+			tag, expected_ino);
+	else
+		do_warn(
+_("would reset superblock %s inode pointer to %"PRIu64"\n"),
+			tag, expected_ino);
+
+	/*
+	 * Just set the value -- safe since the superblock doesn't get flushed
+	 * out if no_modify is set.
+	 */
+	*ino = expected_ino;
+}
+
 static void
 calc_mkfs(xfs_mount_t *mp)
 {
@@ -463,75 +494,12 @@ calc_mkfs(xfs_mount_t *mp)
 	/*
 	 * now the first 3 inodes in the system
 	 */
-	if (mp->m_sb.sb_rootino != first_prealloc_ino)  {
-		do_warn(
-_("sb root inode value %" PRIu64 " %sinconsistent with calculated value %u\n"),
-			mp->m_sb.sb_rootino,
-			(mp->m_sb.sb_rootino == NULLFSINO ? "(NULLFSINO) ":""),
-			first_prealloc_ino);
-
-		if (!no_modify)
-			do_warn(
-		_("resetting superblock root inode pointer to %u\n"),
-				first_prealloc_ino);
-		else
-			do_warn(
-		_("would reset superblock root inode pointer to %u\n"),
-				first_prealloc_ino);
-
-		/*
-		 * just set the value -- safe since the superblock
-		 * doesn't get flushed out if no_modify is set
-		 */
-		mp->m_sb.sb_rootino = first_prealloc_ino;
-	}
-
-	if (mp->m_sb.sb_rbmino != first_prealloc_ino + 1)  {
-		do_warn(
-_("sb realtime bitmap inode %" PRIu64 " %sinconsistent with calculated value %u\n"),
-			mp->m_sb.sb_rbmino,
-			(mp->m_sb.sb_rbmino == NULLFSINO ? "(NULLFSINO) ":""),
-			first_prealloc_ino + 1);
-
-		if (!no_modify)
-			do_warn(
-		_("resetting superblock realtime bitmap ino pointer to %u\n"),
-				first_prealloc_ino + 1);
-		else
-			do_warn(
-		_("would reset superblock realtime bitmap ino pointer to %u\n"),
-				first_prealloc_ino + 1);
-
-		/*
-		 * just set the value -- safe since the superblock
-		 * doesn't get flushed out if no_modify is set
-		 */
-		mp->m_sb.sb_rbmino = first_prealloc_ino + 1;
-	}
-
-	if (mp->m_sb.sb_rsumino != first_prealloc_ino + 2)  {
-		do_warn(
-_("sb realtime summary inode %" PRIu64 " %sinconsistent with calculated value %u\n"),
-			mp->m_sb.sb_rsumino,
-			(mp->m_sb.sb_rsumino == NULLFSINO ? "(NULLFSINO) ":""),
-			first_prealloc_ino + 2);
-
-		if (!no_modify)
-			do_warn(
-		_("resetting superblock realtime summary ino pointer to %u\n"),
-				first_prealloc_ino + 2);
-		else
-			do_warn(
-		_("would reset superblock realtime summary ino pointer to %u\n"),
-				first_prealloc_ino + 2);
-
-		/*
-		 * just set the value -- safe since the superblock
-		 * doesn't get flushed out if no_modify is set
-		 */
-		mp->m_sb.sb_rsumino = first_prealloc_ino + 2;
-	}
-
+	ensure_fixed_ino(&mp->m_sb.sb_rootino, first_prealloc_ino,
+			_("root"));
+	ensure_fixed_ino(&mp->m_sb.sb_rbmino, first_prealloc_ino + 1,
+			_("realtime bitmap"));
+	ensure_fixed_ino(&mp->m_sb.sb_rsumino, first_prealloc_ino + 2,
+			_("realtime summary"));
 }
 
 /*

Re: [PATCH 3/6] xfs_repair: refactor fixed inode location checks

[Eric Sandeen]

On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the checking and resetting of fixed-location inodes (root,
> rbmino, rsumino) into a helper function.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good, though it might mess with translations a bit?

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Re: [PATCH 3/6] xfs_repair: refactor fixed inode location checks

[Eric Sandeen]

On 1/30/20 1:45 PM, Eric Sandeen wrote:
> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Refactor the checking and resetting of fixed-location inodes (root,
>> rbmino, rsumino) into a helper function.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Looks good, though it might mess with translations a bit?
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Actually, would you mind if I changed "ensure_fixed_ino" to something
like "check_inode_loc" or "validate_inode_nr" ... I don't know what
it means to "ensure" an inode.  :)

Not a huge deal but if there's something more descriptive, let's use
it.

-Eric

[PATCH 4/6] xfs_repair: use libxfs function to calculate root inode location

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Use libxfs_ialloc_calc_rootino to compute the location of the root
inode, and improve the function comments while we're at it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/globals.c    |    5 ---
 repair/globals.h    |    5 ---
 repair/xfs_repair.c |   78 +++++++--------------------------------------------
 3 files changed, 11 insertions(+), 77 deletions(-)


diff --git a/repair/globals.c b/repair/globals.c
index 8a60e706..299bacd1 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -72,11 +72,6 @@ int	lost_uquotino;
 int	lost_gquotino;
 int	lost_pquotino;
 
-xfs_agino_t	first_prealloc_ino;
-xfs_agblock_t	bnobt_root;
-xfs_agblock_t	bcntbt_root;
-xfs_agblock_t	inobt_root;
-
 /* configuration vars -- fs geometry dependent */
 
 int		inodes_per_block;
diff --git a/repair/globals.h b/repair/globals.h
index 2ed5c894..953e3dfb 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -113,11 +113,6 @@ extern int		lost_uquotino;
 extern int		lost_gquotino;
 extern int		lost_pquotino;
 
-extern xfs_agino_t	first_prealloc_ino;
-extern xfs_agblock_t	bnobt_root;
-extern xfs_agblock_t	bcntbt_root;
-extern xfs_agblock_t	inobt_root;
-
 /* configuration vars -- fs geometry dependent */
 
 extern int		inodes_per_block;
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 94673750..53b04dae 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -426,79 +426,23 @@ _("would reset superblock %s inode pointer to %"PRIu64"\n"),
 	*ino = expected_ino;
 }
 
+/*
+ * Make sure that the first 3 inodes in the filesystem are the root directory,
+ * the realtime bitmap, and the realtime summary, in that order.
+ */
 static void
-calc_mkfs(xfs_mount_t *mp)
+calc_mkfs(
+	struct xfs_mount	*mp)
 {
-	xfs_agblock_t	fino_bno;
-	int		do_inoalign;
-
-	do_inoalign = M_IGEO(mp)->ialloc_align;
-
-	/*
-	 * Pre-calculate the geometry of ag 0. We know what it looks like
-	 * because we know what mkfs does: 2 allocation btree roots (by block
-	 * and by size), the inode allocation btree root, the free inode
-	 * allocation btree root (if enabled) and some number of blocks to
-	 * prefill the agfl.
-	 *
-	 * Because the current shape of the btrees may differ from the current
-	 * shape, we open code the mkfs freelist block count here. mkfs creates
-	 * single level trees, so the calculation is pertty straight forward for
-	 * the trees that use the AGFL.
-	 */
-	bnobt_root = howmany(4 * mp->m_sb.sb_sectsize, mp->m_sb.sb_blocksize);
-	bcntbt_root = bnobt_root + 1;
-	inobt_root = bnobt_root + 2;
-	fino_bno = inobt_root + (2 * min(2, mp->m_ag_maxlevels)) + 1;
-	if (xfs_sb_version_hasfinobt(&mp->m_sb))
-		fino_bno++;
-	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
-		fino_bno += min(2, mp->m_rmap_maxlevels); /* agfl blocks */
-		fino_bno++;
-	}
-	if (xfs_sb_version_hasreflink(&mp->m_sb))
-		fino_bno++;
+	xfs_ino_t		rootino;
 
-	/*
-	 * If the log is allocated in the first allocation group we need to
-	 * add the number of blocks used by the log to the above calculation.
-	 *
-	 * This can happens with filesystems that only have a single
-	 * allocation group, or very odd geometries created by old mkfs
-	 * versions on very small filesystems.
-	 */
-	if (mp->m_sb.sb_logstart &&
-	    XFS_FSB_TO_AGNO(mp, mp->m_sb.sb_logstart) == 0) {
+	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
 
-		/*
-		 * XXX(hch): verify that sb_logstart makes sense?
-		 */
-		 fino_bno += mp->m_sb.sb_logblocks;
-	}
-
-	/*
-	 * ditto the location of the first inode chunks in the fs ('/')
-	 */
-	if (xfs_sb_version_hasdalign(&mp->m_sb) && do_inoalign)  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, roundup(fino_bno,
-					mp->m_sb.sb_unit));
-	} else if (xfs_sb_version_hasalign(&mp->m_sb) &&
-					mp->m_sb.sb_inoalignmt > 1)  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp,
-					roundup(fino_bno,
-						mp->m_sb.sb_inoalignmt));
-	} else  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
-	}
-
-	/*
-	 * now the first 3 inodes in the system
-	 */
-	ensure_fixed_ino(&mp->m_sb.sb_rootino, first_prealloc_ino,
+	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
 			_("root"));
-	ensure_fixed_ino(&mp->m_sb.sb_rbmino, first_prealloc_ino + 1,
+	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
 			_("realtime bitmap"));
-	ensure_fixed_ino(&mp->m_sb.sb_rsumino, first_prealloc_ino + 2,
+	ensure_fixed_ino(&mp->m_sb.sb_rsumino, rootino + 2,
 			_("realtime summary"));
 }
 

Re: [PATCH 4/6] xfs_repair: use libxfs function to calculate root inode location

[Eric Sandeen]

On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use libxfs_ialloc_calc_rootino to compute the location of the root
> inode, and improve the function comments while we're at it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

[PATCH 5/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If sb_rootino doesn't point to where we think mkfs should have allocated
the root directory, check to see if the alleged root directory actually
looks like a root directory.  If so, we'll let it live because someone
could have changed sunit since formatting time, and that changes the
root directory inode estimate.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 53b04dae..372616c4 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -426,6 +426,37 @@ _("would reset superblock %s inode pointer to %"PRIu64"\n"),
 	*ino = expected_ino;
 }
 
+/* Does the root directory inode look like a plausible root directory? */
+static bool
+has_plausible_rootdir(
+	struct xfs_mount	*mp)
+{
+	struct xfs_inode	*ip;
+	xfs_ino_t		ino;
+	int			error;
+	bool			ret = false;
+
+	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
+			&xfs_default_ifork_ops);
+	if (error)
+		goto out;
+	if (!S_ISDIR(VFS_I(ip)->i_mode))
+		goto out_rele;
+
+	error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL);
+	if (error)
+		goto out_rele;
+
+	/* The root directory '..' entry points to the directory. */
+	if (ino == mp->m_sb.sb_rootino)
+		ret = true;
+
+out_rele:
+	libxfs_irele(ip);
+out:
+	return ret;
+}
+
 /*
  * Make sure that the first 3 inodes in the filesystem are the root directory,
  * the realtime bitmap, and the realtime summary, in that order.
@@ -438,6 +469,20 @@ calc_mkfs(
 
 	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
 
+	/*
+	 * If the root inode isn't where we think it is, check its plausibility
+	 * as a root directory.  It's possible that somebody changed sunit
+	 * since the filesystem was created, which can change the value of the
+	 * above computation.  Don't blow up the root directory if this is the
+	 * case.
+	 */
+	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
+		do_warn(
+_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
+			mp->m_sb.sb_rootino, rootino);
+		rootino = mp->m_sb.sb_rootino;
+	}
+
 	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
 			_("root"));
 	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,

Re: [PATCH 5/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Eric Sandeen]

On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If sb_rootino doesn't point to where we think mkfs should have allocated
> the root directory, check to see if the alleged root directory actually
> looks like a root directory.  If so, we'll let it live because someone
> could have changed sunit since formatting time, and that changes the
> root directory inode estimate.

I forget, is there an fstest for this?

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

...

> @@ -438,6 +469,20 @@ calc_mkfs(
>  
>  	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
>  
> +	/*
> +	 * If the root inode isn't where we think it is, check its plausibility
> +	 * as a root directory.  It's possible that somebody changed sunit
> +	 * since the filesystem was created, which can change the value of the
> +	 * above computation.  Don't blow up the root directory if this is the
> +	 * case.
> +	 */
> +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
> +		do_warn(
> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
> +			mp->m_sb.sb_rootino, rootino);

what would a user do with this warning?  Is there any value in emitting it?

Otherwise this looks good.


> +		rootino = mp->m_sb.sb_rootino;
> +	}
> +
>  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
>  			_("root"));
>  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
> 

Re: [PATCH 5/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If sb_rootino doesn't point to where we think mkfs should have allocated
> > the root directory, check to see if the alleged root directory actually
> > looks like a root directory.  If so, we'll let it live because someone
> > could have changed sunit since formatting time, and that changes the
> > root directory inode estimate.
> 
> I forget, is there an fstest for this?

https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> ...
> 
> > @@ -438,6 +469,20 @@ calc_mkfs(
> >  
> >  	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
> >  
> > +	/*
> > +	 * If the root inode isn't where we think it is, check its plausibility
> > +	 * as a root directory.  It's possible that somebody changed sunit
> > +	 * since the filesystem was created, which can change the value of the
> > +	 * above computation.  Don't blow up the root directory if this is the
> > +	 * case.
> > +	 */
> > +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
> > +		do_warn(
> > +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
> > +			mp->m_sb.sb_rootino, rootino);
> 
> what would a user do with this warning?  Is there any value in emitting it?
> 
> Otherwise this looks good.

I dunno -- on the one hand, I understand that nobody wants to deal with
the support calls that will erupt from that message.  On the other hand,
it's an indication that this filesystem isn't /quite/ the way we
expected it to be, and that would be a helpful hint if you were
debugging some other weird problem with an xfs filesystem.

What if this were a do_log()?

--D
> 
> 
> > +		rootino = mp->m_sb.sb_rootino;
> > +	}
> > +
> >  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
> >  			_("root"));
> >  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
> > 

Re: [PATCH 5/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Eric Sandeen]

On 1/30/20 2:34 PM, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
>> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> If sb_rootino doesn't point to where we think mkfs should have allocated
>>> the root directory, check to see if the alleged root directory actually
>>> looks like a root directory.  If so, we'll let it live because someone
>>> could have changed sunit since formatting time, and that changes the
>>> root directory inode estimate.
>>
>> I forget, is there an fstest for this?
> 
> https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/

of course :)

...

>>> +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
>>> +		do_warn(
>>> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
>>> +			mp->m_sb.sb_rootino, rootino);
>>
>> what would a user do with this warning?  Is there any value in emitting it?
>>
>> Otherwise this looks good.
> 
> I dunno -- on the one hand, I understand that nobody wants to deal with
> the support calls that will erupt from that message.  On the other hand,
> it's an indication that this filesystem isn't /quite/ the way we
> expected it to be, and that would be a helpful hint if you were
> debugging some other weird problem with an xfs filesystem.
> 
> What if this were a do_log()?

how about something that's less indicative of a problem and more informational,

"sb root inode validated in unaligned location possibly due to sunit change"

-Eric

Re: [PATCH 5/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 02:41:29PM -0600, Eric Sandeen wrote:
> On 1/30/20 2:34 PM, Darrick J. Wong wrote:
> > On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
> >> On 1/23/20 6:17 PM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>>
> >>> If sb_rootino doesn't point to where we think mkfs should have allocated
> >>> the root directory, check to see if the alleged root directory actually
> >>> looks like a root directory.  If so, we'll let it live because someone
> >>> could have changed sunit since formatting time, and that changes the
> >>> root directory inode estimate.
> >>
> >> I forget, is there an fstest for this?
> > 
> > https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/
> 
> of course :)

:D

> ...
> 
> >>> +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
> >>> +		do_warn(
> >>> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
> >>> +			mp->m_sb.sb_rootino, rootino);
> >>
> >> what would a user do with this warning?  Is there any value in emitting it?
> >>
> >> Otherwise this looks good.
> > 
> > I dunno -- on the one hand, I understand that nobody wants to deal with
> > the support calls that will erupt from that message.  On the other hand,
> > it's an indication that this filesystem isn't /quite/ the way we
> > expected it to be, and that would be a helpful hint if you were
> > debugging some other weird problem with an xfs filesystem.
> > 
> > What if this were a do_log()?
> 
> how about something that's less indicative of a problem and more informational,
> 
> "sb root inode validated in unaligned location possibly due to sunit change"

Sounds good to me.

--D

> -Eric

[PATCH 6/6] xfs_repair: try to correct sb_unit value from secondaries

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If the primary superblock's sb_unit leads to a rootino calculation that
doesn't match sb_rootino /but/ we can find a secondary superblock whose
sb_unit does match, fix the primary.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/xfs_repair.c      |   79 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 9ede0125..d9f497e0 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -178,5 +178,6 @@
 #define xfs_finobt_calc_reserves	libxfs_finobt_calc_reserves
 #define xfs_rmapbt_calc_reserves	libxfs_rmapbt_calc_reserves
 #define xfs_read_agf			libxfs_read_agf
+#define xfs_sb_read_secondary		libxfs_sb_read_secondary
 
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 372616c4..66e2c335 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -457,6 +457,84 @@ has_plausible_rootdir(
 	return ret;
 }
 
+/*
+ * If any of the secondary SBs contain a *correct* value for sunit, write that
+ * back to the primary superblock.
+ */
+static void
+guess_correct_sunit(
+	struct xfs_mount	*mp)
+{
+	struct xfs_sb		sb;
+	struct xfs_buf		*bp;
+	xfs_ino_t		calc_rootino = NULLFSINO;
+	xfs_agnumber_t		agno;
+	unsigned int		new_sunit;
+	unsigned int		sunit_guess;
+	int			error;
+
+	/* Try reading secondary supers to see if we find a good sb_unit. */
+	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
+		error = -libxfs_sb_read_secondary(mp, NULL, agno, &bp);
+		if (error)
+			continue;
+		libxfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp));
+		libxfs_putbuf(bp);
+
+		calc_rootino = libxfs_ialloc_calc_rootino(mp, sb.sb_unit);
+		if (calc_rootino == mp->m_sb.sb_rootino)
+			break;
+	}
+
+	/* If we found a reasonable value, log where we found it. */
+	if (calc_rootino == mp->m_sb.sb_rootino) {
+		do_warn(_("AG %u superblock contains plausible sb_unit value\n"),
+				agno);
+		new_sunit = sb.sb_unit;
+		goto fix;
+	}
+
+	/* Try successive powers of two. */
+	for (sunit_guess = 1;
+	     sunit_guess <= XFS_AG_MAX_BLOCKS(mp->m_sb.sb_blocklog);
+	     sunit_guess *= 2) {
+		calc_rootino = libxfs_ialloc_calc_rootino(mp, sunit_guess);
+		if (calc_rootino == mp->m_sb.sb_rootino)
+			break;
+	}
+
+	/* If we found a reasonable value, log where we found it. */
+	if (calc_rootino == mp->m_sb.sb_rootino) {
+		do_warn(_("Found an sb_unit value that looks plausible\n"));
+		new_sunit = sunit_guess;
+		goto fix;
+	}
+
+	do_warn(_("Could not estimate a plausible sb_unit value\n"));
+	return;
+
+fix:
+	if (!no_modify)
+		do_warn(_("Resetting sb_unit to %u\n"), new_sunit);
+	else
+		do_warn(_("Would reset sb_unit to %u\n"), new_sunit);
+
+	/*
+	 * Just set the value -- safe since the superblock doesn't get flushed
+	 * out if no_modify is set.
+	 */
+	mp->m_sb.sb_unit = new_sunit;
+
+	/* Make sure that swidth is still a multiple of sunit. */
+	if (mp->m_sb.sb_width % mp->m_sb.sb_unit == 0)
+		return;
+
+	if (!no_modify)
+		do_warn(_("Resetting sb_width to %u\n"), new_sunit);
+	else
+		do_warn(_("Would reset sb_width to %u\n"), new_sunit);
+}
+
 /*
  * Make sure that the first 3 inodes in the filesystem are the root directory,
  * the realtime bitmap, and the realtime summary, in that order.
@@ -480,6 +558,7 @@ calc_mkfs(
 		do_warn(
 _("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
 			mp->m_sb.sb_rootino, rootino);
+		guess_correct_sunit(mp);
 		rootino = mp->m_sb.sb_rootino;
 	}
 

[PATCH v2 0/6] xfs_repair: do not trash valid root dirs

[Darrick J. Wong]

Hi all,

Alex Lyakas observed that xfs_repair can accidentally trash the root
directory on a filesystem.  Specifically, if one formats a V4 filesystem
without sparse inodes but with sunit/swidth set and then mounts that
filesystem with a different sunit/swidth, the kernel will update the
values in the superblock.  This causes xfs_repair's sb_rootino
estimation to differ from the actual root directory, and it discards the
actual root directory even though there's nothing wrong with it.

Therefore, hoist the logic that computes the root inode location into
libxfs so that the kernel will avoid the sb update if the proposed
sunit/swidth changes would alter the sb_rootino estimation; and then
teach xfs_repair to retain the root directory even if the estimation
doesn't add up, as long as sb_rootino points to a directory whose '..'
entry points to itself.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-find-rootdir

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=repair-find-rootdir

[PATCH 2/6] xfs_repair: enforce that inode btree chunks can't point to AG headers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

xfs_repair has a very old check that evidently excuses the AG 0 inode
btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
AG headers).  mkfs never formats filesystems that way and it looks like
an error, so purge the check.  After this, we always complain if inodes
overlap with AG headers because that should never happen.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/globals.c    |    1 -
 repair/globals.h    |    1 -
 repair/scan.c       |   19 -------------------
 repair/xfs_repair.c |    7 -------
 4 files changed, 28 deletions(-)


diff --git a/repair/globals.c b/repair/globals.c
index dcd79ea4..8a60e706 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -73,7 +73,6 @@ int	lost_gquotino;
 int	lost_pquotino;
 
 xfs_agino_t	first_prealloc_ino;
-xfs_agino_t	last_prealloc_ino;
 xfs_agblock_t	bnobt_root;
 xfs_agblock_t	bcntbt_root;
 xfs_agblock_t	inobt_root;
diff --git a/repair/globals.h b/repair/globals.h
index 008bdd90..2ed5c894 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -114,7 +114,6 @@ extern int		lost_gquotino;
 extern int		lost_pquotino;
 
 extern xfs_agino_t	first_prealloc_ino;
-extern xfs_agino_t	last_prealloc_ino;
 extern xfs_agblock_t	bnobt_root;
 extern xfs_agblock_t	bcntbt_root;
 extern xfs_agblock_t	inobt_root;
diff --git a/repair/scan.c b/repair/scan.c
index c383f3aa..05707dd2 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1645,13 +1645,6 @@ scan_single_ino_chunk(
 				break;
 			case XR_E_INUSE_FS:
 			case XR_E_INUSE_FS1:
-				if (agno == 0 &&
-				    ino + j >= first_prealloc_ino &&
-				    ino + j < last_prealloc_ino) {
-					set_bmap(agno, agbno, XR_E_INO);
-					break;
-				}
-				/* fall through */
 			default:
 				/* XXX - maybe should mark block a duplicate */
 				do_warn(
@@ -1782,18 +1775,6 @@ _("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\
 				break;
 			case XR_E_INUSE_FS:
 			case XR_E_INUSE_FS1:
-				if (agno == 0 &&
-				    ino + j >= first_prealloc_ino &&
-				    ino + j < last_prealloc_ino) {
-					do_warn(
-_("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\n"),
-						agno, agbno, mp->m_sb.sb_inopblock);
-
-					set_bmap(agno, agbno, XR_E_INO);
-					suspect++;
-					break;
-				}
-				/* fall through */
 			default:
 				do_warn(
 _("inode chunk claims used block, finobt block - agno %d, bno %d, inopb %d\n"),
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9295673d..3e9059f3 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -460,13 +460,6 @@ calc_mkfs(xfs_mount_t *mp)
 		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
 	}
 
-	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
-
-	if (M_IGEO(mp)->ialloc_blks > 1)
-		last_prealloc_ino = first_prealloc_ino + XFS_INODES_PER_CHUNK;
-	else
-		last_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno + 1);
-
 	/*
 	 * now the first 3 inodes in the system
 	 */