| Message ID | 161284384955.3057868.8076509276356847362.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: add the ability to flag a fs for repair | expand |
On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at > program start and (presumably) clear it by the end of the run. This > code isn't terribly useful to users; it's mainly here so that fstests > can exercise the functionality. What does the quietly above mean? Otherwise this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 2/9/21 3:15 AM, Christoph Hellwig wrote: > On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: >> From: Darrick J. Wong <djwong@kernel.org> >> >> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at >> program start and (presumably) clear it by the end of the run. This >> code isn't terribly useful to users; it's mainly here so that fstests >> can exercise the functionality. > > What does the quietly above mean? I think it means "don't document it in the man page, this is a secret for XFS developers and testers" > Otherwise this looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> >
On Tue, Feb 09, 2021 at 08:41:34AM -0600, Eric Sandeen wrote: > > > On 2/9/21 3:15 AM, Christoph Hellwig wrote: > > On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: > >> From: Darrick J. Wong <djwong@kernel.org> > >> > >> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at > >> program start and (presumably) clear it by the end of the run. This > >> code isn't terribly useful to users; it's mainly here so that fstests > >> can exercise the functionality. > > > > What does the quietly above mean? > > I think it means "don't document it in the man page, this is a secret > for XFS developers and testers" Yep. I'll add the following to the commit message: "We don't document this flag in the manual pages at all because repair clears needsrepair at exit, which means the knobs only exist for fstests to exercise the functionality." --D > > > Otherwise this looks good: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > >
On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at > program start and (presumably) clear it by the end of the run. This > code isn't terribly useful to users; it's mainly here so that fstests > can exercise the functionality. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > repair/globals.c | 2 ++ > repair/globals.h | 2 ++ > repair/phase1.c | 23 +++++++++++++++++++++++ > repair/xfs_repair.c | 9 +++++++++ > 4 files changed, 36 insertions(+) > > ... > diff --git a/repair/phase1.c b/repair/phase1.c > index 00b98584..b26d25f8 100644 > --- a/repair/phase1.c > +++ b/repair/phase1.c > @@ -30,6 +30,26 @@ alloc_ag_buf(int size) > return(bp); > } > > +static void > +set_needsrepair( > + struct xfs_sb *sb) > +{ > + if (!xfs_sb_version_hascrc(sb)) { > + printf( > + _("needsrepair flag only supported on V5 filesystems.\n")); > + exit(0); > + } > + > + if (xfs_sb_version_needsrepair(sb)) { > + printf(_("Filesystem already marked as needing repair.\n")); > + return; > + } Any reason this doesn't exit the repair like the lazy counter logic does? Otherwise seems fine: Reviewed-by: Brian Foster <bfoster@redhat.com> > + > + printf(_("Marking filesystem in need of repair.\n")); > + primary_sb_modified = 1; > + sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; > +} > + > /* > * this has got to be big enough to hold 4 sectors > */ > @@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n")); > } > } > > + if (add_needsrepair) > + set_needsrepair(sb); > + > /* shared_vn should be zero */ > if (sb->sb_shared_vn) { > do_warn(_("resetting shared_vn to zero\n")); > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > index 9dc73854..ee377e8a 100644 > --- a/repair/xfs_repair.c > +++ b/repair/xfs_repair.c > @@ -65,11 +65,13 @@ static char *o_opts[] = { > */ > enum c_opt_nums { > CONVERT_LAZY_COUNT = 0, > + CONVERT_NEEDSREPAIR, > C_MAX_OPTS, > }; > > static char *c_opts[] = { > [CONVERT_LAZY_COUNT] = "lazycount", > + [CONVERT_NEEDSREPAIR] = "needsrepair", > [C_MAX_OPTS] = NULL, > }; > > @@ -302,6 +304,13 @@ process_args(int argc, char **argv) > lazy_count = (int)strtol(val, NULL, 0); > convert_lazy_count = 1; > break; > + case CONVERT_NEEDSREPAIR: > + if (!val) > + do_abort( > + _("-c needsrepair requires a parameter\n")); > + if (strtol(val, NULL, 0) == 1) > + add_needsrepair = true; > + break; > default: > unknown('c', val); > break; >
On Tue, Feb 09, 2021 at 12:21:21PM -0500, Brian Foster wrote: > On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at > > program start and (presumably) clear it by the end of the run. This > > code isn't terribly useful to users; it's mainly here so that fstests > > can exercise the functionality. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > repair/globals.c | 2 ++ > > repair/globals.h | 2 ++ > > repair/phase1.c | 23 +++++++++++++++++++++++ > > repair/xfs_repair.c | 9 +++++++++ > > 4 files changed, 36 insertions(+) > > > > > ... > > diff --git a/repair/phase1.c b/repair/phase1.c > > index 00b98584..b26d25f8 100644 > > --- a/repair/phase1.c > > +++ b/repair/phase1.c > > @@ -30,6 +30,26 @@ alloc_ag_buf(int size) > > return(bp); > > } > > > > +static void > > +set_needsrepair( > > + struct xfs_sb *sb) > > +{ > > + if (!xfs_sb_version_hascrc(sb)) { > > + printf( > > + _("needsrepair flag only supported on V5 filesystems.\n")); > > + exit(0); > > + } > > + > > + if (xfs_sb_version_needsrepair(sb)) { > > + printf(_("Filesystem already marked as needing repair.\n")); > > + return; > > + } > > Any reason this doesn't exit the repair like the lazy counter logic > does? Otherwise seems fine: No particular reason. Will fix for consistency's sake. > Reviewed-by: Brian Foster <bfoster@redhat.com> > > > + > > + printf(_("Marking filesystem in need of repair.\n")); > > + primary_sb_modified = 1; > > + sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; > > +} > > + > > /* > > * this has got to be big enough to hold 4 sectors > > */ > > @@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n")); > > } > > } > > > > + if (add_needsrepair) > > + set_needsrepair(sb); > > + > > /* shared_vn should be zero */ > > if (sb->sb_shared_vn) { > > do_warn(_("resetting shared_vn to zero\n")); > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > index 9dc73854..ee377e8a 100644 > > --- a/repair/xfs_repair.c > > +++ b/repair/xfs_repair.c > > @@ -65,11 +65,13 @@ static char *o_opts[] = { > > */ > > enum c_opt_nums { > > CONVERT_LAZY_COUNT = 0, > > + CONVERT_NEEDSREPAIR, > > C_MAX_OPTS, > > }; > > > > static char *c_opts[] = { > > [CONVERT_LAZY_COUNT] = "lazycount", > > + [CONVERT_NEEDSREPAIR] = "needsrepair", > > [C_MAX_OPTS] = NULL, > > }; > > > > @@ -302,6 +304,13 @@ process_args(int argc, char **argv) > > lazy_count = (int)strtol(val, NULL, 0); > > convert_lazy_count = 1; > > break; > > + case CONVERT_NEEDSREPAIR: > > + if (!val) > > + do_abort( > > + _("-c needsrepair requires a parameter\n")); > > + if (strtol(val, NULL, 0) == 1) > > + add_needsrepair = true; > > + break; > > default: > > unknown('c', val); > > break; > > >
On 2/9/21 12:10 PM, Darrick J. Wong wrote: > On Tue, Feb 09, 2021 at 12:21:21PM -0500, Brian Foster wrote: >> On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote: >>> From: Darrick J. Wong <djwong@kernel.org> >>> >>> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at >>> program start and (presumably) clear it by the end of the run. This >>> code isn't terribly useful to users; it's mainly here so that fstests >>> can exercise the functionality. >>> >>> Signed-off-by: Darrick J. Wong <djwong@kernel.org> >>> --- >>> repair/globals.c | 2 ++ >>> repair/globals.h | 2 ++ >>> repair/phase1.c | 23 +++++++++++++++++++++++ >>> repair/xfs_repair.c | 9 +++++++++ >>> 4 files changed, 36 insertions(+) >>> >>> >> ... >>> diff --git a/repair/phase1.c b/repair/phase1.c >>> index 00b98584..b26d25f8 100644 >>> --- a/repair/phase1.c >>> +++ b/repair/phase1.c >>> @@ -30,6 +30,26 @@ alloc_ag_buf(int size) >>> return(bp); >>> } >>> >>> +static void >>> +set_needsrepair( >>> + struct xfs_sb *sb) >>> +{ >>> + if (!xfs_sb_version_hascrc(sb)) { >>> + printf( >>> + _("needsrepair flag only supported on V5 filesystems.\n")); >>> + exit(0); >>> + } >>> + >>> + if (xfs_sb_version_needsrepair(sb)) { >>> + printf(_("Filesystem already marked as needing repair.\n")); >>> + return; >>> + } >> >> Any reason this doesn't exit the repair like the lazy counter logic >> does? Otherwise seems fine: > > No particular reason. Will fix for consistency's sake. I can swap the return; for an exit(0); on my end if you like and note it in the commit log. -Eric
On 2/9/21 10:47 AM, Darrick J. Wong wrote: > "We don't document this flag in the manual pages at all because repair > clears needsrepair at exit, which means the knobs only exist for fstests > to exercise the functionality." I can add that and save you a re-send. -Eric
diff --git a/repair/globals.c b/repair/globals.c index 110d98b6..699a96ee 100644 --- a/repair/globals.c +++ b/repair/globals.c @@ -49,6 +49,8 @@ int rt_spec; /* Realtime dev specified as option */ int convert_lazy_count; /* Convert lazy-count mode on/off */ int lazy_count; /* What to set if to if converting */ +bool add_needsrepair; /* forcibly set needsrepair while repairing */ + /* misc status variables */ int primary_sb_modified; diff --git a/repair/globals.h b/repair/globals.h index 1d397b35..043b3e8e 100644 --- a/repair/globals.h +++ b/repair/globals.h @@ -90,6 +90,8 @@ extern int rt_spec; /* Realtime dev specified as option */ extern int convert_lazy_count; /* Convert lazy-count mode on/off */ extern int lazy_count; /* What to set if to if converting */ +extern bool add_needsrepair; + /* misc status variables */ extern int primary_sb_modified; diff --git a/repair/phase1.c b/repair/phase1.c index 00b98584..b26d25f8 100644 --- a/repair/phase1.c +++ b/repair/phase1.c @@ -30,6 +30,26 @@ alloc_ag_buf(int size) return(bp); } +static void +set_needsrepair( + struct xfs_sb *sb) +{ + if (!xfs_sb_version_hascrc(sb)) { + printf( + _("needsrepair flag only supported on V5 filesystems.\n")); + exit(0); + } + + if (xfs_sb_version_needsrepair(sb)) { + printf(_("Filesystem already marked as needing repair.\n")); + return; + } + + printf(_("Marking filesystem in need of repair.\n")); + primary_sb_modified = 1; + sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR; +} + /* * this has got to be big enough to hold 4 sectors */ @@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n")); } } + if (add_needsrepair) + set_needsrepair(sb); + /* shared_vn should be zero */ if (sb->sb_shared_vn) { do_warn(_("resetting shared_vn to zero\n")); diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index 9dc73854..ee377e8a 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -65,11 +65,13 @@ static char *o_opts[] = { */ enum c_opt_nums { CONVERT_LAZY_COUNT = 0, + CONVERT_NEEDSREPAIR, C_MAX_OPTS, }; static char *c_opts[] = { [CONVERT_LAZY_COUNT] = "lazycount", + [CONVERT_NEEDSREPAIR] = "needsrepair", [C_MAX_OPTS] = NULL, }; @@ -302,6 +304,13 @@ process_args(int argc, char **argv) lazy_count = (int)strtol(val, NULL, 0); convert_lazy_count = 1; break; + case CONVERT_NEEDSREPAIR: + if (!val) + do_abort( + _("-c needsrepair requires a parameter\n")); + if (strtol(val, NULL, 0) == 1) + add_needsrepair = true; + break; default: unknown('c', val); break;