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

["Darrick J. Wong" <djwong@kernel.org>]

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

> 
> > +}