[PATCH v3 0/3] fstests: Fix checking of UDF filesystems

[PATCH v3 0/3] fstests: Fix checking of UDF filesystems

[Jan Kara]

Hello,

these two patches fix the usage of udf_test program in _check_udf_filesystem
so that it can reliably check udf filesystems. It provides it proper arguments
and also unmounts the filesystem prior to running the check.

Changes since v2:
* Include another patch adding better blocksize support to _scratch_mkfs_sized
  and _scratch_mkfs_blocksized

Changes since v1:
* Added Reviewed-by tag
* Accept also --blocksize version of the option

								Honza
Previous versions:
Link: http://lore.kernel.org/r/20230119104618.28701-1-jack@suse.cz # v1
Link: http://lore.kernel.org/r/20230120154944.25573-1-jack@suse.cz # v2

[PATCH v3 1/3] common: Provide blocksize and ecclength to udf fsck

[Jan Kara]

udf_test program used for verifying filesystem is not able to determine
filesystem blocksize. Provide it in the options together with disabling
ecclength as it is not used on harddrives.

Reviewed-by: David Disseldorp <ddiss@suse.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 common/rc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 8060c03b7d18..bf3effa4c1ed 100644
--- a/common/rc
+++ b/common/rc
@@ -3075,9 +3075,13 @@ _check_udf_filesystem()
     fi
 
     local device=$1
-    local opt_arg=""
+    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
+    if [ -z "$blksz" ]; then
+	blksz=512
+    fi
+    local opt_arg="-ecclength 1 -blocksize $blksz"
     if [ $# -eq 2 ]; then
-	opt_arg="-lastvalidblock $(( $2 - 1 ))"
+	opt_arg+=" -lastvalidblock $(( $2 - 1 ))"
     fi
 
     rm -f $seqres.checkfs
-- 
2.35.3

[PATCH v3 2/3] common: Unmount udf filesystem prior checking

[Jan Kara]

_check_udf_filesystem forgot to unmount the filesystem prior to checking
it. That was leading to check failures.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 common/rc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/common/rc b/common/rc
index bf3effa4c1ed..7f80afe7aeec 100644
--- a/common/rc
+++ b/common/rc
@@ -3074,6 +3074,12 @@ _check_udf_filesystem()
 	return
     fi
 
+    # Is the filesystem mounted?
+    local type=`_fs_type $device`
+    if [ "$type" = "$FSTYP" ]; then
+	local mountpoint=`_umount_or_remount_ro $device`
+    fi
+
     local device=$1
     local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
     if [ -z "$blksz" ]; then
@@ -3090,6 +3096,10 @@ _check_udf_filesystem()
 	_udf_test_known_error_filter | \
 	grep -E -iv "Error count:.*[0-9]+.*total occurrences:.*[0-9]+|Warning count:.*[0-9]+.*total occurrences:.*[0-9]+" && \
         echo "Warning UDF Verifier reported errors see $seqres.checkfs." && return 1
+    # Remount the filesystem
+    if [ "$type" = "$FSTYP" ]; then
+	_mount_or_remount_rw "$MOUNT_OPTIONS" $device $mountpoint
+    fi
     return 0
 }
 
-- 
2.35.3

Re: [PATCH v3 2/3] common: Unmount udf filesystem prior checking

[Zorro Lang]

On Tue, Jan 31, 2023 at 01:39:58PM +0100, Jan Kara wrote:
> _check_udf_filesystem forgot to unmount the filesystem prior to checking
> it. That was leading to check failures.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---

Looks good to me,

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

>  common/rc | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index bf3effa4c1ed..7f80afe7aeec 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3074,6 +3074,12 @@ _check_udf_filesystem()
>  	return
>      fi
>  
> +    # Is the filesystem mounted?
> +    local type=`_fs_type $device`
> +    if [ "$type" = "$FSTYP" ]; then
> +	local mountpoint=`_umount_or_remount_ro $device`
> +    fi
> +
>      local device=$1
>      local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
>      if [ -z "$blksz" ]; then
> @@ -3090,6 +3096,10 @@ _check_udf_filesystem()
>  	_udf_test_known_error_filter | \
>  	grep -E -iv "Error count:.*[0-9]+.*total occurrences:.*[0-9]+|Warning count:.*[0-9]+.*total occurrences:.*[0-9]+" && \
>          echo "Warning UDF Verifier reported errors see $seqres.checkfs." && return 1
> +    # Remount the filesystem
> +    if [ "$type" = "$FSTYP" ]; then
> +	_mount_or_remount_rw "$MOUNT_OPTIONS" $device $mountpoint
> +    fi
>      return 0
>  }
>  
> -- 
> 2.35.3
> 

[PATCH v3 3/3] common: Improve blocksize support for udf

[Jan Kara]

Add better support for blocksize selection in _scratch_mkfs_sized
(accept another variant of mkfs options, select correct default block
size if not specified). Also add blocksize selection support to
_scratch_mkfs_blocksized.

For _check_udf_filesystem to keep working when blocksize is not
specified in MKFS_OPTIONS, add detection of blocksize from a mounted
filesystem.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 common/rc | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/common/rc b/common/rc
index 7f80afe7aeec..e49918c9deab 100644
--- a/common/rc
+++ b/common/rc
@@ -922,9 +922,15 @@ _scratch_mkfs_sized()
 	btrfs)
 		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'`
 		;;
