On Tue, Mar 06, 2018 at 12:34:20PM -0600, Eric Sandeen wrote:
>
>
> On 3/6/18 11:27 AM, Darrick J. Wong wrote:
> > On Tue, Mar 06, 2018 at 11:16:50AM -0600, Eric Sandeen wrote:
> >>
> >>
> >> On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >>>
> >>> Don't advise the user to run xfs_repair on a filesystem that triggers
> >>> warnings but no errors; there's no corruption for it to fix.
> >>>
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >>
> >> I went looking for why ->need_repair is set if repair isn't needed, and:
> >>
> >> C symbol: need_repair
> >>
> >> File Function Line
> >> 0 scrub/xfs_scrub.h <global> 98 bool need_repair;
> >> 1 scrub/phase1.c xfs_setup_fs 239 ctx->need_repair = true;
> >> 2 scrub/xfs_scrub.c report_outcome 517 if (ctx->need_repair)
> >>
> >> um, when is ->need_repair ever false? What am I missing?
> >
> > In main():
> >
> > struct scrub_ctx ctx = {0};
> >
> > ctx.need_repair is false from the start of the program until the end of
> > phase 1 when we've decided that yes we can check this xfs filesystem.
>
> Ok so after more looking & discussion, what ->need_repair really means
> is "we got far enough to run the scrub ioctl?"
>
> If that's true, and errors remain for any reason (?), the user is told
> to run repair.
>
> So while I see that this patch improves the user experience, I wonder
> if we shouldn't take this opportunity to improve the developer experience
> by renaming ->need_repair to ->scrub_ran or something, because I think
> that makes a bit more sense semantically:
>
> if (scrub ioctl ran && errors remain)
> tell_user("run repair")
Ok. I'll update the name.
> My other quibble is that if (scrub ioctl ran && errors remain) is true only
> because "-n" was specified, it seems a little odd to instruct the user
> to run repair, when the errors may remain only because of -n. But that's
> a separate issue, I guess.
My thought process here is that any time we leave errors behind on the
filesystem we should advise the caller to run xfs_repair, whether that's
because the caller told us to fix things and we failed, or because the
caller trusts xfs_scrub to find the errors but not to fix them and
therefore ran xfs_scrub -n. Either way you have a broken fs and need to
repair it.
However, I wonder if you're thinking "the user told (scrub) they didn't
want to change anything, so why would we advise the user to run a
(repair) tool that changes things"?
--D
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
