On Tue, 28 Mar 2023 17:58:10 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxx>
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)}'
> +}
