[PATCHSET 0/3] fstests: random fixes for v2023.03.26

[PATCHSET 0/3] fstests: random fixes for v2023.03.26

[Darrick J. Wong]

Hi all,

Here's the usual odd fixes for fstests.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 common/punch      |    8 ++++++++
 common/rc         |   15 +++++++++++++++
 common/report     |    2 +-
 common/xfs        |   11 +++++++++++
 tests/generic/251 |    2 +-
 tests/generic/260 |    2 +-
 6 files changed, 37 insertions(+), 3 deletions(-)

[PATCH 1/3] generic/{251,260}: compute maximum fitrim offset

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

FITRIM is a bizarre ioctl.  Callers are allowed to pass in "start" and
"length" parameters, which are clearly some kind of range argument.  No
means is provided to discover the minimum or maximum range.  Although
regular userspace programs default to (start=0, length=-1ULL), this test
tries to exercise different parameters.

However, the test assumes that the "size" column returned by the df
command is the maximum value supported by the FITRIM command, and is
surprised if the number of bytes trimmed by (start=0, length=-1ULL) is
larger than this size quantity.

This is completely wrong on XFS with realtime volumes, because the
statfs output (which is what df reports) will reflect the realtime
volume if the directory argument is a realtime file or a directory
flagged with rtinherit.  This is trivially reproducible by configuring a
rt volume that is much larger than the data volume, setting rtinherit on
the root dir at mkfs time, and running either of these tests.

Refactor the open-coded df logic so that we can determine the value
programmatically for XFS.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/rc         |   15 +++++++++++++++
 common/xfs        |   11 +++++++++++
 tests/generic/251 |    2 +-
 tests/generic/260 |    2 +-
 4 files changed, 28 insertions(+), 2 deletions(-)


diff --git a/common/rc b/common/rc
index 90749343f3..228982fa4d 100644
--- a/common/rc
+++ b/common/rc
@@ -3927,6 +3927,21 @@ _require_batched_discard()
 	fi
 }
 
+# Given a mountpoint and the device associated with that mountpoint, return the
+# maximum start offset that the FITRIM command will accept, in units of 1024
+# byte blocks.
+_discard_max_offset_kb()
+{
+	case "$FSTYP" in
+	xfs)
+		_xfs_discard_max_offset_kb "$1"
+		;;
+	*)
+		$DF_PROG -k | grep "$1" | grep "$2" | awk '{print $3}'
+		;;
+	esac
+}
+
 _require_dumpe2fs()
 {
 	if [ -z "$DUMPE2FS_PROG" ]; then
diff --git a/common/xfs b/common/xfs
index e8e4832cea..a6c82fc6c7 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1783,3 +1783,14 @@ _require_xfs_scratch_atomicswap()
 		_notrun "atomicswap dependencies not supported by scratch filesystem type: $FSTYP"
 	_scratch_unmount
 }
+
+# Return the maximum start offset that the FITRIM command will accept, in units
+# of 1024 byte blocks.
+_xfs_discard_max_offset_kb()
+{
+	local path="$1"
+	local blksz="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.bsize" | cut -d ' ' -f 3)"
+	local dblks="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.datablocks" | cut -d ' ' -f 3)"
+
+	echo "$((dblks * blksz / 1024))"
+}
diff --git a/tests/generic/251 b/tests/generic/251
index 2a271cd126..8ee74980cc 100755
--- a/tests/generic/251
+++ b/tests/generic/251
@@ -71,7 +71,7 @@ _guess_max_minlen()
 fstrim_loop()
 {
 	trap "_destroy_fstrim; exit \$status" 2 15
-	fsize=$($DF_PROG | grep $SCRATCH_MNT | grep $SCRATCH_DEV  | awk '{print $3}')
+	fsize=$(_discard_max_offset_kb "$SCRATCH_MNT" "$SCRATCH_DEV")
 	mmlen=$(_guess_max_minlen)
 
 	while true ; do
diff --git a/tests/generic/260 b/tests/generic/260
index 2f653b4af2..08fde46873 100755
--- a/tests/generic/260
+++ b/tests/generic/260
@@ -27,7 +27,7 @@ _scratch_mount
 
 _require_batched_discard $SCRATCH_MNT
 
-fssize=$($DF_PROG -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $3}')
+fssize=$(_discard_max_offset_kb "$SCRATCH_MNT" "$SCRATCH_DEV")
 
 beyond_eofs=$(_math "$fssize*2048")
 max_64bit=$(_math "2^64 - 1")

Re: [PATCH 1/3] generic/{251,260}: compute maximum fitrim offset

[David Disseldorp]

On Tue, 28 Mar 2023 17:58:10 -0700, Darrick J. Wong wrote:

> From: Darrick J. Wong <djwong@kernel.org>
> 
> FITRIM is a bizarre ioctl.  Callers are allowed to pass in "start" and
> "length" parameters, which are clearly some kind of range argument.  No
> means is provided to discover the minimum or maximum range.  Although
> regular userspace programs default to (start=0, length=-1ULL), this test
> tries to exercise different parameters.
> 
> However, the test assumes that the "size" column returned by the df
> command is the maximum value supported by the FITRIM command, and is
> surprised if the number of bytes trimmed by (start=0, length=-1ULL) is
> larger than this size quantity.
> 
> This is completely wrong on XFS with realtime volumes, because the
> statfs output (which is what df reports) will reflect the realtime
> volume if the directory argument is a realtime file or a directory
> flagged with rtinherit.  This is trivially reproducible by configuring a
> rt volume that is much larger than the data volume, setting rtinherit on
> the root dir at mkfs time, and running either of these tests.
> 
> Refactor the open-coded df logic so that we can determine the value
> programmatically for XFS.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/rc         |   15 +++++++++++++++
>  common/xfs        |   11 +++++++++++
>  tests/generic/251 |    2 +-
>  tests/generic/260 |    2 +-
>  4 files changed, 28 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/common/rc b/common/rc
> index 90749343f3..228982fa4d 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3927,6 +3927,21 @@ _require_batched_discard()
>  	fi
>  }
>  
> +# Given a mountpoint and the device associated with that mountpoint, return the
> +# maximum start offset that the FITRIM command will accept, in units of 1024
> +# byte blocks.
> +_discard_max_offset_kb()
> +{
> +	case "$FSTYP" in
> +	xfs)
> +		_xfs_discard_max_offset_kb "$1"
> +		;;
> +	*)
> +		$DF_PROG -k | grep "$1" | grep "$2" | awk '{print $3}'
> +		;;

