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")
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.
-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
