Message ID | 158086361666.2079685.8451949513769071894.stgit@magnolia (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | xfs_repair: do not trash valid root dirs | expand |
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>
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
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
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
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
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
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 --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 */