-	ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs)
+	ext2|ext3|ext4|ext4dev|reiser4|ocfs2|reiserfs)
 		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
 		;;
+	udf)
+		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
+		if [ -z "$def_blksz" ]; then
+			def_blksz=512
+		fi
+		;;
 	jfs)
 		def_blksz=4096
 		;;
@@ -1116,6 +1122,9 @@ _scratch_mkfs_blocksized()
 		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS --block_size=$blocksize \
 								$SCRATCH_DEV
 		;;
+	udf)
+		_scratch_mkfs -b $blocksize
+		;;
 	*)
 		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_blocksized"
 		;;
@@ -3074,17 +3083,18 @@ _check_udf_filesystem()
 	return
     fi
 
+    local device=$1
+    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
+    if [ -z "$blksz" ]; then
+	blksz=512
+    fi
     # Is the filesystem mounted?
     local type=`_fs_type $device`
     if [ "$type" = "$FSTYP" ]; then
+	blksz=`blockdev --getbsz $device`
 	local mountpoint=`_umount_or_remount_ro $device`
     fi
 
-    local device=$1
-    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
-    if [ -z "$blksz" ]; then
-	blksz=512
-    fi
     local opt_arg="-ecclength 1 -blocksize $blksz"
     if [ $# -eq 2 ]; then
 	opt_arg+=" -lastvalidblock $(( $2 - 1 ))"
-- 
2.35.3

Re: [PATCH v3 3/3] common: Improve blocksize support for udf

[Zorro Lang]

On Tue, Jan 31, 2023 at 01:39:59PM +0100, Jan Kara wrote:
> Add better support for blocksize selection in _scratch_mkfs_sized
> (accept another variant of mkfs options, select correct default block
> size if not specified). Also add blocksize selection support to
> _scratch_mkfs_blocksized.
> 
> For _check_udf_filesystem to keep working when blocksize is not
> specified in MKFS_OPTIONS, add detection of blocksize from a mounted
> filesystem.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---

This patch looks good to me, thanks for doing this for udf filesystem :)

It's been long time, no patches from udf list to fstests list. For exmaple,
The udf/102 missed the _setup_udf_scratchdir() helper (be removed) for long
time, if you're working on udf testing, you might be interested in it.
Anyway, I'll merge this patchset this week.

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

Thanks,
Zorro

>  common/rc | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f80afe7aeec..e49918c9deab 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -922,9 +922,15 @@ _scratch_mkfs_sized()
>  	btrfs)
>  		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'`
>  		;;
> -	ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs)
> +	ext2|ext3|ext4|ext4dev|reiser4|ocfs2|reiserfs)
>  		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
>  		;;
> +	udf)
> +		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
> +		if [ -z "$def_blksz" ]; then
> +			def_blksz=512
> +		fi
> +		;;
>  	jfs)
>  		def_blksz=4096
>  		;;
> @@ -1116,6 +1122,9 @@ _scratch_mkfs_blocksized()
>  		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS --block_size=$blocksize \
>  								$SCRATCH_DEV
>  		;;
> +	udf)
> +		_scratch_mkfs -b $blocksize
> +		;;
>  	*)
>  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_blocksized"
>  		;;
> @@ -3074,17 +3083,18 @@ _check_udf_filesystem()
>  	return
>      fi
>  
> +    local device=$1
> +    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
> +    if [ -z "$blksz" ]; then
> +	blksz=512
> +    fi
>      # Is the filesystem mounted?
>      local type=`_fs_type $device`
>      if [ "$type" = "$FSTYP" ]; then
> +	blksz=`blockdev --getbsz $device`
>  	local mountpoint=`_umount_or_remount_ro $device`
>      fi
>  
> -    local device=$1
> -    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*(-b|--blocksize)[ =]?+([0-9]+).*/\2/p'`
> -    if [ -z "$blksz" ]; then
> -	blksz=512
> -    fi
>      local opt_arg="-ecclength 1 -blocksize $blksz"
>      if [ $# -eq 2 ]; then
>  	opt_arg+=" -lastvalidblock $(( $2 - 1 ))"
> -- 
> 2.35.3
> 

Re: [PATCH v3 3/3] common: Improve blocksize support for udf

[Jan Kara]

On Mon 06-02-23 19:12:35, Zorro Lang wrote:
> On Tue, Jan 31, 2023 at 01:39:59PM +0100, Jan Kara wrote:
> > Add better support for blocksize selection in _scratch_mkfs_sized
> > (accept another variant of mkfs options, select correct default block
> > size if not specified). Also add blocksize selection support to
> > _scratch_mkfs_blocksized.
> > 
> > For _check_udf_filesystem to keep working when blocksize is not
> > specified in MKFS_OPTIONS, add detection of blocksize from a mounted
> > filesystem.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> This patch looks good to me, thanks for doing this for udf filesystem :)
> 
> It's been long time, no patches from udf list to fstests list. For exmaple,
> The udf/102 missed the _setup_udf_scratchdir() helper (be removed) for long
> time, if you're working on udf testing, you might be interested in it.

So I did some larger UDF kernel changes in this cycle so I also found some
time to do more testing which has shown all the problems. My plan is to run
fstests for UDF more often and slowly reduce the amount of failing tests
(so far I was more or less happy that the basic functionality looked solid
and the kernel didn't crash during whole fstests run ;).

> Anyway, I'll merge this patchset this week.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR