[PATCH v2] common/rc: add repair fsck flag -f for ext4

[PATCH v2] common/rc: add repair fsck flag -f for ext4

[Leah Rumancik]

There is a descrepancy between the fsck flags for ext4 during
filesystem repair and filesystem checking which causes occasional test
failures. In particular, _check_generic_filesystems uses -f for force
checking, but _repair_scratch_fs does not. In some tests, such as
generic/441, we sometimes exit fsck repair early with the filesystem
being deemed "clean" but then _check_generic_filesystems finds issues
during the forced full check. Bringing these flags in sync fixes the
flakes.

Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
---

v2: update to fix for ext2/3 as well

 common/rc | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index daf62c92..ddced1b7 100644
--- a/common/rc
+++ b/common/rc
@@ -1496,19 +1496,24 @@ _repair_scratch_fs()
 	_check_scratch_fs
 	;;
     *)
 	local dev=$SCRATCH_DEV
 	local fstyp=$FSTYP
+	local fsopts=
 	if [ $FSTYP = "overlay" -a -n "$OVL_BASE_SCRATCH_DEV" ]; then
 		_repair_overlay_scratch_fs
 		# Fall through to repair base fs
 		dev=$OVL_BASE_SCRATCH_DEV
 		fstyp=$OVL_BASE_FSTYP
 		_unmount $OVL_BASE_SCRATCH_MNT
 	fi
+	if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then
+		fsopts="-f"
+	fi
+
 	# Let's hope fsck -y suffices...
-	fsck -t $fstyp -y $dev 2>&1
+	fsck -t $fstyp -y ${fsopts} $dev 2>&1
 	local res=$?
 	case $res in
 	$FSCK_OK|$FSCK_NONDESTRUCT|$FSCK_REBOOT)
 		res=0
 		;;
@@ -1548,12 +1553,16 @@ _repair_test_fs()
 	yes | $BTRFS_UTIL_PROG check --repair --force "$TEST_DEV" >> \
 								$tmp.repair 2>&1
 		res=$?
 		;;
 	*)
+		local fsopts=
+		if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then
+			fsopts="-f"
+		fi
 		# Let's hope fsck -y suffices...
-		fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
+		fsck -t $FSTYP -y ${fsopts} $TEST_DEV >$tmp.repair 2>&1
 		res=$?
 		if test "$res" -lt 4 ; then
 			res=0
 		fi
 		;;
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v2] common/rc: add repair fsck flag -f for ext4

[Theodore Ts'o]

On Wed, Jun 25, 2025 at 02:20:22PM -0700, Leah Rumancik wrote:
> There is a descrepancy between the fsck flags for ext4 during
> filesystem repair and filesystem checking which causes occasional test
> failures. In particular, _check_generic_filesystems uses -f for force
> checking, but _repair_scratch_fs does not. In some tests, such as
> generic/441, we sometimes exit fsck repair early with the filesystem
> being deemed "clean" but then _check_generic_filesystems finds issues
> during the forced full check. Bringing these flags in sync fixes the
> flakes.
> 
> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Looks good to me, although I might suggest ammending or just dropping
the comment:

>  	# Let's hope fsck -y suffices...

... since obviously, for ext[234] it wasn't sufficient.   :-)

    	  	     	 	     	    - Ted

Re: [PATCH v2] common/rc: add repair fsck flag -f for ext4

[Zorro Lang]

On Wed, Jun 25, 2025 at 11:51:25PM -0400, Theodore Ts'o wrote:
> On Wed, Jun 25, 2025 at 02:20:22PM -0700, Leah Rumancik wrote:
> > There is a descrepancy between the fsck flags for ext4 during
> > filesystem repair and filesystem checking which causes occasional test
> > failures. In particular, _check_generic_filesystems uses -f for force
> > checking, but _repair_scratch_fs does not. In some tests, such as
> > generic/441, we sometimes exit fsck repair early with the filesystem
> > being deemed "clean" but then _check_generic_filesystems finds issues
> > during the forced full check. Bringing these flags in sync fixes the
> > flakes.
> > 
> > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
> 
> Reviewed-by: Theodore Ts'o <tytso@mit.edu>
> 
> Looks good to me, although I might suggest ammending or just dropping
> the comment:
> 
> >  	# Let's hope fsck -y suffices...
> 
> ... since obviously, for ext[234] it wasn't sufficient.   :-)

Thanks Ted, I'll remove it when I merge this patch :)

Thanks,
Zorro

> 
>     	  	     	 	     	    - Ted
> 

Re: [PATCH v2] common/rc: add repair fsck flag -f for ext4

[Zorro Lang]

On Wed, Jun 25, 2025 at 02:20:22PM -0700, Leah Rumancik wrote:
> There is a descrepancy between the fsck flags for ext4 during
> filesystem repair and filesystem checking which causes occasional test
> failures. In particular, _check_generic_filesystems uses -f for force
> checking, but _repair_scratch_fs does not. In some tests, such as
> generic/441, we sometimes exit fsck repair early with the filesystem
> being deemed "clean" but then _check_generic_filesystems finds issues
> during the forced full check. Bringing these flags in sync fixes the
> flakes.
> 
> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
> ---
> 
> v2: update to fix for ext2/3 as well
> 
>  common/rc | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index daf62c92..ddced1b7 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1496,19 +1496,24 @@ _repair_scratch_fs()
>  	_check_scratch_fs
>  	;;
>      *)
>  	local dev=$SCRATCH_DEV
>  	local fstyp=$FSTYP
> +	local fsopts=
>  	if [ $FSTYP = "overlay" -a -n "$OVL_BASE_SCRATCH_DEV" ]; then
>  		_repair_overlay_scratch_fs
>  		# Fall through to repair base fs
>  		dev=$OVL_BASE_SCRATCH_DEV
>  		fstyp=$OVL_BASE_FSTYP
>  		_unmount $OVL_BASE_SCRATCH_MNT
>  	fi
> +	if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then