Might as well fix it to properly match full paths, e.g.
  $DF_PROG -k|awk '$1 == "'$dev'" && $7 == "'$mnt'" { print $3 }'

With this:
   Reviewed-by: David Disseldorp <ddiss@suse.de>

One other minor suggestion below...

> +	esac
> +}
> +
>  _require_dumpe2fs()
>  {
>  	if [ -z "$DUMPE2FS_PROG" ]; then
> diff --git a/common/xfs b/common/xfs
> index e8e4832cea..a6c82fc6c7 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1783,3 +1783,14 @@ _require_xfs_scratch_atomicswap()
>  		_notrun "atomicswap dependencies not supported by scratch filesystem type: $FSTYP"
>  	_scratch_unmount
>  }
> +
> +# Return the maximum start offset that the FITRIM command will accept, in units
> +# of 1024 byte blocks.
> +_xfs_discard_max_offset_kb()
> +{
> +	local path="$1"
> +	local blksz="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.bsize" | cut -d ' ' -f 3)"
> +	local dblks="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.datablocks" | cut -d ' ' -f 3)"
> +
> +	echo "$((dblks * blksz / 1024))"

This could be simplified a little:
 $XFS_IO_PROG -c 'statfs' "$path" \
   | awk '{g[$1] = $3} END {print (g["geom.bsize"] * g["geom.datablocks"] / 1024)}'

> +}

Re: [PATCH 1/3] generic/{251,260}: compute maximum fitrim offset

[Darrick J. Wong]

On Wed, Mar 29, 2023 at 12:39:17PM +0200, David Disseldorp wrote:
> On Tue, 28 Mar 2023 17:58:10 -0700, Darrick J. Wong wrote:
> 
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > FITRIM is a bizarre ioctl.  Callers are allowed to pass in "start" and
> > "length" parameters, which are clearly some kind of range argument.  No
> > means is provided to discover the minimum or maximum range.  Although
> > regular userspace programs default to (start=0, length=-1ULL), this test
> > tries to exercise different parameters.
> > 
> > However, the test assumes that the "size" column returned by the df
> > command is the maximum value supported by the FITRIM command, and is
> > surprised if the number of bytes trimmed by (start=0, length=-1ULL) is
> > larger than this size quantity.
> > 
> > This is completely wrong on XFS with realtime volumes, because the
> > statfs output (which is what df reports) will reflect the realtime
> > volume if the directory argument is a realtime file or a directory
> > flagged with rtinherit.  This is trivially reproducible by configuring a
> > rt volume that is much larger than the data volume, setting rtinherit on
> > the root dir at mkfs time, and running either of these tests.
> > 
> > Refactor the open-coded df logic so that we can determine the value
> > programmatically for XFS.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/rc         |   15 +++++++++++++++
> >  common/xfs        |   11 +++++++++++
> >  tests/generic/251 |    2 +-
> >  tests/generic/260 |    2 +-
> >  4 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/common/rc b/common/rc
> > index 90749343f3..228982fa4d 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3927,6 +3927,21 @@ _require_batched_discard()
> >  	fi
> >  }
> >  
> > +# Given a mountpoint and the device associated with that mountpoint, return the
> > +# maximum start offset that the FITRIM command will accept, in units of 1024
> > +# byte blocks.
> > +_discard_max_offset_kb()
> > +{
> > +	case "$FSTYP" in
> > +	xfs)
> > +		_xfs_discard_max_offset_kb "$1"
> > +		;;
> > +	*)
> > +		$DF_PROG -k | grep "$1" | grep "$2" | awk '{print $3}'
> > +		;;
> 
> Might as well fix it to properly match full paths, e.g.
>   $DF_PROG -k|awk '$1 == "'$dev'" && $7 == "'$mnt'" { print $3 }'

