[PATCH 0/5] Misceillaneous ext4 test fix ups

[PATCH 0/5] Misceillaneous ext4 test fix ups

[Theodore Ts'o]

The first patch has been in my tree for a long time, but I had forgotten
to send it upstream.

The last two patches are a rewrite (in response to comments) of
"ext4/054,ext4/055: don't run when using DAX" which I had sent in late
April.

Theodore Ts'o (5):
  ext4/050: support indirect as well as extent mapped journals
  ext4/044: skip test if the file system does not have a journal
  ext4/045: clean up unnecessary arguments to _require_e2fsprogs
  ext4/054: skip test if the dax mount option is enabled
  ext4/055: skip test if dax mount option is used

 common/rc      |  3 +++
 tests/ext4/044 |  6 ++++++
 tests/ext4/045 |  2 +-
 tests/ext4/050 | 58 +++++---------------------------------------------
 tests/ext4/054 |  5 ++---
 tests/ext4/055 |  1 +
 6 files changed, 18 insertions(+), 57 deletions(-)

-- 
2.31.0

[PATCH 1/5] ext4/050: support indirect as well as extent mapped journals

[Theodore Ts'o]

Simplify the test and fix ext4/050 failures when running ext4 without
extents enabled (e.g., in ext3 emulation mode).

Instead of relying on parsing debugfs output's (which varies depending
on whether the journal inode is extent mapped or indirect block
mapped), use debugfs's "cat" command to get the contents of the
journal.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/ext4/050 | 58 +++++---------------------------------------------
 1 file changed, 5 insertions(+), 53 deletions(-)

diff --git a/tests/ext4/050 b/tests/ext4/050
index 79961957..6f93b86d 100755
--- a/tests/ext4/050
+++ b/tests/ext4/050
@@ -22,55 +22,6 @@ _require_command "$DEBUGFS_PROG" debugfs
 checkpoint_journal=$here/src/checkpoint_journal
 _require_test_program "checkpoint_journal"
 
-# convert output from stat<journal_inode> to list of block numbers
-get_journal_extents() {
-	inode_info=$($DEBUGFS_PROG $SCRATCH_DEV -R "stat <8>" 2>> $seqres.full)
-	echo -e "\nJournal info:" >> $seqres.full
-	echo "$inode_info" >> $seqres.full
-
-	extents_line=$(echo "$inode_info" | awk '/EXTENTS:/{ print NR; exit }')
-	get_extents=$(echo "$inode_info" | sed -n "$(($extents_line + 1))"p)
-
-	# get just the physical block numbers
-	get_extents=$(echo "$get_extents" |  perl -pe 's|\(.*?\):||g' | sed -e 's/, /\n/g' | perl -pe 's|(\d+)-(\d+)|\1 \2|g')
-
-	echo "$get_extents"
-}
-
-# checks all extents are zero'd out except for the superblock
-# arg 1: extents (output of get_journal_extents())
-check_extents() {
-	echo -e "\nChecking extents:" >> $seqres.full
-	echo "$1" >> $seqres.full
-
-	super_block="true"
-	echo "$1" | while IFS= read line; do
-		start_block=$(echo $line | cut -f1 -d' ')
-		end_block=$(echo $line | cut -f2 -d' ' -s)
-
-		# if first block of journal, shouldn't be wiped
-		if [ "$super_block" == "true" ]; then
-			super_block="false"
-
-			#if super block only block in this extent, skip extent
-			if [ -z "$end_block" ]; then
-				continue;
-			fi
-			start_block=$(($start_block + 1))
-		fi
-
-		if [ ! -z "$end_block" ]; then
-			blocks=$(($end_block - $start_block + 1))
-		else
-			blocks=1
-		fi
-
-		check=$(od $SCRATCH_DEV --skip-bytes=$(($start_block * $blocksize)) --read-bytes=$(($blocks * $blocksize)) -An -v | sed -e 's/[0 \t\n\r]//g')
-
-		[ ! -z "$check" ] && echo "error" && break
-	done
-}
-
 testdir="${SCRATCH_MNT}/testdir"
 
 _scratch_mkfs_sized $((64 * 1024 * 1024)) >> $seqres.full 2>&1
@@ -93,11 +44,12 @@ sync --file-system $testdir/1
 # call ioctl to checkpoint and zero-fill journal blocks
 $checkpoint_journal $SCRATCH_MNT --erase=zeroout || _fail "ioctl returned error"
 
-extents=$(get_journal_extents)
-
 # check journal blocks zeroed out
-ret=$(check_extents "$extents")
-[ "$ret" = "error" ] && _fail "Journal was not zero-filled"
+$DEBUGFS_PROG $SCRATCH_DEV -R "cat <8>" 2> /dev/null | od >> $seqres.full
+check=$($DEBUGFS_PROG $SCRATCH_DEV -R "cat <8>" 2> /dev/null | \
+	    od --skip-bytes="$blocksize" -An -v | sed -e '/^[0 \t]*$/d')
+
+[ ! -z "$check" ] && _fail "Journal was not zeroed"
 
 _scratch_unmount >> $seqres.full 2>&1
 
-- 
2.31.0

[PATCH 2/5] ext4/044: skip test if the file system does not have a journal

[Theodore Ts'o]

This test mounts the file system using "mount -t ext3"; if the file
system config creates the file system without the jbd2 journal, the
"mount -t ext3" will fail.  So skip this test in that case.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/ext4/044 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/ext4/044 b/tests/ext4/044
index 46e44053..fd27ae2d 100755
--- a/tests/ext4/044
+++ b/tests/ext4/044
@@ -16,11 +16,17 @@ _begin_fstest auto quick
 _supported_fs ext4
 _require_scratch
 _require_test_program "t_get_file_time"
+_require_dumpe2fs
 
 echo "Silence is golden"
 
 echo "Test timestamps with 256 inode size one device $SCRATCH_DEV" >$seqres.full
 _scratch_mkfs -t ext3 -I 256 >> $seqres.full 2>&1
+
+$DUMPE2FS_PROG -h $SCRATCH_DEV 2>> $seqres.full | grep '^Filesystem features' | grep -q has_journal
+if [ $? -ne 0 ]; then
+    _notrun "ext4 file system formatted without a journal"
+fi
 _scratch_mount
 
 # Create file
-- 
2.31.0

[PATCH 3/5] ext4/045: clean up unnecessary arguments to _require_e2fsprogs

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/ext4/045 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ext4/045 b/tests/ext4/045
index ee7c0de3..4f0ad4aa 100755
--- a/tests/ext4/045
+++ b/tests/ext4/045
@@ -22,7 +22,7 @@ _supported_fs ext4
 _require_scratch
 _require_test_program "t_create_short_dirs"
 _require_test_program "t_create_long_dirs"
-_require_dumpe2fs "$DUMPE2FS_PROG" dumpe2fs
+_require_dumpe2fs
 
 echo "Silence is golden"
 
-- 
2.31.0

[PATCH 4/5] ext4/054: skip test if the dax mount option is enabled

[Theodore Ts'o]

The ext4/054 test explicitly creates a file system with a 1k
blocksize.  This can't possibly work on if the dax mount option is
enabled, so change ext4/054 to use _scratch_mkfs_blocksized, and and a
check to _scratch_mkfs_blocksized to _notrun the test if the block
size is less than the page size.

Also remove an unnecessary _require_test declaration since this test
does not use the test device.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/rc      | 3 +++
 tests/ext4/054 | 5 ++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/common/rc b/common/rc
index 9378ff26..3014f5fc 100644
--- a/common/rc
+++ b/common/rc
@@ -1220,6 +1220,9 @@ _scratch_mkfs_blocksized()
 	if ! [[ $blocksize =~ $re ]] ; then
 		_notrun "error: _scratch_mkfs_sized: block size \"$blocksize\" not an integer."
 	fi
+	if [ $blocksize -lt $(get_page_size) ]; then
+		_exclude_scratch_mount_option dax
+	fi
 
 	case $FSTYP in
 	btrfs)
diff --git a/tests/ext4/054 b/tests/ext4/054
index 9a11719f..e23acbb1 100755
--- a/tests/ext4/054
+++ b/tests/ext4/054
@@ -19,7 +19,6 @@ _begin_fstest auto quick dangerous_fuzzers
 
 # real QA test starts here
 _supported_fs ext4
-_require_test
 _require_scratch_nocheck
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "pwrite"
@@ -28,8 +27,8 @@ _require_xfs_io_command "fpunch"
 _require_command "$DEBUGFS_PROG" debugfs
 
 # In order to accurately construct the damaged extent in the following
-# test steps, the blocksize is set to 1024 here
-_scratch_mkfs "-b 1024" > $seqres.full 2>&1
+# test steps, the block size is set to 1024 here
+_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1
 _scratch_mount
 
 TEST_FILE="${SCRATCH_MNT}/testfile"
-- 
2.31.0

[PATCH 5/5] ext4/055: skip test if dax mount option is used

[Theodore Ts'o]

This test creates a 1k block file system with the quota feature
enabled.   As such, it is incompatible with DAX.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/ext4/055 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/ext4/055 b/tests/ext4/055
index 8f466f1b..aa15cfe9 100755
--- a/tests/ext4/055
+++ b/tests/ext4/055
@@ -26,6 +26,7 @@ _require_command "$DEBUGFS_PROG" debugfs
 echo "Silence is golden"
 
 # The 1K blocksize is designed for debugfs.
+_exclude_scratch_mount_option dax
 _scratch_mkfs "-F -O quota -b 1024" > $seqres.full 2>&1
 
 # Start from 0, fill block 1 with 6，replace the original 2.
-- 
2.31.0

Re: [PATCH 1/5] ext4/050: support indirect as well as extent mapped journals

[Zorro Lang]

On Fri, Jun 24, 2022 at 11:07:14PM -0400, Theodore Ts'o wrote:
> Simplify the test and fix ext4/050 failures when running ext4 without
> extents enabled (e.g., in ext3 emulation mode).
> 
> Instead of relying on parsing debugfs output's (which varies depending
> on whether the journal inode is extent mapped or indirect block
> mapped), use debugfs's "cat" command to get the contents of the
> journal.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---

It looks good to me,

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

But as it contains many ext4 specific format things, so I hope to get one more
reviewing from ext4 mail list, before I merge it.

Thanks,
Zorro

>  tests/ext4/050 | 58 +++++---------------------------------------------
>  1 file changed, 5 insertions(+), 53 deletions(-)
> 
> diff --git a/tests/ext4/050 b/tests/ext4/050
> index 79961957..6f93b86d 100755
> --- a/tests/ext4/050
> +++ b/tests/ext4/050
> @@ -22,55 +22,6 @@ _require_command "$DEBUGFS_PROG" debugfs
>  checkpoint_journal=$here/src/checkpoint_journal
>  _require_test_program "checkpoint_journal"
>  
> -# convert output from stat<journal_inode> to list of block numbers
> -get_journal_extents() {
> -	inode_info=$($DEBUGFS_PROG $SCRATCH_DEV -R "stat <8>" 2>> $seqres.full)
> -	echo -e "\nJournal info:" >> $seqres.full
> -	echo "$inode_info" >> $seqres.full
> -
> -	extents_line=$(echo "$inode_info" | awk '/EXTENTS:/{ print NR; exit }')
> -	get_extents=$(echo "$inode_info" | sed -n "$(($extents_line + 1))"p)
> -
> -	# get just the physical block numbers
> -	get_extents=$(echo "$get_extents" |  perl -pe 's|\(.*?\):||g' | sed -e 's/, /\n/g' | perl -pe 's|(\d+)-(\d+)|\1 \2|g')
> -
> -	echo "$get_extents"
> -}
> -
> -# checks all extents are zero'd out except for the superblock
> -# arg 1: extents (output of get_journal_extents())
> -check_extents() {
> -	echo -e "\nChecking extents:" >> $seqres.full
> -	echo "$1" >> $seqres.full
> -
> -	super_block="true"
> -	echo "$1" | while IFS= read line; do
> -		start_block=$(echo $line | cut -f1 -d' ')
> -		end_block=$(echo $line | cut -f2 -d' ' -s)
> -
> -		# if first block of journal, shouldn't be wiped
> -		if [ "$super_block" == "true" ]; then
> -			super_block="false"
> -
> -			#if super block only block in this extent, skip extent
> -			if [ -z "$end_block" ]; then
> -				continue;
> -			fi
> -			start_block=$(($start_block + 1))
> -		fi
> -
> -		if [ ! -z "$end_block" ]; then
> -			blocks=$(($end_block - $start_block + 1))
> -		else
> -			blocks=1
> -		fi
> -
> -		check=$(od $SCRATCH_DEV --skip-bytes=$(($start_block * $blocksize)) --read-bytes=$(($blocks * $blocksize)) -An -v | sed -e 's/[0 \t\n\r]//g')
> -
> -		[ ! -z "$check" ] && echo "error" && break
> -	done
> -}
> -
>  testdir="${SCRATCH_MNT}/testdir"
>  
>  _scratch_mkfs_sized $((64 * 1024 * 1024)) >> $seqres.full 2>&1
> @@ -93,11 +44,12 @@ sync --file-system $testdir/1
>  # call ioctl to checkpoint and zero-fill journal blocks
>  $checkpoint_journal $SCRATCH_MNT --erase=zeroout || _fail "ioctl returned error"
>  
> -extents=$(get_journal_extents)
> -
>  # check journal blocks zeroed out
> -ret=$(check_extents "$extents")
> -[ "$ret" = "error" ] && _fail "Journal was not zero-filled"
> +$DEBUGFS_PROG $SCRATCH_DEV -R "cat <8>" 2> /dev/null | od >> $seqres.full
> +check=$($DEBUGFS_PROG $SCRATCH_DEV -R "cat <8>" 2> /dev/null | \
> +	    od --skip-bytes="$blocksize" -An -v | sed -e '/^[0 \t]*$/d')
> +
> +[ ! -z "$check" ] && _fail "Journal was not zeroed"
>  
>  _scratch_unmount >> $seqres.full 2>&1
>  
> -- 
> 2.31.0
> 

Re: [PATCH 2/5] ext4/044: skip test if the file system does not have a journal

[Zorro Lang]

On Fri, Jun 24, 2022 at 11:07:15PM -0400, Theodore Ts'o wrote:
> This test mounts the file system using "mount -t ext3"; if the file
> system config creates the file system without the jbd2 journal, the
> "mount -t ext3" will fail.  So skip this test in that case.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/ext4/044 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/ext4/044 b/tests/ext4/044
> index 46e44053..fd27ae2d 100755
> --- a/tests/ext4/044
> +++ b/tests/ext4/044
> @@ -16,11 +16,17 @@ _begin_fstest auto quick
>  _supported_fs ext4
>  _require_scratch
>  _require_test_program "t_get_file_time"
> +_require_dumpe2fs
>  
>  echo "Silence is golden"
>  
>  echo "Test timestamps with 256 inode size one device $SCRATCH_DEV" >$seqres.full
>  _scratch_mkfs -t ext3 -I 256 >> $seqres.full 2>&1
> +
> +$DUMPE2FS_PROG -h $SCRATCH_DEV 2>> $seqres.full | grep '^Filesystem features' | grep -q has_journal
> +if [ $? -ne 0 ]; then
> +    _notrun "ext4 file system formatted without a journal"
> +fi

Can _has_metadata_journaling help that?

Thanks,
Zorro

>  _scratch_mount
>  
>  # Create file
> -- 
> 2.31.0
> 

Re: [PATCH 2/5] ext4/044: skip test if the file system does not have a journal

[Zorro Lang]

On Tue, Jun 28, 2022 at 12:28:17PM +0800, Zorro Lang wrote:
> On Fri, Jun 24, 2022 at 11:07:15PM -0400, Theodore Ts'o wrote:
> > This test mounts the file system using "mount -t ext3"; if the file
> > system config creates the file system without the jbd2 journal, the
> > "mount -t ext3" will fail.  So skip this test in that case.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  tests/ext4/044 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/ext4/044 b/tests/ext4/044
> > index 46e44053..fd27ae2d 100755
> > --- a/tests/ext4/044
> > +++ b/tests/ext4/044
> > @@ -16,11 +16,17 @@ _begin_fstest auto quick
> >  _supported_fs ext4
> >  _require_scratch
> >  _require_test_program "t_get_file_time"
> > +_require_dumpe2fs
> >  
> >  echo "Silence is golden"
> >  
> >  echo "Test timestamps with 256 inode size one device $SCRATCH_DEV" >$seqres.full
> >  _scratch_mkfs -t ext3 -I 256 >> $seqres.full 2>&1
> > +
> > +$DUMPE2FS_PROG -h $SCRATCH_DEV 2>> $seqres.full | grep '^Filesystem features' | grep -q has_journal
> > +if [ $? -ne 0 ]; then
> > +    _notrun "ext4 file system formatted without a journal"
> > +fi
> 
> Can _has_metadata_journaling help that?

Or use _require_metadata_journaling directly?

> 
> Thanks,
> Zorro
> 
> >  _scratch_mount
> >  
> >  # Create file
> > -- 
> > 2.31.0
> > 

Re: [PATCH 3/5] ext4/045: clean up unnecessary arguments to _require_e2fsprogs

[Zorro Lang]

On Fri, Jun 24, 2022 at 11:07:16PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/ext4/045 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/ext4/045 b/tests/ext4/045
> index ee7c0de3..4f0ad4aa 100755
> --- a/tests/ext4/045
> +++ b/tests/ext4/045
> @@ -22,7 +22,7 @@ _supported_fs ext4
>  _require_scratch
>  _require_test_program "t_create_short_dirs"
>  _require_test_program "t_create_long_dirs"
> -_require_dumpe2fs "$DUMPE2FS_PROG" dumpe2fs
> +_require_dumpe2fs

Thanks, I think it planned to write "_require_command" :)

This change is good, but the patch subject:
  "ext4/045: clean up unnecessary arguments to _require_e2fsprogs"
is a little weird, dut to it's not changing "_require_e2fsprogs" (and there's
not _require_e2fsprogs).

