| Message ID | 161284385516.3057868.355176047687079022.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: add the ability to flag a fs for repair | expand |
On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Simulate a crash when anyone calls force_needsrepair. This is a debug > knob so that we can test that the kernel won't mount after setting > needsrepair and that a re-run of xfs_repair will clear the flag. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Simulate a crash when anyone calls force_needsrepair. This is a debug > knob so that we can test that the kernel won't mount after setting > needsrepair and that a re-run of xfs_repair will clear the flag. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- Can't we just use db to manually set the bit on the superblock? Brian > repair/globals.c | 1 + > repair/globals.h | 2 ++ > repair/phase1.c | 5 +++++ > repair/xfs_repair.c | 7 +++++++ > 4 files changed, 15 insertions(+) > > > diff --git a/repair/globals.c b/repair/globals.c > index 699a96ee..b0e23864 100644 > --- a/repair/globals.c > +++ b/repair/globals.c > @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ > int isa_file; > int zap_log; > int dumpcore; /* abort, not exit on fatal errs */ > +bool abort_after_force_needsrepair; > int force_geo; /* can set geo on low confidence info */ > int assume_xfs; /* assume we have an xfs fs */ > char *log_name; /* Name of log device */ > diff --git a/repair/globals.h b/repair/globals.h > index 043b3e8e..9fa73b2c 100644 > --- a/repair/globals.h > +++ b/repair/globals.h > @@ -82,6 +82,8 @@ extern int isa_file; > extern int zap_log; > extern int dumpcore; /* abort, not exit on fatal errs */ > extern int force_geo; /* can set geo on low confidence info */ > +/* Abort after forcing NEEDSREPAIR to test its functionality */ > +extern bool abort_after_force_needsrepair; > extern int assume_xfs; /* assume we have an xfs fs */ > extern char *log_name; /* Name of log device */ > extern int log_spec; /* Log dev specified as option */ > diff --git a/repair/phase1.c b/repair/phase1.c > index b26d25f8..57f72cd0 100644 > --- a/repair/phase1.c > +++ b/repair/phase1.c > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); > */ > sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; > > + /* Simulate a crash after setting needsrepair. */ > + if (primary_sb_modified && add_needsrepair && > + abort_after_force_needsrepair) > + exit(55); > + > free(sb); > } > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > index ee377e8a..ae7106a6 100644 > --- a/repair/xfs_repair.c > +++ b/repair/xfs_repair.c > @@ -44,6 +44,7 @@ enum o_opt_nums { > BLOAD_LEAF_SLACK, > BLOAD_NODE_SLACK, > NOQUOTA, > + FORCE_NEEDSREPAIR_ABORT, > O_MAX_OPTS, > }; > > @@ -57,6 +58,7 @@ static char *o_opts[] = { > [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", > [BLOAD_NODE_SLACK] = "debug_bload_node_slack", > [NOQUOTA] = "noquota", > + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", > [O_MAX_OPTS] = NULL, > }; > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv) > _("-o debug_bload_node_slack requires a parameter\n")); > bload_node_slack = (int)strtol(val, NULL, 0); > break; > + case FORCE_NEEDSREPAIR_ABORT: > + abort_after_force_needsrepair = true; > + break; > case NOQUOTA: > quotacheck_skip(); > break; > @@ -795,6 +800,8 @@ force_needsrepair( > error = -libxfs_bwrite(bp); > if (error) > do_log(_("couldn't force needsrepair, err=%d\n"), error); > + if (abort_after_force_needsrepair) > + exit(55); > } > if (bp) > libxfs_buf_relse(bp); >
On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote: > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Simulate a crash when anyone calls force_needsrepair. This is a debug > > knob so that we can test that the kernel won't mount after setting > > needsrepair and that a re-run of xfs_repair will clear the flag. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > Can't we just use db to manually set the bit on the superblock? No, because the fstest uses this debug knob to simulate the following: 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1' 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR 3) system goes down and repair never completes 4) verify that we can't mount 5) verify that repair clears NEEDSREPAIR and gives us a clean fs 6) verify that mount works again and the other scenario is: 1) fuzz a directory entry in such a way that repair will decide to blow out the dirent and rebuild the directory later 2) sysadmin issues 'xfs_repair /dev/sda1' 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the dirent to trigger the rebuild later 3) system goes down and repair never completes 4) verify that we can't mount 5) verify that repair clears NEEDSREPAIR and gives us a clean fs 6) verify that mount works again Both cases reflect what I think are the most likely failure scenarios, hence the knob needs to be in xfs_repair to prevent it from running to completion. (And yes, I've been recently very bad at sending fstests out for review the past few months; I will get that done by this afternoon.) --D > Brian > > > repair/globals.c | 1 + > > repair/globals.h | 2 ++ > > repair/phase1.c | 5 +++++ > > repair/xfs_repair.c | 7 +++++++ > > 4 files changed, 15 insertions(+) > > > > > > diff --git a/repair/globals.c b/repair/globals.c > > index 699a96ee..b0e23864 100644 > > --- a/repair/globals.c > > +++ b/repair/globals.c > > @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ > > int isa_file; > > int zap_log; > > int dumpcore; /* abort, not exit on fatal errs */ > > +bool abort_after_force_needsrepair; > > int force_geo; /* can set geo on low confidence info */ > > int assume_xfs; /* assume we have an xfs fs */ > > char *log_name; /* Name of log device */ > > diff --git a/repair/globals.h b/repair/globals.h > > index 043b3e8e..9fa73b2c 100644 > > --- a/repair/globals.h > > +++ b/repair/globals.h > > @@ -82,6 +82,8 @@ extern int isa_file; > > extern int zap_log; > > extern int dumpcore; /* abort, not exit on fatal errs */ > > extern int force_geo; /* can set geo on low confidence info */ > > +/* Abort after forcing NEEDSREPAIR to test its functionality */ > > +extern bool abort_after_force_needsrepair; > > extern int assume_xfs; /* assume we have an xfs fs */ > > extern char *log_name; /* Name of log device */ > > extern int log_spec; /* Log dev specified as option */ > > diff --git a/repair/phase1.c b/repair/phase1.c > > index b26d25f8..57f72cd0 100644 > > --- a/repair/phase1.c > > +++ b/repair/phase1.c > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); > > */ > > sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; > > > > + /* Simulate a crash after setting needsrepair. */ > > + if (primary_sb_modified && add_needsrepair && > > + abort_after_force_needsrepair) > > + exit(55); > > + > > free(sb); > > } > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > index ee377e8a..ae7106a6 100644 > > --- a/repair/xfs_repair.c > > +++ b/repair/xfs_repair.c > > @@ -44,6 +44,7 @@ enum o_opt_nums { > > BLOAD_LEAF_SLACK, > > BLOAD_NODE_SLACK, > > NOQUOTA, > > + FORCE_NEEDSREPAIR_ABORT, > > O_MAX_OPTS, > > }; > > > > @@ -57,6 +58,7 @@ static char *o_opts[] = { > > [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", > > [BLOAD_NODE_SLACK] = "debug_bload_node_slack", > > [NOQUOTA] = "noquota", > > + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", > > [O_MAX_OPTS] = NULL, > > }; > > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv) > > _("-o debug_bload_node_slack requires a parameter\n")); > > bload_node_slack = (int)strtol(val, NULL, 0); > > break; > > + case FORCE_NEEDSREPAIR_ABORT: > > + abort_after_force_needsrepair = true; > > + break; > > case NOQUOTA: > > quotacheck_skip(); > > break; > > @@ -795,6 +800,8 @@ force_needsrepair( > > error = -libxfs_bwrite(bp); > > if (error) > > do_log(_("couldn't force needsrepair, err=%d\n"), error); > > + if (abort_after_force_needsrepair) > > + exit(55); > > } > > if (bp) > > libxfs_buf_relse(bp); > > >
On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote: > On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote: > > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > Simulate a crash when anyone calls force_needsrepair. This is a debug > > > knob so that we can test that the kernel won't mount after setting > > > needsrepair and that a re-run of xfs_repair will clear the flag. > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > > Can't we just use db to manually set the bit on the superblock? > > No, because the fstest uses this debug knob to simulate the following: > > 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1' > 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR > 3) system goes down and repair never completes > 4) verify that we can't mount > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > 6) verify that mount works again > Ok, but that seems like circular reasoning. It wouldn't be quite the same as a simulated repair failure, but ISTM that if we set the bit manually, we can still verify steps 4, 5 and 6 as is (with the caveat that the repair invocation performs a feature upgrade). I'm not sure how important it really is to verify that a feature upgrade sequence sets the bit if it happens to fail provided we have independent tests that 1. verify the needsrepair bit works as expected and 2. verify the feature upgrades work appropriately, since that is the primary functionality. I wanted to think about that a little more before replying, but I also just realized something odd when digging into the debug code: # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch Phase 1 - find and verify superblock... Marking filesystem in need of repair. writing modified primary superblock Phase 2 - using internal log - zero log... ERROR: The filesystem has valuable metadata changes in a log which needs to ... # mount /dev/test/scratch /mnt/ mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error. # It looks like we can set a feature upgrade bit on the superblock before we've examined the log and potentially discovered that it's dirty (phase 2). If the log is recoverable, that puts the user in a bit of a bind.. Brian > and the other scenario is: > > 1) fuzz a directory entry in such a way that repair will decide to > blow out the dirent and rebuild the directory later > 2) sysadmin issues 'xfs_repair /dev/sda1' > 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the > dirent to trigger the rebuild later > 3) system goes down and repair never completes > 4) verify that we can't mount > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > 6) verify that mount works again > > Both cases reflect what I think are the most likely failure scenarios, > hence the knob needs to be in xfs_repair to prevent it from running to > completion. > > (And yes, I've been recently very bad at sending fstests out for review > the past few months; I will get that done by this afternoon.) > > --D > > > Brian > > > > > repair/globals.c | 1 + > > > repair/globals.h | 2 ++ > > > repair/phase1.c | 5 +++++ > > > repair/xfs_repair.c | 7 +++++++ > > > 4 files changed, 15 insertions(+) > > > > > > > > > diff --git a/repair/globals.c b/repair/globals.c > > > index 699a96ee..b0e23864 100644 > > > --- a/repair/globals.c > > > +++ b/repair/globals.c > > > @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ > > > int isa_file; > > > int zap_log; > > > int dumpcore; /* abort, not exit on fatal errs */ > > > +bool abort_after_force_needsrepair; > > > int force_geo; /* can set geo on low confidence info */ > > > int assume_xfs; /* assume we have an xfs fs */ > > > char *log_name; /* Name of log device */ > > > diff --git a/repair/globals.h b/repair/globals.h > > > index 043b3e8e..9fa73b2c 100644 > > > --- a/repair/globals.h > > > +++ b/repair/globals.h > > > @@ -82,6 +82,8 @@ extern int isa_file; > > > extern int zap_log; > > > extern int dumpcore; /* abort, not exit on fatal errs */ > > > extern int force_geo; /* can set geo on low confidence info */ > > > +/* Abort after forcing NEEDSREPAIR to test its functionality */ > > > +extern bool abort_after_force_needsrepair; > > > extern int assume_xfs; /* assume we have an xfs fs */ > > > extern char *log_name; /* Name of log device */ > > > extern int log_spec; /* Log dev specified as option */ > > > diff --git a/repair/phase1.c b/repair/phase1.c > > > index b26d25f8..57f72cd0 100644 > > > --- a/repair/phase1.c > > > +++ b/repair/phase1.c > > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); > > > */ > > > sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; > > > > > > + /* Simulate a crash after setting needsrepair. */ > > > + if (primary_sb_modified && add_needsrepair && > > > + abort_after_force_needsrepair) > > > + exit(55); > > > + > > > free(sb); > > > } > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > > index ee377e8a..ae7106a6 100644 > > > --- a/repair/xfs_repair.c > > > +++ b/repair/xfs_repair.c > > > @@ -44,6 +44,7 @@ enum o_opt_nums { > > > BLOAD_LEAF_SLACK, > > > BLOAD_NODE_SLACK, > > > NOQUOTA, > > > + FORCE_NEEDSREPAIR_ABORT, > > > O_MAX_OPTS, > > > }; > > > > > > @@ -57,6 +58,7 @@ static char *o_opts[] = { > > > [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", > > > [BLOAD_NODE_SLACK] = "debug_bload_node_slack", > > > [NOQUOTA] = "noquota", > > > + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", > > > [O_MAX_OPTS] = NULL, > > > }; > > > > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv) > > > _("-o debug_bload_node_slack requires a parameter\n")); > > > bload_node_slack = (int)strtol(val, NULL, 0); > > > break; > > > + case FORCE_NEEDSREPAIR_ABORT: > > > + abort_after_force_needsrepair = true; > > > + break; > > > case NOQUOTA: > > > quotacheck_skip(); > > > break; > > > @@ -795,6 +800,8 @@ force_needsrepair( > > > error = -libxfs_bwrite(bp); > > > if (error) > > > do_log(_("couldn't force needsrepair, err=%d\n"), error); > > > + if (abort_after_force_needsrepair) > > > + exit(55); > > > } > > > if (bp) > > > libxfs_buf_relse(bp); > > > > > >
On Tue, Feb 09, 2021 at 01:59:39PM -0500, Brian Foster wrote: > On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote: > > On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote: > > > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > Simulate a crash when anyone calls force_needsrepair. This is a debug > > > > knob so that we can test that the kernel won't mount after setting > > > > needsrepair and that a re-run of xfs_repair will clear the flag. > > > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > --- > > > > > > Can't we just use db to manually set the bit on the superblock? > > > > No, because the fstest uses this debug knob to simulate the following: > > > > 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1' > > 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR > > 3) system goes down and repair never completes > > 4) verify that we can't mount > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > > 6) verify that mount works again > > > > Ok, but that seems like circular reasoning. I'm sorry, but I don't see how this is circular logic? The test needs to show that NEEDSREPAIR is turned on during phase 1 (or 2) when we apply an upgrade, and it needs to induce some kind of early exit so that the needsrepair clearing code after phase 7 does not run. If we set NEEDSREPAIR with xfs_db before running repair then we have no way to detect if the inobtcount upgrade doesn't set needsrepair. If we don't have a debugging knob to stop repair before it reaches phase 7, we're not really testing a genuine early-repair-exit scenario. Yes, we can use xfs_db to manually set the flag after repair returns, but that doesn't fill the testing gap above. > It wouldn't be quite the > same as a simulated repair failure, but ISTM that if we set the bit > manually, we can still verify steps 4, 5 and 6 as is (with the caveat > that the repair invocation performs a feature upgrade). I'm not sure how > important it really is to verify that a feature upgrade sequence sets > the bit if it happens to fail provided we have independent tests that 1. > verify the needsrepair bit works as expected and 2. verify the feature > upgrades work appropriately, since that is the primary functionality. > > I wanted to think about that a little more before replying, but I also > just realized something odd when digging into the debug code: > > # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch > Phase 1 - find and verify superblock... > Marking filesystem in need of repair. > writing modified primary superblock > Phase 2 - using internal log > - zero log... > ERROR: The filesystem has valuable metadata changes in a log which needs to > ... > # mount /dev/test/scratch /mnt/ > mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error. > # > > It looks like we can set a feature upgrade bit on the superblock before > we've examined the log and potentially discovered that it's dirty (phase > 2). If the log is recoverable, that puts the user in a bit of a bind.. Heh, funny that I was thinking that the upgrades shouldn't really be happening in phase 1 anyway-- I've (separately) started working on a patch to make it so that you can add reflink and finobt to a filesystem. Those upgrades require somewhat more intensive checks of the filesystem (such as checking free space in each AG), so I ended up dumping them into phase 2, since the xfs_mount and buffer cache aren't fully initialized until after phase 1. So, yeah, the upgrade code should move to phase2() after log zeroing and before the AG scan. --D > Brian > > > and the other scenario is: > > > > 1) fuzz a directory entry in such a way that repair will decide to > > blow out the dirent and rebuild the directory later > > 2) sysadmin issues 'xfs_repair /dev/sda1' > > 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the > > dirent to trigger the rebuild later > > 3) system goes down and repair never completes > > 4) verify that we can't mount > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > > 6) verify that mount works again > > > > Both cases reflect what I think are the most likely failure scenarios, > > hence the knob needs to be in xfs_repair to prevent it from running to > > completion. > > > > (And yes, I've been recently very bad at sending fstests out for review > > the past few months; I will get that done by this afternoon.) > > > > --D > > > > > Brian > > > > > > > repair/globals.c | 1 + > > > > repair/globals.h | 2 ++ > > > > repair/phase1.c | 5 +++++ > > > > repair/xfs_repair.c | 7 +++++++ > > > > 4 files changed, 15 insertions(+) > > > > > > > > > > > > diff --git a/repair/globals.c b/repair/globals.c > > > > index 699a96ee..b0e23864 100644 > > > > --- a/repair/globals.c > > > > +++ b/repair/globals.c > > > > @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ > > > > int isa_file; > > > > int zap_log; > > > > int dumpcore; /* abort, not exit on fatal errs */ > > > > +bool abort_after_force_needsrepair; > > > > int force_geo; /* can set geo on low confidence info */ > > > > int assume_xfs; /* assume we have an xfs fs */ > > > > char *log_name; /* Name of log device */ > > > > diff --git a/repair/globals.h b/repair/globals.h > > > > index 043b3e8e..9fa73b2c 100644 > > > > --- a/repair/globals.h > > > > +++ b/repair/globals.h > > > > @@ -82,6 +82,8 @@ extern int isa_file; > > > > extern int zap_log; > > > > extern int dumpcore; /* abort, not exit on fatal errs */ > > > > extern int force_geo; /* can set geo on low confidence info */ > > > > +/* Abort after forcing NEEDSREPAIR to test its functionality */ > > > > +extern bool abort_after_force_needsrepair; > > > > extern int assume_xfs; /* assume we have an xfs fs */ > > > > extern char *log_name; /* Name of log device */ > > > > extern int log_spec; /* Log dev specified as option */ > > > > diff --git a/repair/phase1.c b/repair/phase1.c > > > > index b26d25f8..57f72cd0 100644 > > > > --- a/repair/phase1.c > > > > +++ b/repair/phase1.c > > > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); > > > > */ > > > > sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; > > > > > > > > + /* Simulate a crash after setting needsrepair. */ > > > > + if (primary_sb_modified && add_needsrepair && > > > > + abort_after_force_needsrepair) > > > > + exit(55); > > > > + > > > > free(sb); > > > > } > > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > > > index ee377e8a..ae7106a6 100644 > > > > --- a/repair/xfs_repair.c > > > > +++ b/repair/xfs_repair.c > > > > @@ -44,6 +44,7 @@ enum o_opt_nums { > > > > BLOAD_LEAF_SLACK, > > > > BLOAD_NODE_SLACK, > > > > NOQUOTA, > > > > + FORCE_NEEDSREPAIR_ABORT, > > > > O_MAX_OPTS, > > > > }; > > > > > > > > @@ -57,6 +58,7 @@ static char *o_opts[] = { > > > > [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", > > > > [BLOAD_NODE_SLACK] = "debug_bload_node_slack", > > > > [NOQUOTA] = "noquota", > > > > + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", > > > > [O_MAX_OPTS] = NULL, > > > > }; > > > > > > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv) > > > > _("-o debug_bload_node_slack requires a parameter\n")); > > > > bload_node_slack = (int)strtol(val, NULL, 0); > > > > break; > > > > + case FORCE_NEEDSREPAIR_ABORT: > > > > + abort_after_force_needsrepair = true; > > > > + break; > > > > case NOQUOTA: > > > > quotacheck_skip(); > > > > break; > > > > @@ -795,6 +800,8 @@ force_needsrepair( > > > > error = -libxfs_bwrite(bp); > > > > if (error) > > > > do_log(_("couldn't force needsrepair, err=%d\n"), error); > > > > + if (abort_after_force_needsrepair) > > > > + exit(55); > > > > } > > > > if (bp) > > > > libxfs_buf_relse(bp); > > > > > > > > > >
On Tue, Feb 09, 2021 at 11:59:20AM -0800, Darrick J. Wong wrote: > On Tue, Feb 09, 2021 at 01:59:39PM -0500, Brian Foster wrote: > > On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote: > > > On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote: > > > > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote: > > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > > > Simulate a crash when anyone calls force_needsrepair. This is a debug > > > > > knob so that we can test that the kernel won't mount after setting > > > > > needsrepair and that a re-run of xfs_repair will clear the flag. > > > > > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > > --- > > > > > > > > Can't we just use db to manually set the bit on the superblock? > > > > > > No, because the fstest uses this debug knob to simulate the following: > > > > > > 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1' > > > 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR > > > 3) system goes down and repair never completes > > > 4) verify that we can't mount > > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > > > 6) verify that mount works again > > > > > > > Ok, but that seems like circular reasoning. > > I'm sorry, but I don't see how this is circular logic? > I was just referring to the insinuation that we have to maintain such debug logic because a test exists that wants it. Clearly that's why it exists. ;) > The test needs to show that NEEDSREPAIR is turned on during phase 1 (or > 2) when we apply an upgrade, and it needs to induce some kind of early > exit so that the needsrepair clearing code after phase 7 does not run. > > If we set NEEDSREPAIR with xfs_db before running repair then we have no > way to detect if the inobtcount upgrade doesn't set needsrepair. > > If we don't have a debugging knob to stop repair before it reaches phase > 7, we're not really testing a genuine early-repair-exit scenario. Yes, > we can use xfs_db to manually set the flag after repair returns, but > that doesn't fill the testing gap above. > But is it worth maintaining test specific debug logic in an application just to confirm that particular feature bit upgrades actually set the bit? It seems sufficient to me to test that needsrepair functionality works as expected and that individual feature upgrade works as well. Given the discussion on patch 7, perhaps it makes more sense to at least defer this sort of injection mechanism until we have a scheme for generic needsrepair usage worked out for xfs_repair? I am wondering if there's a way to make repair fail without requiring additional code, but if not and we do require some sort of injection mode, I suspect we might end up better served by something more generic (i.e. capable of failures at random points) rather than defining a command line option specifically for a particular fstest.. Brian > > It wouldn't be quite the > > same as a simulated repair failure, but ISTM that if we set the bit > > manually, we can still verify steps 4, 5 and 6 as is (with the caveat > > that the repair invocation performs a feature upgrade). I'm not sure how > > important it really is to verify that a feature upgrade sequence sets > > the bit if it happens to fail provided we have independent tests that 1. > > verify the needsrepair bit works as expected and 2. verify the feature > > upgrades work appropriately, since that is the primary functionality. > > > > I wanted to think about that a little more before replying, but I also > > just realized something odd when digging into the debug code: > > > > # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch > > Phase 1 - find and verify superblock... > > Marking filesystem in need of repair. > > writing modified primary superblock > > Phase 2 - using internal log > > - zero log... > > ERROR: The filesystem has valuable metadata changes in a log which needs to > > ... > > # mount /dev/test/scratch /mnt/ > > mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error. > > # > > > > It looks like we can set a feature upgrade bit on the superblock before > > we've examined the log and potentially discovered that it's dirty (phase > > 2). If the log is recoverable, that puts the user in a bit of a bind.. > > Heh, funny that I was thinking that the upgrades shouldn't really be > happening in phase 1 anyway-- > > I've (separately) started working on a patch to make it so that you can > add reflink and finobt to a filesystem. Those upgrades require somewhat > more intensive checks of the filesystem (such as checking free space in > each AG), so I ended up dumping them into phase 2, since the xfs_mount > and buffer cache aren't fully initialized until after phase 1. > > So, yeah, the upgrade code should move to phase2() after log zeroing and > before the AG scan. > > --D > > > Brian > > > > > and the other scenario is: > > > > > > 1) fuzz a directory entry in such a way that repair will decide to > > > blow out the dirent and rebuild the directory later > > > 2) sysadmin issues 'xfs_repair /dev/sda1' > > > 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the > > > dirent to trigger the rebuild later > > > 3) system goes down and repair never completes > > > 4) verify that we can't mount > > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs > > > 6) verify that mount works again > > > > > > Both cases reflect what I think are the most likely failure scenarios, > > > hence the knob needs to be in xfs_repair to prevent it from running to > > > completion. > > > > > > (And yes, I've been recently very bad at sending fstests out for review > > > the past few months; I will get that done by this afternoon.) > > > > > > --D > > > > > > > Brian > > > > > > > > > repair/globals.c | 1 + > > > > > repair/globals.h | 2 ++ > > > > > repair/phase1.c | 5 +++++ > > > > > repair/xfs_repair.c | 7 +++++++ > > > > > 4 files changed, 15 insertions(+) > > > > > > > > > > > > > > > diff --git a/repair/globals.c b/repair/globals.c > > > > > index 699a96ee..b0e23864 100644 > > > > > --- a/repair/globals.c > > > > > +++ b/repair/globals.c > > > > > @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ > > > > > int isa_file; > > > > > int zap_log; > > > > > int dumpcore; /* abort, not exit on fatal errs */ > > > > > +bool abort_after_force_needsrepair; > > > > > int force_geo; /* can set geo on low confidence info */ > > > > > int assume_xfs; /* assume we have an xfs fs */ > > > > > char *log_name; /* Name of log device */ > > > > > diff --git a/repair/globals.h b/repair/globals.h > > > > > index 043b3e8e..9fa73b2c 100644 > > > > > --- a/repair/globals.h > > > > > +++ b/repair/globals.h > > > > > @@ -82,6 +82,8 @@ extern int isa_file; > > > > > extern int zap_log; > > > > > extern int dumpcore; /* abort, not exit on fatal errs */ > > > > > extern int force_geo; /* can set geo on low confidence info */ > > > > > +/* Abort after forcing NEEDSREPAIR to test its functionality */ > > > > > +extern bool abort_after_force_needsrepair; > > > > > extern int assume_xfs; /* assume we have an xfs fs */ > > > > > extern char *log_name; /* Name of log device */ > > > > > extern int log_spec; /* Log dev specified as option */ > > > > > diff --git a/repair/phase1.c b/repair/phase1.c > > > > > index b26d25f8..57f72cd0 100644 > > > > > --- a/repair/phase1.c > > > > > +++ b/repair/phase1.c > > > > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); > > > > > */ > > > > > sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; > > > > > > > > > > + /* Simulate a crash after setting needsrepair. */ > > > > > + if (primary_sb_modified && add_needsrepair && > > > > > + abort_after_force_needsrepair) > > > > > + exit(55); > > > > > + > > > > > free(sb); > > > > > } > > > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > > > > index ee377e8a..ae7106a6 100644 > > > > > --- a/repair/xfs_repair.c > > > > > +++ b/repair/xfs_repair.c > > > > > @@ -44,6 +44,7 @@ enum o_opt_nums { > > > > > BLOAD_LEAF_SLACK, > > > > > BLOAD_NODE_SLACK, > > > > > NOQUOTA, > > > > > + FORCE_NEEDSREPAIR_ABORT, > > > > > O_MAX_OPTS, > > > > > }; > > > > > > > > > > @@ -57,6 +58,7 @@ static char *o_opts[] = { > > > > > [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", > > > > > [BLOAD_NODE_SLACK] = "debug_bload_node_slack", > > > > > [NOQUOTA] = "noquota", > > > > > + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", > > > > > [O_MAX_OPTS] = NULL, > > > > > }; > > > > > > > > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv) > > > > > _("-o debug_bload_node_slack requires a parameter\n")); > > > > > bload_node_slack = (int)strtol(val, NULL, 0); > > > > > break; > > > > > + case FORCE_NEEDSREPAIR_ABORT: > > > > > + abort_after_force_needsrepair = true; > > > > > + break; > > > > > case NOQUOTA: > > > > > quotacheck_skip(); > > > > > break; > > > > > @@ -795,6 +800,8 @@ force_needsrepair( > > > > > error = -libxfs_bwrite(bp); > > > > > if (error) > > > > > do_log(_("couldn't force needsrepair, err=%d\n"), error); > > > > > + if (abort_after_force_needsrepair) > > > > > + exit(55); > > > > > } > > > > > if (bp) > > > > > libxfs_buf_relse(bp); > > > > > > > > > > > > > > >
On 2/9/21 1:59 PM, Darrick J. Wong wrote: >> # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch >> Phase 1 - find and verify superblock... >> Marking filesystem in need of repair. >> writing modified primary superblock >> Phase 2 - using internal log >> - zero log... >> ERROR: The filesystem has valuable metadata changes in a log which needs to >> ... >> # mount /dev/test/scratch /mnt/ >> mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error. >> # >> >> It looks like we can set a feature upgrade bit on the superblock before >> we've examined the log and potentially discovered that it's dirty (phase >> 2). If the log is recoverable, that puts the user in a bit of a bind.. > Heh, funny that I was thinking that the upgrades shouldn't really be > happening in phase 1 anyway-- > > I've (separately) started working on a patch to make it so that you can > add reflink and finobt to a filesystem. Those upgrades require somewhat > more intensive checks of the filesystem (such as checking free space in > each AG), so I ended up dumping them into phase 2, since the xfs_mount > and buffer cache aren't fully initialized until after phase 1. > > So, yeah, the upgrade code should move to phase2() after log zeroing and > before the AG scan. Oh, whoops - based on this, I think that the prior patch [PATCH 08/10] xfs_repair: allow setting the needsrepair flag needs to be adjusted as well, right; we don't want to set it in phase 1 or we might set it then abort on a dirty log and the user is stuck. So maybe I should merge up through [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories and hold off on the rest? Or should I just hold off on the series and let you reassemble (again?) (Am I right that the upgrade bits will need to be moved and the series resent?) -Eric
On Wed, Feb 10, 2021 at 03:41:52PM -0600, Eric Sandeen wrote: > On 2/9/21 1:59 PM, Darrick J. Wong wrote: > >> # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch > >> Phase 1 - find and verify superblock... > >> Marking filesystem in need of repair. > >> writing modified primary superblock > >> Phase 2 - using internal log > >> - zero log... > >> ERROR: The filesystem has valuable metadata changes in a log which needs to > >> ... > >> # mount /dev/test/scratch /mnt/ > >> mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error. > >> # > >> > >> It looks like we can set a feature upgrade bit on the superblock before > >> we've examined the log and potentially discovered that it's dirty (phase > >> 2). If the log is recoverable, that puts the user in a bit of a bind.. > > Heh, funny that I was thinking that the upgrades shouldn't really be > > happening in phase 1 anyway-- > > > > I've (separately) started working on a patch to make it so that you can > > add reflink and finobt to a filesystem. Those upgrades require somewhat > > more intensive checks of the filesystem (such as checking free space in > > each AG), so I ended up dumping them into phase 2, since the xfs_mount > > and buffer cache aren't fully initialized until after phase 1. > > > > So, yeah, the upgrade code should move to phase2() after log zeroing and > > before the AG scan. > > Oh, whoops - > > based on this, I think that the prior patch > [PATCH 08/10] xfs_repair: allow setting the needsrepair flag > needs to be adjusted as well, right; we don't want to set it in phase 1 > or we might set it then abort on a dirty log and the user is stuck. > > So maybe I should merge up through > > [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories > > and hold off on the rest? Or should I just hold off on the series and let > you reassemble (again?) Yeah, I'll push out v5 tomorrow morning with all this cleaned up. --D > (Am I right that the upgrade bits will need to be moved and the series resent?) > > -Eric
diff --git a/repair/globals.c b/repair/globals.c index 699a96ee..b0e23864 100644 --- a/repair/globals.c +++ b/repair/globals.c @@ -40,6 +40,7 @@ int dangerously; /* live dangerously ... fix ro mount */ int isa_file; int zap_log; int dumpcore; /* abort, not exit on fatal errs */ +bool abort_after_force_needsrepair; int force_geo; /* can set geo on low confidence info */ int assume_xfs; /* assume we have an xfs fs */ char *log_name; /* Name of log device */ diff --git a/repair/globals.h b/repair/globals.h index 043b3e8e..9fa73b2c 100644 --- a/repair/globals.h +++ b/repair/globals.h @@ -82,6 +82,8 @@ extern int isa_file; extern int zap_log; extern int dumpcore; /* abort, not exit on fatal errs */ extern int force_geo; /* can set geo on low confidence info */ +/* Abort after forcing NEEDSREPAIR to test its functionality */ +extern bool abort_after_force_needsrepair; extern int assume_xfs; /* assume we have an xfs fs */ extern char *log_name; /* Name of log device */ extern int log_spec; /* Log dev specified as option */ diff --git a/repair/phase1.c b/repair/phase1.c index b26d25f8..57f72cd0 100644 --- a/repair/phase1.c +++ b/repair/phase1.c @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n")); */ sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0; + /* Simulate a crash after setting needsrepair. */ + if (primary_sb_modified && add_needsrepair && + abort_after_force_needsrepair) + exit(55); + free(sb); } diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index ee377e8a..ae7106a6 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -44,6 +44,7 @@ enum o_opt_nums { BLOAD_LEAF_SLACK, BLOAD_NODE_SLACK, NOQUOTA, + FORCE_NEEDSREPAIR_ABORT, O_MAX_OPTS, }; @@ -57,6 +58,7 @@ static char *o_opts[] = { [BLOAD_LEAF_SLACK] = "debug_bload_leaf_slack", [BLOAD_NODE_SLACK] = "debug_bload_node_slack", [NOQUOTA] = "noquota", + [FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort", [O_MAX_OPTS] = NULL, }; @@ -282,6 +284,9 @@ process_args(int argc, char **argv) _("-o debug_bload_node_slack requires a parameter\n")); bload_node_slack = (int)strtol(val, NULL, 0); break; + case FORCE_NEEDSREPAIR_ABORT: + abort_after_force_needsrepair = true; + break; case NOQUOTA: quotacheck_skip(); break; @@ -795,6 +800,8 @@ force_needsrepair( error = -libxfs_bwrite(bp); if (error) do_log(_("couldn't force needsrepair, err=%d\n"), error); + if (abort_after_force_needsrepair) + exit(55); } if (bp) libxfs_buf_relse(bp);