[PATCH v2] common/rc: drop 'fsck -f' parameter from _repair_test_fs

[PATCH v2] common/rc: drop 'fsck -f' parameter from _repair_test_fs

[David Disseldorp]

The '-f' parameter is fsck.ext# specific, where it's documented to:
  Force checking even if filesystem is marked clean

_repair_test_fs() is only called on _check_test_fs() failure, so
dropping the parameter should be possible without changing ext#
behaviour.
Doing so fixes _repair_test_fs() on exfat, where fsck.exfat doesn't
support '-f'.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
v2: drop -f from default case instead of splitting out exfat case

 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 5c4429ed..66d270ac 100644
--- a/common/rc
+++ b/common/rc
@@ -1231,7 +1231,7 @@ _repair_test_fs()
 		;;
 	*)
 		# Let's hope fsck -y suffices...
-		fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1
+		fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
 		res=$?
 		if test "$res" -lt 4 ; then
 			res=0
-- 
2.35.3

Re: [PATCH v2] common/rc: drop 'fsck -f' parameter from _repair_test_fs

[Zorro Lang]

On Wed, Aug 16, 2023 at 12:33:30PM +0200, David Disseldorp wrote:
> The '-f' parameter is fsck.ext# specific, where it's documented to:
>   Force checking even if filesystem is marked clean
> 
> _repair_test_fs() is only called on _check_test_fs() failure, so
> dropping the parameter should be possible without changing ext#
> behaviour.
> Doing so fixes _repair_test_fs() on exfat, where fsck.exfat doesn't
> support '-f'.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> v2: drop -f from default case instead of splitting out exfat case

This version is good to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

I remembered you hope to add a btrfs branch to _repair_scratch_fs and
_repair_test_fs [1]. Is that still in your plan? Anand, could you provide
more suggestions about that?

[1]
https://lore.kernel.org/fstests/20230808091454.4skdyjnaxjqa7zyi@zlang-mailbox/

> 
>  common/rc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 5c4429ed..66d270ac 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1231,7 +1231,7 @@ _repair_test_fs()
>  		;;
>  	*)
>  		# Let's hope fsck -y suffices...
> -		fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1
> +		fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
>  		res=$?
>  		if test "$res" -lt 4 ; then
>  			res=0
> -- 
> 2.35.3
> 

Re: [PATCH v2] common/rc: drop 'fsck -f' parameter from _repair_test_fs

[David Disseldorp]

On Wed, 16 Aug 2023 22:55:43 +0800, Zorro Lang wrote:

> On Wed, Aug 16, 2023 at 12:33:30PM +0200, David Disseldorp wrote:
> > The '-f' parameter is fsck.ext# specific, where it's documented to:
> >   Force checking even if filesystem is marked clean
> > 
> > _repair_test_fs() is only called on _check_test_fs() failure, so
> > dropping the parameter should be possible without changing ext#
> > behaviour.
> > Doing so fixes _repair_test_fs() on exfat, where fsck.exfat doesn't
> > support '-f'.
> > 
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> > v2: drop -f from default case instead of splitting out exfat case  
> 
> This version is good to me.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks.

> I remembered you hope to add a btrfs branch to _repair_scratch_fs and
> _repair_test_fs [1]. Is that still in your plan? Anand, could you provide
> more suggestions about that?

Yes, that's still my plan, but I was hoping to get some more input on
whether "btrfs check --repair ..." is sufficient before proposing the
change.

Cheers, David

Re: [PATCH v2] common/rc: drop 'fsck -f' parameter from _repair_test_fs

[Anand Jain]

On 16/8/23 22:55, Zorro Lang wrote:
> On Wed, Aug 16, 2023 at 12:33:30PM +0200, David Disseldorp wrote:
>> The '-f' parameter is fsck.ext# specific, where it's documented to:
>>    Force checking even if filesystem is marked clean
>>
>> _repair_test_fs() is only called on _check_test_fs() failure, so
>> dropping the parameter should be possible without changing ext#
>> behaviour.
>> Doing so fixes _repair_test_fs() on exfat, where fsck.exfat doesn't
>> support '-f'.
>>
>> Signed-off-by: David Disseldorp <ddiss@suse.de>
>> ---
>> v2: drop -f from default case instead of splitting out exfat case
> 
> This version is good to me.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> I remembered you hope to add a btrfs branch to _repair_scratch_fs and
> _repair_test_fs [1]. Is that still in your plan? Anand, could you provide
> more suggestions about that?
> 
> [1]
> https://lore.kernel.org/fstests/20230808091454.4skdyjnaxjqa7zyi@zlang-mailbox/

Thanks for bringing to my notice.

The reason fstyp=btrfs has not detected this missing part so far is
that 'fsck -t btrfs' returns 0, along with a message to use 'btrfs
check', which means repair is never invoked for the 'fstyp=btrfs'.

The appropriate repair command to use here is 'btrfs check --repair'.
However, it would be better to address this in a separate patch
as I guess some tests may fail. I will send a patch for it.

Thanks, Anand


> 
>>
>>   common/rc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 5c4429ed..66d270ac 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1231,7 +1231,7 @@ _repair_test_fs()
>>   		;;
>>   	*)
>>   		# Let's hope fsck -y suffices...
>> -		fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1
>> +		fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
>>   		res=$?
>>   		if test "$res" -lt 4 ; then
>>   			res=0
>> -- 
>> 2.35.3
>>
>