Thanks,
Zorro

>  
>  echo "Silence is golden"
>  
> -- 
> 2.31.0
> 

Re: [PATCH 4/5] ext4/054: skip test if the dax mount option is enabled

[Zorro Lang]

On Fri, Jun 24, 2022 at 11:07:17PM -0400, Theodore Ts'o wrote:
> The ext4/054 test explicitly creates a file system with a 1k
> blocksize.  This can't possibly work on if the dax mount option is
> enabled, so change ext4/054 to use _scratch_mkfs_blocksized, and and a
> check to _scratch_mkfs_blocksized to _notrun the test if the block
> size is less than the page size.
> 
> Also remove an unnecessary _require_test declaration since this test
> does not use the test device.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---

Looks good to me.

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

>  common/rc      | 3 +++
>  tests/ext4/054 | 5 ++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 9378ff26..3014f5fc 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1220,6 +1220,9 @@ _scratch_mkfs_blocksized()
>  	if ! [[ $blocksize =~ $re ]] ; then
>  		_notrun "error: _scratch_mkfs_sized: block size \"$blocksize\" not an integer."
>  	fi
> +	if [ $blocksize -lt $(get_page_size) ]; then
> +		_exclude_scratch_mount_option dax
> +	fi
>  
>  	case $FSTYP in
>  	btrfs)
> diff --git a/tests/ext4/054 b/tests/ext4/054
> index 9a11719f..e23acbb1 100755
> --- a/tests/ext4/054
> +++ b/tests/ext4/054
> @@ -19,7 +19,6 @@ _begin_fstest auto quick dangerous_fuzzers
>  
>  # real QA test starts here
>  _supported_fs ext4
> -_require_test
>  _require_scratch_nocheck
>  _require_xfs_io_command "falloc"
>  _require_xfs_io_command "pwrite"
> @@ -28,8 +27,8 @@ _require_xfs_io_command "fpunch"
>  _require_command "$DEBUGFS_PROG" debugfs
>  
>  # In order to accurately construct the damaged extent in the following
> -# test steps, the blocksize is set to 1024 here
> -_scratch_mkfs "-b 1024" > $seqres.full 2>&1
> +# test steps, the block size is set to 1024 here
> +_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1
>  _scratch_mount
>  
>  TEST_FILE="${SCRATCH_MNT}/testfile"
> -- 
> 2.31.0
> 

Re: [PATCH 2/5] ext4/044: skip test if the file system does not have a journal

[Theodore Ts'o]

On Tue, Jun 28, 2022 at 12:32:29PM +0800, Zorro Lang wrote:
> > 
> > Can _has_metadata_journaling help that?
> 
> Or use _require_metadata_journaling directly?

Yep, that's just what we need.  I'll fix this in the next version of
these patches.

						- Ted

Re: [PATCH 3/5] ext4/045: clean up unnecessary arguments to _require_e2fsprogs

[Theodore Ts'o]

On Tue, Jun 28, 2022 at 12:37:16PM +0800, Zorro Lang wrote:
> 
> This change is good, but the patch subject:
>   "ext4/045: clean up unnecessary arguments to _require_e2fsprogs"
> is a little weird, dut to it's not changing "_require_e2fsprogs" (and there's
> not _require_e2fsprogs).

I've replaced the commit description with:

ext4/045: _require_dumpe2fs doesn't take any arguments so remove them

	  		    	    	     - Ted