| Message ID | 168005149047.4147931.2729971759269213680.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: random fixes for v2023.03.26 | expand |
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)}' > +}
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 > > > +}
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")