| Message ID | 20230807112850.9198-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | common/rc: drop '-f' parameter from fsck.exfat | expand |
On Mon, Aug 07, 2023 at 01:28:50PM +0200, David Disseldorp wrote: > fsck.exfat doesn't support the '-f' flag, so add a special case to > _repair_test_fs(). I'm wondering why _repair_scratch_fs() doesn't have the '-f', but the _repair_test_fs() has it. Looks like the '-f' option was for extN fs originally, it's not a fsck common option, but in fsck.ext4. So I think the '-f' might not be a necessary option. As _repair_scratch_fs works without it, can we just remove the '-f' from _repair_test_fs()? As _repair_test_fs was added by Ted, and _repair_scratch_fs was added by Darrick, so CC them to get more review -- do we real need the '-f' or not by default :) Thanks, Zorro > > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > common/rc | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/common/rc b/common/rc > index 5c4429ed..ac7e50f1 100644 > --- a/common/rc > +++ b/common/rc > @@ -1229,6 +1229,14 @@ _repair_test_fs() > res=$? > fi > ;; > + exfat) > + # exfat doesn't support -f > + fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1 > + res=$? > + if ((res < 4)); then > + res=0 > + fi > + ;; > *) > # Let's hope fsck -y suffices... > fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1 > -- > 2.35.3 >
Thanks for the feedback, Zorro... On Tue, 8 Aug 2023 01:48:35 +0800, Zorro Lang wrote: > On Mon, Aug 07, 2023 at 01:28:50PM +0200, David Disseldorp wrote: > > fsck.exfat doesn't support the '-f' flag, so add a special case to > > _repair_test_fs(). > > I'm wondering why _repair_scratch_fs() doesn't have the '-f', but the > _repair_test_fs() has it. Looks like the '-f' option was for extN fs > originally, it's not a fsck common option, but in fsck.ext4. > > So I think the '-f' might not be a necessary option. As _repair_scratch_fs > works without it, can we just remove the '-f' from _repair_test_fs()? The fsck.ext4 usage states: -f Force checking even if filesystem is marked clean As _repair_test_fs() is only called on _check_test_fs() failure, I suppose '-f' removal should be fine. Still, to avoid any behavioural changes I could also just add an explicit ext case with '-f'. There's also a question of what btrfs should do here, as calling the "fsck.btrfs" no-op script following btrfs check failure likely doesn't make much sense. Cheers, David
On Tue, Aug 08, 2023 at 01:48:35AM +0800, Zorro Lang wrote: > On Mon, Aug 07, 2023 at 01:28:50PM +0200, David Disseldorp wrote: > > fsck.exfat doesn't support the '-f' flag, so add a special case to > > _repair_test_fs(). > > I'm wondering why _repair_scratch_fs() doesn't have the '-f', but the > _repair_test_fs() has it. Looks like the '-f' option was for extN fs > originally, it's not a fsck common option, but in fsck.ext4. > > So I think the '-f' might not be a necessary option. As _repair_scratch_fs > works without it, can we just remove the '-f' from _repair_test_fs()? > > As _repair_test_fs was added by Ted, and _repair_scratch_fs was added > by Darrick, so CC them to get more review -- do we real need the '-f' > or not by default :) I suspect that was just an ext4ism, since fsck itself doesn't document any such option. --D > Thanks, > Zorro > > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > --- > > common/rc | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/common/rc b/common/rc > > index 5c4429ed..ac7e50f1 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -1229,6 +1229,14 @@ _repair_test_fs() > > res=$? > > fi > > ;; > > + exfat) > > + # exfat doesn't support -f > > + fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1 > > + res=$? > > + if ((res < 4)); then > > + res=0 > > + fi > > + ;; > > *) > > # Let's hope fsck -y suffices... > > fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1 > > -- > > 2.35.3 > > >
On Tue, Aug 08, 2023 at 12:34:49AM +0200, David Disseldorp wrote: > Thanks for the feedback, Zorro... > > On Tue, 8 Aug 2023 01:48:35 +0800, Zorro Lang wrote: > > > On Mon, Aug 07, 2023 at 01:28:50PM +0200, David Disseldorp wrote: > > > fsck.exfat doesn't support the '-f' flag, so add a special case to > > > _repair_test_fs(). > > > > I'm wondering why _repair_scratch_fs() doesn't have the '-f', but the > > _repair_test_fs() has it. Looks like the '-f' option was for extN fs > > originally, it's not a fsck common option, but in fsck.ext4. > > > > So I think the '-f' might not be a necessary option. As _repair_scratch_fs > > works without it, can we just remove the '-f' from _repair_test_fs()? > > The fsck.ext4 usage states: > -f Force checking even if filesystem is marked clean > > As _repair_test_fs() is only called on _check_test_fs() failure, I > suppose '-f' removal should be fine. Still, to avoid any behavioural > changes I could also just add an explicit ext case with '-f'. > > There's also a question of what btrfs should do here, as calling the > "fsck.btrfs" no-op script following btrfs check failure likely > doesn't make much sense. Hmm, I think you're right. I just checked the commit log, the code (check calls _repair_test_fs if TEST_DEV is corrupted) is added by this commit: commit c7d81cdecbefd5768163a195e8d5257279216a34 Author: Theodore Ts'o <tytso@mit.edu> Date: Fri Apr 21 10:51:31 2023 -0700 check: try to fix the test device if it gets corrupted That commit cares about xfs and extN more, didn't think about other fs likes btrfs. If btrfs would like to call btrfs-check, not fsck.btrfs, we should do that. And as the "-f" isn't a common option of fsck, so we'd better to not use it in " *)" branch. If extN hope to use it, we'd better to have a "ext4)" branch. If btrfs would like to call btrfs-check in _repair_test_fs(), we need to do the same thing in _repair_scratch_fs(). CC btrfs list to get more review/suggestion about that. Thanks, Zorro > > Cheers, David >
diff --git a/common/rc b/common/rc index 5c4429ed..ac7e50f1 100644 --- a/common/rc +++ b/common/rc @@ -1229,6 +1229,14 @@ _repair_test_fs() res=$? fi ;; + exfat) + # exfat doesn't support -f + fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1 + res=$? + if ((res < 4)); then + res=0 + fi + ;; *) # Let's hope fsck -y suffices... fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1
fsck.exfat doesn't support the '-f' flag, so add a special case to _repair_test_fs(). Signed-off-by: David Disseldorp <ddiss@suse.de> --- common/rc | 8 ++++++++ 1 file changed, 8 insertions(+)