I think I could simplify that even more to:

$DF_PROG -k | awk -v dev="$dev" -v mnt="$mnt" '$1 == dev && $7 == mnt {print $3}'

> With this:
>    Reviewed-by: David Disseldorp <ddiss@suse.de>

Thanks!

> One other minor suggestion below...
> 
> > +	esac
> > +}
> > +
> >  _require_dumpe2fs()
> >  {
> >  	if [ -z "$DUMPE2FS_PROG" ]; then
> > diff --git a/common/xfs b/common/xfs
> > index e8e4832cea..a6c82fc6c7 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -1783,3 +1783,14 @@ _require_xfs_scratch_atomicswap()
> >  		_notrun "atomicswap dependencies not supported by scratch filesystem type: $FSTYP"
> >  	_scratch_unmount
> >  }
> > +
> > +# Return the maximum start offset that the FITRIM command will accept, in units
> > +# of 1024 byte blocks.
> > +_xfs_discard_max_offset_kb()
> > +{
> > +	local path="$1"
> > +	local blksz="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.bsize" | cut -d ' ' -f 3)"
> > +	local dblks="$($XFS_IO_PROG -c 'statfs' "$path" | grep "geom.datablocks" | cut -d ' ' -f 3)"
> > +
> > +	echo "$((dblks * blksz / 1024))"
> 
> This could be simplified a little:
>  $XFS_IO_PROG -c 'statfs' "$path" \
>    | awk '{g[$1] = $3} END {print (g["geom.bsize"] * g["geom.datablocks"] / 1024)}'

Oooh, I like this better.  Thanks for the suggestion!

--D

> 
> > +}

[PATCH v1.1 1/3] generic/{251,260}: compute maximum fitrim offset

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

FITRIM is a bizarre ioctl.  Callers are allowed to pass in "start" and
"length" parameters, which are clearly some kind of range argument.  No
means is provided to discover the minimum or maximum range.  Although
regular userspace programs default to (start=0, length=-1ULL), this test
tries to exercise different parameters.

However, the test assumes that the "size" column returned by the df
command is the maximum value supported by the FITRIM command, and is
surprised if the number of bytes trimmed by (start=0, length=-1ULL) is
larger than this size quantity.

This is completely wrong on XFS with realtime volumes, because the
statfs output (which is what df reports) will reflect the realtime
volume if the directory argument is a realtime file or a directory
flagged with rtinherit.  This is trivially reproducible by configuring a
rt volume that is much larger than the data volume, setting rtinherit on
the root dir at mkfs time, and running either of these tests.

Refactor the open-coded df logic so that we can determine the value
programmatically for XFS.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: David Disseldorp <ddiss@suse.de>
---
v1.1: tighten up the awk usage
---
 common/rc         |   15 +++++++++++++++
 common/xfs        |    8 ++++++++
 tests/generic/251 |    2 +-
 tests/generic/260 |    2 +-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 90749343f3..563d2978a7 100644
--- a/common/rc
+++ b/common/rc
@@ -3927,6 +3927,21 @@ _require_batched_discard()
 	fi
 }
 
+# Given a mountpoint and the device associated with that mountpoint, return the
+# maximum start offset that the FITRIM command will accept, in units of 1024
+# byte blocks.
+_discard_max_offset_kb()
+{
+	case "$FSTYP" in
+	xfs)
+		_xfs_discard_max_offset_kb "$1"
+		;;
+	*)
+		$DF_PROG -k | awk -v dev="$2" -v mnt="$1" '$1 == dev && $7 == mnt { print $3 }'
+		;;
+	esac
+}
+
 _require_dumpe2fs()
 {
 	if [ -z "$DUMPE2FS_PROG" ]; then
diff --git a/common/xfs b/common/xfs
index e8e4832cea..c558e940cd 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1783,3 +1783,11 @@ _require_xfs_scratch_atomicswap()
 		_notrun "atomicswap dependencies not supported by scratch filesystem type: $FSTYP"
 	_scratch_unmount
 }