I think you can use [[ "$FSTYP" == ext[234] ]] directly.

> +		fsopts="-f"
> +	fi
> +
>  	# Let's hope fsck -y suffices...
> -	fsck -t $fstyp -y $dev 2>&1
> +	fsck -t $fstyp -y ${fsopts} $dev 2>&1
>  	local res=$?
>  	case $res in
>  	$FSCK_OK|$FSCK_NONDESTRUCT|$FSCK_REBOOT)
>  		res=0
>  		;;
> @@ -1548,12 +1553,16 @@ _repair_test_fs()
>  	yes | $BTRFS_UTIL_PROG check --repair --force "$TEST_DEV" >> \
>  								$tmp.repair 2>&1
>  		res=$?
>  		;;
>  	*)
> +		local fsopts=
> +		if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then

Same here

Others look good to me. If you agree, I can help to do this change
when I merge it :)

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

> +			fsopts="-f"
> +		fi
>  		# Let's hope fsck -y suffices...
> -		fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
> +		fsck -t $FSTYP -y ${fsopts} $TEST_DEV >$tmp.repair 2>&1
>  		res=$?
>  		if test "$res" -lt 4 ; then
>  			res=0
>  		fi
>  		;;
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 
> 

Re: [PATCH v2] common/rc: add repair fsck flag -f for ext4

[Leah Rumancik]

Sounds great, thanks Zorro!

- leah

On Thu, Jun 26, 2025 at 12:53 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Wed, Jun 25, 2025 at 02:20:22PM -0700, Leah Rumancik wrote:
> > There is a descrepancy between the fsck flags for ext4 during
> > filesystem repair and filesystem checking which causes occasional test
> > failures. In particular, _check_generic_filesystems uses -f for force
> > checking, but _repair_scratch_fs does not. In some tests, such as
> > generic/441, we sometimes exit fsck repair early with the filesystem
> > being deemed "clean" but then _check_generic_filesystems finds issues
> > during the forced full check. Bringing these flags in sync fixes the
> > flakes.
> >
> > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
> > ---
> >
> > v2: update to fix for ext2/3 as well
> >
> >  common/rc | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index daf62c92..ddced1b7 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1496,19 +1496,24 @@ _repair_scratch_fs()
> >       _check_scratch_fs
> >       ;;
> >      *)
> >       local dev=$SCRATCH_DEV
> >       local fstyp=$FSTYP
> > +     local fsopts=
> >       if [ $FSTYP = "overlay" -a -n "$OVL_BASE_SCRATCH_DEV" ]; then
> >               _repair_overlay_scratch_fs
> >               # Fall through to repair base fs
> >               dev=$OVL_BASE_SCRATCH_DEV
> >               fstyp=$OVL_BASE_FSTYP
> >               _unmount $OVL_BASE_SCRATCH_MNT
> >       fi
> > +     if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then
>
> I think you can use [[ "$FSTYP" == ext[234] ]] directly.
>
> > +             fsopts="-f"
> > +     fi
> > +
> >       # Let's hope fsck -y suffices...
> > -     fsck -t $fstyp -y $dev 2>&1
> > +     fsck -t $fstyp -y ${fsopts} $dev 2>&1
> >       local res=$?
> >       case $res in
> >       $FSCK_OK|$FSCK_NONDESTRUCT|$FSCK_REBOOT)
> >               res=0
> >               ;;
> > @@ -1548,12 +1553,16 @@ _repair_test_fs()
> >       yes | $BTRFS_UTIL_PROG check --repair --force "$TEST_DEV" >> \
> >                                                               $tmp.repair 2>&1
> >               res=$?
> >               ;;
> >       *)
> > +             local fsopts=
> > +             if [ $FSTYP = "ext4" ] || [ $FSTYP = "ext3" ] || [ $FSTYP = "ext2" ]; then
>
> Same here
>
> Others look good to me. If you agree, I can help to do this change
> when I merge it :)
>
> Reviewed-by: Zorro Lang <zlang@redhat.com>
>
> > +                     fsopts="-f"
> > +             fi
> >               # Let's hope fsck -y suffices...
> > -             fsck -t $FSTYP -y $TEST_DEV >$tmp.repair 2>&1
> > +             fsck -t $FSTYP -y ${fsopts} $TEST_DEV >$tmp.repair 2>&1
> >               res=$?
> >               if test "$res" -lt 4 ; then
> >                       res=0
> >               fi
> >               ;;
> > --
> > 2.50.0.727.gbf7dc18ff4-goog
> >
> >
>