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

[David Disseldorp <ddiss@suse.de>]

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)}'

> +}