+
+# Return the maximum start offset that the FITRIM command will accept, in units
+# of 1024 byte blocks.
+_xfs_discard_max_offset_kb()
+{
+	$XFS_IO_PROG -c 'statfs' "$1" | \
+		awk '{g[$1] = $3} END {print (g["geom.bsize"] * g["geom.datablocks"] / 1024)}'
+}
diff --git a/tests/generic/251 b/tests/generic/251
index 2a271cd126..8ee74980cc 100755
--- a/tests/generic/251
+++ b/tests/generic/251
@@ -71,7 +71,7 @@ _guess_max_minlen()
 fstrim_loop()
 {
 	trap "_destroy_fstrim; exit \$status" 2 15
-	fsize=$($DF_PROG | grep $SCRATCH_MNT | grep $SCRATCH_DEV  | awk '{print $3}')
+	fsize=$(_discard_max_offset_kb "$SCRATCH_MNT" "$SCRATCH_DEV")
 	mmlen=$(_guess_max_minlen)
 
 	while true ; do
diff --git a/tests/generic/260 b/tests/generic/260
index 2f653b4af2..08fde46873 100755
--- a/tests/generic/260
+++ b/tests/generic/260
@@ -27,7 +27,7 @@ _scratch_mount
 
 _require_batched_discard $SCRATCH_MNT
 
-fssize=$($DF_PROG -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $3}')
+fssize=$(_discard_max_offset_kb "$SCRATCH_MNT" "$SCRATCH_DEV")
 
 beyond_eofs=$(_math "$fssize*2048")
 max_64bit=$(_math "2^64 - 1")

[PATCH 2/3] xfs/242: fix _filter_bmap for xfs_io bmap that does rt file properly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xfsprogs commit b1faed5f787 ("xfs_io: fix bmap command not detecting
realtime files with xattrs") fixed the xfs_io bmap output to display
realtime file columns for realtime files with xattrs.  As a result, the
data and unwritten flags are in column 5 and not column 7.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/punch |    8 ++++++++
 1 file changed, 8 insertions(+)


diff --git a/common/punch b/common/punch
index 3b8be21a2a..9e730404e2 100644
--- a/common/punch
+++ b/common/punch
@@ -188,6 +188,7 @@ _filter_hole_fiemap()
 	_coalesce_extents
 }
 
+# Column 7 for datadev files and column 5 for rtdev files
 #     10000 Unwritten preallocated extent
 #     01000 Doesn't begin on stripe unit
 #     00100 Doesn't end   on stripe unit
@@ -200,6 +201,13 @@ _filter_bmap()
 			print $1, $2, $3;
 			next;
 		}
+		$5 ~ /1[01][01][01][01]/ {
+			print $1, $2, "unwritten";
+			next;
+		}
+		$5 ~ /0[01][01][01][01]/ {
+			print $1, $2, "data"
+		}
 		$7 ~ /1[01][01][01][01]/ {
 			print $1, $2, "unwritten";
 			next;

Re: [PATCH 2/3] xfs/242: fix _filter_bmap for xfs_io bmap that does rt file properly

[David Disseldorp]

On Tue, 28 Mar 2023 17:58:16 -0700, Darrick J. Wong wrote:

> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfsprogs commit b1faed5f787 ("xfs_io: fix bmap command not detecting
> realtime files with xattrs") fixed the xfs_io bmap output to display
> realtime file columns for realtime files with xattrs.  As a result, the
> data and unwritten flags are in column 5 and not column 7.

Reviewed-by: David Disseldorp <ddiss@suse.de>

[PATCH 3/3] common/report: fix typo in FSSTRESS_AVOID

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix a minor typo.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/report |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/common/report b/common/report
index 23ddbb096d..be930e0b06 100644
--- a/common/report
+++ b/common/report
@@ -10,7 +10,7 @@ REPORT_ENV_LIST=("SECTION" "FSTYP" "PLATFORM" "MKFS_OPTIONS" "MOUNT_OPTIONS" \
 		 "SCRATCH_DEV" "SCRATCH_MNT" "OVL_UPPER" "OVL_LOWER" "OVL_WORK")
 
 # Variables that are captured in the report /if/ they are set.
-REPORT_ENV_LIST_OPT=("TAPE_DEV" "RMT_TAPE_DEV" "FSSTRES_AVOID" "FSX_AVOID"
+REPORT_ENV_LIST_OPT=("TAPE_DEV" "RMT_TAPE_DEV" "FSSTRESS_AVOID" "FSX_AVOID"
 		     "KCONFIG_PATH" "PERF_CONFIGNAME" "MIN_FSSIZE"
 		     "IDMAPPED_MOUNTS")
 

Re: [PATCH 3/3] common/report: fix typo in FSSTRESS_AVOID

[David Disseldorp]

On Tue, 28 Mar 2023 17:58:21 -0700, Darrick J. Wong wrote:

> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix a minor typo.

Reviewed-by: David Disseldorp <ddiss@suse.de>