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@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 }'
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@xxxxxxx>
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
>
> > +}
