diff mbox series

[3/7] xfs_repair: enforce that inode btree chunks can't point to AG headers

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

Commit Message

Darrick J. Wong Feb. 5, 2020, 12:46 a.m. UTC 
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(-)

Comments

Christoph Hellwig Feb. 17, 2020, 1:51 p.m. UTC | #1 
On Tue, Feb 04, 2020 at 04:46:56PM -0800, 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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Eric Sandeen Feb. 26, 2020, 5:09 p.m. UTC | #2 
On 2/4/20 4:46 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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I know it's hard to keep track, but it'd be nice if

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

this line had been kept per the feedback on the last patchset...

This also lost my feedback the first time, re:

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

"I guess there's no real reason to list a couple cases that all fall through
to default:, I'd just remove them as well since they aren't any more special
than the other unmentioned cases."

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

I guess I should stop saying "I'll do that on the way in" if 2 more versions are going
to hit the list, maybe it takes the feedback off your radar.

-Eric
Eric Sandeen Feb. 26, 2020, 5:19 p.m. UTC | #3 
On 2/4/20 4:46 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.

On a previous version, you and Brian had a fairly long conversation about
the warning this presents, and how it doesn't tell the user what to do
about it, and how the warning will persist, and may generate bug reports
or questions.

It sounded like you had a plan to address that, which does not seem to be
present in this patch? So I'm not sure Brian's concerns have been resolved
yet.

-Eric
Darrick J. Wong Feb. 26, 2020, 5:24 p.m. UTC | #4 
On Wed, Feb 26, 2020 at 09:09:42AM -0800, Eric Sandeen wrote:
> On 2/4/20 4:46 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.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I know it's hard to keep track, but it'd be nice if
> 
> > -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
> 
> this line had been kept per the feedback on the last patchset...

I've added that back.  For real this time.

> This also lost my feedback the first time, re:
> 
> @@ -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:
> 
> "I guess there's no real reason to list a couple cases that all fall through
> to default:, I'd just remove them as well since they aren't any more special
> than the other unmentioned cases."
> 
> -				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:
> 
> I guess I should stop saying "I'll do that on the way in" if 2 more
> versions are going to hit the list, maybe it takes the feedback off
> your radar.

I (almost) always make the changes to my local tree even if you say
you'll do it on the way in, because that makes it easier to compare the
for-next tree vs. my about-to-be-rebased dev tree.

Unfortunately, I do occasionally slip up and forget to make the changes,
even if I've sent email assenting to the changes, because there's not
anything linking "I will make this change" in the email thread to
actually scribbling in the git tree.

Add to that the fact that email clients don't maintain spatial locality
between v3->v4->v5 of a patchset and that just makes it more difficult
to stay on top of reviews as a developer, because I can't even
self-check without having to scroll through hundreds of emails.

So yeah, I guess I'll go review my reviews...  sorry for the crap.

--D

> -Eric
Darrick J. Wong Feb. 26, 2020, 5:32 p.m. UTC | #5 
On Wed, Feb 26, 2020 at 09:19:53AM -0800, Eric Sandeen wrote:
> On 2/4/20 4:46 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.
> 
> On a previous version, you and Brian had a fairly long conversation about
> the warning this presents, and how it doesn't tell the user what to do
> about it, and how the warning will persist, and may generate bug reports
> or questions.
> 
> It sounded like you had a plan to address that, which does not seem to be
> present in this patch? So I'm not sure Brian's concerns have been resolved
> yet.

I'm confused about "the warning this presents" -- are you talking about
this patch specifically, where we couldn't figure out the weird masking
behavior that dated back to 2001 and the hysterical raisins?

Or are you referring to Brian's criticism of earlier versions of this
series that would whine about our root inode computation not leading to
the root inode without actually telling the user what to do about it?

If it's the second, then I the answer is that I added another patch
("xfs_repair: try to correct sb_unit value from secondaries") to try to
recover a working sunit value from the backup superblocks, or try some
power of two guesses to see if we find one that matches, and then reset
the value to something that will make the computation work again.

--D

> 
> -Eric
Eric Sandeen Feb. 26, 2020, 5:34 p.m. UTC | #6 
On 2/26/20 9:24 AM, Darrick J. Wong wrote:
> On Wed, Feb 26, 2020 at 09:09:42AM -0800, Eric Sandeen wrote:

...

>> I guess I should stop saying "I'll do that on the way in" if 2 more
>> versions are going to hit the list, maybe it takes the feedback off
>> your radar.
> 
> I (almost) always make the changes to my local tree even if you say
> you'll do it on the way in, because that makes it easier to compare the
> for-next tree vs. my about-to-be-rebased dev tree.
> 
> Unfortunately, I do occasionally slip up and forget to make the changes,
> even if I've sent email assenting to the changes, because there's not
> anything linking "I will make this change" in the email thread to
> actually scribbling in the git tree.
> 
> Add to that the fact that email clients don't maintain spatial locality
> between v3->v4->v5 of a patchset and that just makes it more difficult
> to stay on top of reviews as a developer, because I can't even
> self-check without having to scroll through hundreds of emails.
> 
> So yeah, I guess I'll go review my reviews...  sorry for the crap.

No problem.  We're all in this together. ;)

Thanks,
-Eric

> --D
Eric Sandeen Feb. 26, 2020, 5:40 p.m. UTC | #7 
On 2/26/20 9:32 AM, Darrick J. Wong wrote:
> On Wed, Feb 26, 2020 at 09:19:53AM -0800, Eric Sandeen wrote:
>> On 2/4/20 4:46 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.
>>
>> On a previous version, you and Brian had a fairly long conversation about
>> the warning this presents, and how it doesn't tell the user what to do
>> about it, and how the warning will persist, and may generate bug reports
>> or questions.
>>
>> It sounded like you had a plan to address that, which does not seem to be
>> present in this patch? So I'm not sure Brian's concerns have been resolved
>> yet.
> 
> I'm confused about "the warning this presents" -- are you talking about
> this patch specifically, where we couldn't figure out the weird masking
> behavior that dated back to 2001 and the hysterical raisins?

nah I made my peace with that ;)
 
> Or are you referring to Brian's criticism of earlier versions of this
> series that would whine about our root inode computation not leading to
> the root inode without actually telling the user what to do about it?

Good grief I sent this reply on the wrong patch, the discussion was on
"check plausibility of root dir pointer before trashing it"

me--

> If it's the second, then I the answer is that I added another patch
> ("xfs_repair: try to correct sb_unit value from secondaries") to try to
> recover a working sunit value from the backup superblocks, or try some
> power of two guesses to see if we find one that matches, and then reset
> the value to something that will make the computation work again.

Ah ok, got it.  Let me go ask a question in reply to that.

Sorry for the confusion.

Thanks,
-Eric
diff mbox series

Patch

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