[PATCH] common/rc: drop '-f' parameter from fsck.exfat

[PATCH] common/rc: drop '-f' parameter from fsck.exfat

[David Disseldorp]

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(+)

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

Re: [PATCH] common/rc: drop '-f' parameter from fsck.exfat

[Zorro Lang]

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
> 

Re: [PATCH] common/rc: drop '-f' parameter from fsck.exfat

[David Disseldorp]

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

Re: [PATCH] common/rc: drop '-f' parameter from fsck.exfat

[Zorro Lang]

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
> 

Re: [PATCH] common/rc: drop '-f' parameter from fsck.exfat

[Darrick J. Wong]

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
> > 
>