[PATCH 1/2 v3] commit.rc: Add helper for math operation using bc

[PATCH 1/2 v3] commit.rc: Add helper for math operation using bc

[Lukas Czerner]

Sometimes using bash $(()) math might not be enough due to some
limitation (big numbers), so add helper using 'bc' program. For
now the results are only in perfect numbers (as in bash) since this is
all I need for now.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v3: Nothing has changed

 common.rc |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/common.rc b/common.rc
index 35f782b..b0e0c6a 100644
--- a/common.rc
+++ b/common.rc
@@ -20,6 +20,20 @@
 #  Mountain View, CA 94043, USA, or: http://www.sgi.com
 #-----------------------------------------------------------------------
 
+BC=$(which bc 2> /dev/null) || BC=
+
+_math() {
+	if [ $# -le 0 ]; then
+		return
+	fi
+	if [ "$BC" ]; then
+		result=$(LANG=C echo "scale=0; $@" | "$BC" -q 2> /dev/null)
+	else
+		result=$(($@))
+	fi
+	echo "$result"
+}
+
 dd()
 {
    if [ "$HOSTOS" == "Linux" ]
-- 
1.7.4.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

This test suppose to validate that file systems are using the fitrim
arguments right. It checks that the fstrim returns EINVAl in case that
the start of the range is beyond the end of the file system, and also
that the fstrim works without an error if the length of the range is
bigger than the file system (it should be truncated to the file system
length automatically within the fitrim implementation).

This test should also catch common problem with overflow of start+len.
Some file systems (ext4,xfs) had overflow problems in the past so there
is a specific test for it (for ext4 and xfs) as well as generic test for
other file systems, but it would be nice if other fs can add their
specific checks if this problem does apply to them as well.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: add check for fsblock to agno overflow
v3: Update comments, use bc math

 257     |  183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 257.out |   14 +++++
 group   |    1 +
 3 files changed, 198 insertions(+), 0 deletions(-)
 create mode 100755 257
 create mode 100644 257.out

diff --git a/257 b/257
new file mode 100755
index 0000000..4c820fd
--- /dev/null
+++ b/257
@@ -0,0 +1,183 @@
+#!/bin/bash
+# FS QA Test No. 251
+#
+# This test was created in order to verify filesystem FITRIM implementation.
+# By many concurrent copy and remove operations and checking that files
+# does not change after copied into SCRATCH_MNT test if FITRIM implementation
+# corrupts the filesystem (data/metadata).
+#
+#-----------------------------------------------------------------------
+# Copyright 2010 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+
+owner=lczerner@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=`mktemp -d`
+status=0
+trap "exit \$status" 0 1 3
+trap "exit \$status" 2 15
+chpid=0
+mypid=$$
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+FSTRIM="$here/src/fstrim"
+"$FSTRIM" -l 10M $SCRATCH_MNT &> /dev/null || _notrun "FSTRIM is not supported"
+
+fssize=$(df -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $2}')
+
+# All these tests should return EINVAL
+# since the start is beyond the end of
+# the file system
+
+echo "[+] Start beyond the end of fs (should fail)"
+"$FSTRIM" -s$(_math "$fssize*2048") $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start beyond the end of fs with len set (should fail)"
+"$FSTRIM" -s$(_math "$fssize*2048") -l1M $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start = 2^64-1 (should fail)"
+"$FSTRIM" -s18446744073709551615 $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start = 2^64-1 and len is set (should fail)"
+"$FSTRIM" -s18446744073709551615 -l1M $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# All these tests should succeed
+# since the length should be truncated
+
+echo "[+] Default length (should succeed)"
+"$FSTRIM" $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Default length with start set (should succeed)"
+"$FSTRIM" -s10M $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Length beyond the end of fs (should succeed)"
+"$FSTRIM" -l$(_math "fssize*2048") $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Length beyond the end of fs with start set (should succeed)"
+"$FSTRIM" -s10M -l$(_math "fssize*2048") $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# This is a bit fuzzy, but since the file system is fresh
+# there should be at least (fssize/2) free space to trim.
+# This is supposed to catch wrong FITRIM argument handling
+out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
+bytes=${out%% *}
+
+if [ $bytes -gt $(_math "$fssize*1024") ]; then
+	status=1
+	echo "After the full fs discard $bytes bytes were discarded"\
+	     "however the file system is $(_math "$fssize*1024") bytes long."
+fi
+
+# Btrfs is special and this test does not apply to it
+# It is because btrfs does not have not-yet-used parts of the device
+# mapped and since we got here right after the mkfs, there is not
+# enough free extents in the root tree.
+if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+	status=1
+	echo "After the full fs discard $bytes bytes were discarded"\
+	     "however the file system is $(_math "$fssize*1024") bytes long."
+fi
+
+# Now to catch overflows due to fsblk->allocation group number conversion
+# This is different for every file system and it also apply just to some of
+# them. In order to add check specific for file system you're interested in
+# compute the arguments as you need and make the file system with proper
+# alignment in function _check_conversion_overflow()
+
+# (2^32-1) + 2 (this is set to overflow 32bit variable by 2)
+base=$(_math "4294967295+2")
+
+case $FSTYP in
+	ext[34])
+		agsize=32768
+		bsize=4096
+		start=$(_math "$base*$agsize*$bsize")
+		len=$(_math "$base*$agsize*$bsize")
+		export MKFS_OPTIONS="-F -b $bsize -g $agsize"
+		;;
+	xfs)
+		agsize=65538
+		bsize=4096
+		start=$(_math "$base*$agsize*$bsize")
+		len=$(_math "$base*$agsize*$bsize")
+		export MKFS_OPTIONS="-f -d agsize=$(_math "$agsize*$bsize") -b size=$bsize"
+		;;
+	*)
+		# (2^32-1) * 4096 * 65536 == 32bit max size * block size * ag size
+		start="1152921504875282432"
+		len="1152921504875282432"
+		;;
+esac
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+# It should fail since $start is beyond the end of file system
+"$FSTRIM" -s$start -l$(_math "$fssize/2") $SCRATCH_MNT &> /dev/null
+if [ $? -eq 0 ]; then
+	status=1
+	echo "It seems that fs logic handling start"\
+	     "argument overflows"
+fi
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# len should be big enough to cover the whole file system, however this
+# test suppose for the overflow, so if the number of discarded bytes is
+# smaller than fssize/2 than it most likely does overflow.
+out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
+bytes=${out%% *}
+
+# Btrfs is special and this test does not apply to it
+# It is because btrfs does not have not-yet-used parts of the device
+# mapped and since we got here right after the mkfs, there is not
+# enough free extents in the root tree.
+if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+	status=1
+	echo "It seems that fs logic handling len argument overflows"
+fi
+
+echo "Test done"
+exit
diff --git a/257.out b/257.out
new file mode 100644
index 0000000..5dac8ac
--- /dev/null
+++ b/257.out
@@ -0,0 +1,14 @@
+QA output created by 257
+[+] Start beyond the end of fs (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start beyond the end of fs with len set (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start = 2^64-1 (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start = 2^64-1 and len is set (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Default length (should succeed)
+[+] Default length with start set (should succeed)
+[+] Length beyond the end of fs (should succeed)
+[+] Length beyond the end of fs with start set (should succeed)
+Test done
diff --git a/group b/group
index 0c746c8..b742f91 100644
--- a/group
+++ b/group
@@ -370,3 +370,4 @@ deprecated
 254 auto quick
 255 auto quick prealloc
 256 auto quick
+257 auto quick trim
-- 
1.7.4.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add test 257: Check proper FITRIM argument handling

[Alex Elder]

On Fri, 2011-09-23 at 16:15 +0200, Lukas Czerner wrote:
> This test suppose to validate that file systems are using the fitrim
> arguments right. It checks that the fstrim returns EINVAl in case that
> the start of the range is beyond the end of the file system, and also
> that the fstrim works without an error if the length of the range is
> bigger than the file system (it should be truncated to the file system
> length automatically within the fitrim implementation).
> 
> This test should also catch common problem with overflow of start+len.
> Some file systems (ext4,xfs) had overflow problems in the past so there
> is a specific test for it (for ext4 and xfs) as well as generic test for
> other file systems, but it would be nice if other fs can add their
> specific checks if this problem does apply to them as well.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Better.  Now I'm taking a closer look and have a bunch
of comments.  Note that I am not actually running the
test so some of what I say may not apply.

					-Alex

> ---
> v2: add check for fsblock to agno overflow
> v3: Update comments, use bc math
> 
>  257     |  183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  257.out |   14 +++++
>  group   |    1 +
>  3 files changed, 198 insertions(+), 0 deletions(-)
>  create mode 100755 257
>  create mode 100644 257.out
> 
> diff --git a/257 b/257
> new file mode 100755
> index 0000000..4c820fd
> --- /dev/null
> +++ b/257
> @@ -0,0 +1,183 @@
> +#!/bin/bash
> +# FS QA Test No. 251
> +#
> +# This test was created in order to verify filesystem FITRIM implementation.
> +# By many concurrent copy and remove operations and checking that files
> +# does not change after copied into SCRATCH_MNT test if FITRIM implementation
> +# corrupts the filesystem (data/metadata).
> +#
> +#-----------------------------------------------------------------------
> +# Copyright 2010 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>

Copyright date is wrong.

> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +
> +owner=lczerner@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=`mktemp -d`
> +status=0
> +trap "exit \$status" 0 1 3
> +trap "exit \$status" 2 15

Why two trap statements?

> +chpid=0
> +mypid=$$
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter

. . .

> +echo "[+] Start beyond the end of fs with len set (should fail)"
> +"$FSTRIM" -s$(_math "$fssize*2048") -l1M $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1

Since you reuse it a lot, maybe:
beyond_eofs=$(_math "$fssize * 2048")

> +echo "[+] Start = 2^64-1 (should fail)"
> +"$FSTRIM" -s18446744073709551615 $SCRATCH_MNT

Since you now have the _math function at your disposal
you can actually represent this symbolically (here and
below).

max64bit=$(_math "2^64 - 1")
"$FSTRIM" -s${max64bit} $SCRATCH_MNT

> +[ $? -eq 0 ] && status=1
> +
> +echo "[+] Start = 2^64-1 and len is set (should fail)"
> +"$FSTRIM" -s18446744073709551615 -l1M $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# All these tests should succeed
> +# since the length should be truncated
> +
> +echo "[+] Default length (should succeed)"
> +"$FSTRIM" $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Default length with start set (should succeed)"
> +"$FSTRIM" -s10M $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Length beyond the end of fs (should succeed)"
> +"$FSTRIM" -l$(_math "fssize*2048") $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Length beyond the end of fs with start set (should succeed)"
> +"$FSTRIM" -s10M -l$(_math "fssize*2048") $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1

Is there any need to make sure these tests both
with the underlying storage not yet trimmed as
well as with them already having been trimmed?
As it is, the first trim (with all defaults) will
trim the whole filesystem.  Then you're running
a trim command that will trim starting at offset
10M to the end of filesystem, but that stuff's
already gone by that point.

If it is important it may be that you should re-make
the filesystem between tests.  You may be able to
order the tests in such a way they aren't *always*
re-made (but then again that may be yet more test
cases... it depends on whether you're testing the
fstrim interface handling of limits or whether you're
testing what the filesystem does under varying
circumstances).

> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# This is a bit fuzzy, but since the file system is fresh
> +# there should be at least (fssize/2) free space to trim.
> +# This is supposed to catch wrong FITRIM argument handling
> +out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
> +bytes=${out%% *}

So this is verifying that trimming all but the first
10MB should never report more than the filesystem
size as the number of bytes being requested to the
filesystem to trim?  The man page only says that
this reports the size requested, not the amount
trimmed.  (Maybe my man page is wrong.)

> +if [ $bytes -gt $(_math "$fssize*1024") ]; then
> +	status=1
> +	echo "After the full fs discard $bytes bytes were discarded"\
> +	     "however the file system is $(_math "$fssize*1024") bytes long."
> +fi
> +
> +# Btrfs is special and this test does not apply to it
> +# It is because btrfs does not have not-yet-used parts of the device
> +# mapped and since we got here right after the mkfs, there is not
> +# enough free extents in the root tree.
> +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> +	status=1

Same comment as above.

> +	echo "After the full fs discard $bytes bytes were discarded"\
> +	     "however the file system is $(_math "$fssize*1024") bytes long."
> +fi
> +
> +# Now to catch overflows due to fsblk->allocation group number conversion
> +# This is different for every file system and it also apply just to some of
> +# them. In order to add check specific for file system you're interested in
> +# compute the arguments as you need and make the file system with proper
> +# alignment in function _check_conversion_overflow()

Looks like this function no longer exists.

> +
> +# (2^32-1) + 2 (this is set to overflow 32bit variable by 2)
> +base=$(_math "4294967295+2")

Consider using $(_math "2^32 + 1")

> +case $FSTYP in
> +	ext[34])
> +		agsize=32768
> +		bsize=4096
> +		start=$(_math "$base*$agsize*$bsize")
> +		len=$(_math "$base*$agsize*$bsize")
> +		export MKFS_OPTIONS="-F -b $bsize -g $agsize"
> +		;;
> +	xfs)
> +		agsize=65538
> +		bsize=4096
> +		start=$(_math "$base*$agsize*$bsize")
> +		len=$(_math "$base*$agsize*$bsize")
> +		export MKFS_OPTIONS="-f -d agsize=$(_math "$agsize*$bsize") -b size=$bsize"
> +		;;
> +	*)
> +		# (2^32-1) * 4096 * 65536 == 32bit max size * block size * ag size
> +		start="1152921504875282432"

Here too, use _math()

> +		len="1152921504875282432"
> +		;;
> +esac
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +# It should fail since $start is beyond the end of file system
> +"$FSTRIM" -s$start -l$(_math "$fssize/2") $SCRATCH_MNT &> /dev/null
> +if [ $? -eq 0 ]; then
> +	status=1
> +	echo "It seems that fs logic handling start"\
> +	     "argument overflows"
> +fi
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# len should be big enough to cover the whole file system, however this
> +# test suppose for the overflow, so if the number of discarded bytes is

I'm not sure I understand this.  Do you mean "however this test is
checking to see if overflow occurs, and if the result is smaller
than fssize/2 then there most likely was an overflow"?

> +# smaller than fssize/2 than it most likely does overflow.
> +out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
> +bytes=${out%% *}

This again assumes that the verbose flag reports
what's trimmed, as opposed as how big the request
to trim is.  (This may be a correct assumption.)

> +# Btrfs is special and this test does not apply to it
> +# It is because btrfs does not have not-yet-used parts of the device
> +# mapped and since we got here right after the mkfs, there is not
> +# enough free extents in the root tree.

Move the calculation of "out" and "bytes" here, just
before it's used and *after* the comment.

> +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> +	status=1
> +	echo "It seems that fs logic handling len argument overflows"
> +fi
> +
> +echo "Test done"
> +exit

. . .


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2 v3] commit.rc: Add helper for math operation using bc

[Alex Elder]

On Fri, 2011-09-23 at 16:15 +0200, Lukas Czerner wrote:
> Sometimes using bash $(()) math might not be enough due to some
> limitation (big numbers), so add helper using 'bc' program. For
> now the results are only in perfect numbers (as in bash) since this is
> all I need for now.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Clever.  This looks OK to me.  I'd like to hear what
others think.

If it gets committed maybe we should revisit tests
to see if using this function is warranted elsewhere.
Boris Ranto's recent test (which I am on the verge
of committing) in particular would be better the
way it was originally posted, making use of this.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

On Fri, 23 Sep 2011, Alex Elder wrote:

> On Fri, 2011-09-23 at 16:15 +0200, Lukas Czerner wrote:
> > This test suppose to validate that file systems are using the fitrim
> > arguments right. It checks that the fstrim returns EINVAl in case that
> > the start of the range is beyond the end of the file system, and also
> > that the fstrim works without an error if the length of the range is
> > bigger than the file system (it should be truncated to the file system
> > length automatically within the fitrim implementation).
> > 
> > This test should also catch common problem with overflow of start+len.
> > Some file systems (ext4,xfs) had overflow problems in the past so there
> > is a specific test for it (for ext4 and xfs) as well as generic test for
> > other file systems, but it would be nice if other fs can add their
> > specific checks if this problem does apply to them as well.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> Better.  Now I'm taking a closer look and have a bunch
> of comments.  Note that I am not actually running the
> test so some of what I say may not apply.
> 
> 					-Alex
> 
> > ---
> > v2: add check for fsblock to agno overflow
> > v3: Update comments, use bc math
> > 
> >  257     |  183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  257.out |   14 +++++
> >  group   |    1 +
> >  3 files changed, 198 insertions(+), 0 deletions(-)
> >  create mode 100755 257
> >  create mode 100644 257.out
> > 
> > diff --git a/257 b/257
> > new file mode 100755
> > index 0000000..4c820fd
> > --- /dev/null
> > +++ b/257
> > @@ -0,0 +1,183 @@
> > +#!/bin/bash
> > +# FS QA Test No. 251
> > +#
> > +# This test was created in order to verify filesystem FITRIM implementation.
> > +# By many concurrent copy and remove operations and checking that files
> > +# does not change after copied into SCRATCH_MNT test if FITRIM implementation
> > +# corrupts the filesystem (data/metadata).
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright 2010 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
> 
> Copyright date is wrong.
> 
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation.
> > +#
> > +# This program is distributed in the hope that it would be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write the Free Software Foundation,
> > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > +#-----------------------------------------------------------------------
> > +
> > +owner=lczerner@redhat.com
> > +
> > +seq=`basename $0`
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=`mktemp -d`
> > +status=0
> > +trap "exit \$status" 0 1 3
> > +trap "exit \$status" 2 15
> 
> Why two trap statements?

Some leftover stuff, will fix it.
> 
> > +chpid=0
> > +mypid=$$
> > +
> > +# get standard environment, filters and checks
> > +. ./common.rc
> > +. ./common.filter
> 
> . . .
> 
> > +echo "[+] Start beyond the end of fs with len set (should fail)"
> > +"$FSTRIM" -s$(_math "$fssize*2048") -l1M $SCRATCH_MNT
> > +[ $? -eq 0 ] && status=1
> 
> Since you reuse it a lot, maybe:
> beyond_eofs=$(_math "$fssize * 2048")
ok

> 
> > +echo "[+] Start = 2^64-1 (should fail)"
> > +"$FSTRIM" -s18446744073709551615 $SCRATCH_MNT
> 
> Since you now have the _math function at your disposal
> you can actually represent this symbolically (here and
> below).
ok

> 
> max64bit=$(_math "2^64 - 1")
> "$FSTRIM" -s${max64bit} $SCRATCH_MNT
> 
> > +[ $? -eq 0 ] && status=1
> > +
> > +echo "[+] Start = 2^64-1 and len is set (should fail)"
> > +"$FSTRIM" -s18446744073709551615 -l1M $SCRATCH_MNT
> > +[ $? -eq 0 ] && status=1
> > +
> > +_scratch_unmount
> > +_scratch_mkfs >/dev/null 2>&1
> > +_scratch_mount
> > +
> > +# All these tests should succeed
> > +# since the length should be truncated
> > +
> > +echo "[+] Default length (should succeed)"
> > +"$FSTRIM" $SCRATCH_MNT
> > +[ $? -ne 0 ] && status=1
> > +echo "[+] Default length with start set (should succeed)"
> > +"$FSTRIM" -s10M $SCRATCH_MNT
> > +[ $? -ne 0 ] && status=1
> > +echo "[+] Length beyond the end of fs (should succeed)"
> > +"$FSTRIM" -l$(_math "fssize*2048") $SCRATCH_MNT
> > +[ $? -ne 0 ] && status=1
> > +echo "[+] Length beyond the end of fs with start set (should succeed)"
> > +"$FSTRIM" -s10M -l$(_math "fssize*2048") $SCRATCH_MNT
> > +[ $? -ne 0 ] && status=1
> 
> Is there any need to make sure these tests both
> with the underlying storage not yet trimmed as
> well as with them already having been trimmed?
> As it is, the first trim (with all defaults) will
> trim the whole filesystem.  Then you're running
> a trim command that will trim starting at offset
> 10M to the end of filesystem, but that stuff's
> already gone by that point.
> 
> If it is important it may be that you should re-make
> the filesystem between tests.  You may be able to
> order the tests in such a way they aren't *always*
> re-made (but then again that may be yet more test
> cases... it depends on whether you're testing the
> fstrim interface handling of limits or whether you're
> testing what the filesystem does under varying
> circumstances).

The only thing we are trying to catch here is the argument handling, it
does not matter whether the discard will be issued or not. We just have
to make sure that the implementation does not return error on certain
circumstances.

> 
> > +_scratch_unmount
> > +_scratch_mkfs >/dev/null 2>&1
> > +_scratch_mount
> > +
> > +# This is a bit fuzzy, but since the file system is fresh
> > +# there should be at least (fssize/2) free space to trim.
> > +# This is supposed to catch wrong FITRIM argument handling
> > +out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
> > +bytes=${out%% *}
> 
> So this is verifying that trimming all but the first
> 10MB should never report more than the filesystem
> size as the number of bytes being requested to the
> filesystem to trim?  The man page only says that
> this reports the size requested, not the amount
> trimmed.  (Maybe my man page is wrong.)

Yes, it reports the number of bytes that has been requested to trim. We
just do not know what the block layer, or device itself will do with it,
and at this point we do not care. In this case we just have to make sure
that the implementation does not report anything bigger than the size of
the file system, since it would not make sense. Note that it is possible
to get it wrong because we are allowed to pass in much bigger 'len' then
the actual size of the file system and it should be adjusted by the
implementation.

> 
> > +if [ $bytes -gt $(_math "$fssize*1024") ]; then
> > +	status=1
> > +	echo "After the full fs discard $bytes bytes were discarded"\
> > +	     "however the file system is $(_math "$fssize*1024") bytes long."
> > +fi
> > +
> > +# Btrfs is special and this test does not apply to it
> > +# It is because btrfs does not have not-yet-used parts of the device
> > +# mapped and since we got here right after the mkfs, there is not
> > +# enough free extents in the root tree.
> > +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> > +	status=1
> 
> Same comment as above.
> 
> > +	echo "After the full fs discard $bytes bytes were discarded"\
> > +	     "however the file system is $(_math "$fssize*1024") bytes long."
> > +fi
> > +
> > +# Now to catch overflows due to fsblk->allocation group number conversion
> > +# This is different for every file system and it also apply just to some of
> > +# them. In order to add check specific for file system you're interested in
> > +# compute the arguments as you need and make the file system with proper
> > +# alignment in function _check_conversion_overflow()
> 
> Looks like this function no longer exists.

oh, right.

> 
> > +
> > +# (2^32-1) + 2 (this is set to overflow 32bit variable by 2)
> > +base=$(_math "4294967295+2")
> 
> Consider using $(_math "2^32 + 1")
> 
> > +case $FSTYP in
> > +	ext[34])
> > +		agsize=32768
> > +		bsize=4096
> > +		start=$(_math "$base*$agsize*$bsize")
> > +		len=$(_math "$base*$agsize*$bsize")
> > +		export MKFS_OPTIONS="-F -b $bsize -g $agsize"
> > +		;;
> > +	xfs)
> > +		agsize=65538
> > +		bsize=4096
> > +		start=$(_math "$base*$agsize*$bsize")
> > +		len=$(_math "$base*$agsize*$bsize")
> > +		export MKFS_OPTIONS="-f -d agsize=$(_math "$agsize*$bsize") -b size=$bsize"
> > +		;;
> > +	*)
> > +		# (2^32-1) * 4096 * 65536 == 32bit max size * block size * ag size
> > +		start="1152921504875282432"
> 
> Here too, use _math()
> 
> > +		len="1152921504875282432"
> > +		;;
> > +esac
> > +
> > +_scratch_unmount
> > +_scratch_mkfs >/dev/null 2>&1
> > +_scratch_mount
> > +# It should fail since $start is beyond the end of file system
> > +"$FSTRIM" -s$start -l$(_math "$fssize/2") $SCRATCH_MNT &> /dev/null
> > +if [ $? -eq 0 ]; then
> > +	status=1
> > +	echo "It seems that fs logic handling start"\
> > +	     "argument overflows"
> > +fi
> > +
> > +_scratch_unmount
> > +_scratch_mkfs >/dev/null 2>&1
> > +_scratch_mount
> > +
> > +# len should be big enough to cover the whole file system, however this
> > +# test suppose for the overflow, so if the number of discarded bytes is
> 
> I'm not sure I understand this.  Do you mean "however this test is
> checking to see if overflow occurs, and if the result is smaller
> than fssize/2 then there most likely was an overflow"?

yes, exactly! however there are some words missing :) it should rather say:

# len should be big enough to cover the whole file system, so if the
# number of discarded bytes is smaller than fssize/2 then it most likely
# overflowed

> 
> > +# smaller than fssize/2 than it most likely does overflow.
> > +out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
> > +bytes=${out%% *}
> 
> This again assumes that the verbose flag reports
> what's trimmed, as opposed as how big the request
> to trim is.  (This may be a correct assumption.)
> 
> > +# Btrfs is special and this test does not apply to it
> > +# It is because btrfs does not have not-yet-used parts of the device
> > +# mapped and since we got here right after the mkfs, there is not
> > +# enough free extents in the root tree.
> 
> Move the calculation of "out" and "bytes" here, just
> before it's used and *after* the comment.
ok

> 
> > +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> > +	status=1
> > +	echo "It seems that fs logic handling len argument overflows"
> > +fi
> > +
> > +echo "Test done"
> > +exit
> 
> . . .

Thanks!
-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add test 257: Check proper FITRIM argument handling

[Michael Monnerie]

[-- Attachment #1.1: Type: Text/Plain, Size: 1120 bytes --]

On Freitag, 23. September 2011 Lukas Czerner wrote:
> +# FS QA Test No. 251
> +#
> +# This test was created in order to verify filesystem FITRIM
> implementation. +# By many concurrent copy and remove operations and
> checking that files +# does not change after copied into SCRATCH_MNT
> test if FITRIM implementation +# corrupts the filesystem
> (data/metadata).

This is a bit misspelled and (for me being non-native english) hard to 
understand. I guess it should be

This test was created in order to verify the filesystem FITRIM 
implementation by doing many concurrent copy and remove operations and
checking that files do not change after being copied into SCRATCH_MNT.
Test if FITRIM implementation corrupts the filesystem (data/metadata).

(Where I'm not sure if the last sentence belongs there, or in a separate 
paragraph, or is redundant to the first sentence)

-- 
mit freundlichen Grüssen,
Michael Monnerie, Ing. BSc

it-management Internet Services: Protéger
http://proteger.at [gesprochen: Prot-e-schee]
Tel: +43 660 / 415 6531

// Haus zu verkaufen: http://zmi.at/langegg/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

On Sat, 24 Sep 2011, Michael Monnerie wrote:

> On Freitag, 23. September 2011 Lukas Czerner wrote:
> > +# FS QA Test No. 251
> > +#
> > +# This test was created in order to verify filesystem FITRIM
> > implementation. +# By many concurrent copy and remove operations and
> > checking that files +# does not change after copied into SCRATCH_MNT
> > test if FITRIM implementation +# corrupts the filesystem
> > (data/metadata).
> 
> This is a bit misspelled and (for me being non-native english) hard to 
> understand. I guess it should be
> 
> This test was created in order to verify the filesystem FITRIM 
> implementation by doing many concurrent copy and remove operations and
> checking that files do not change after being copied into SCRATCH_MNT.
> Test if FITRIM implementation corrupts the filesystem (data/metadata).
> 
> (Where I'm not sure if the last sentence belongs there, or in a separate 
> paragraph, or is redundant to the first sentence)
> 

Oops, the whole paragraph does not belong there. It is leftover from a
different test I have wrote earlier. Thanks for noticing it and sorry
for the confusion.

-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

This test suppose to validate that file systems are using the fitrim
arguments right. It checks that the fstrim returns EINVAl in case that
the start of the range is beyond the end of the file system, and also
that the fstrim works without an error if the length of the range is
bigger than the file system (it should be truncated to the file system
length automatically within the fitrim implementation).

This test should also catch common problem with overflow of start+len.
Some file systems (ext4,xfs) had overflow problems in the past so there
is a specific test for it (for ext4 and xfs) as well as generic test for
other file systems, but it would be nice if other fs can add their
specific checks if this problem does apply to them as well.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: add check for fsblock to agno overflow
v3: Update comments, use bc math
v4: Update comments, copyright, and use precomputed values

 257     |  182 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 257.out |   14 +++++
 group   |    1 +
 3 files changed, 197 insertions(+), 0 deletions(-)
 create mode 100755 257
 create mode 100644 257.out

diff --git a/257 b/257
new file mode 100755
index 0000000..f605309
--- /dev/null
+++ b/257
@@ -0,0 +1,182 @@
+#!/bin/bash
+# FS QA Test No. 257
+#
+# Purpose of this test is to check FITRIM argument handling to make sure
+# that the argument processing is right and that it does not overflow.
+#
+#-----------------------------------------------------------------------
+# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+
+owner=lczerner@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=`mktemp -d`
+status=0
+trap "exit \$status" 0 1 2 3 15
+chpid=0
+mypid=$$
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+FSTRIM="$here/src/fstrim"
+"$FSTRIM" -l 10M $SCRATCH_MNT &> /dev/null || _notrun "FSTRIM is not supported"
+
+fssize=$(df -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $2}')
+
+beyond_eofs=$(_math "$fssize*2048")
+max_64bit=$(_math "2^64 - 1")
+
+# All these tests should return EINVAL
+# since the start is beyond the end of
+# the file system
+
+echo "[+] Start beyond the end of fs (should fail)"
+"$FSTRIM" -s $beyond_eofs $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start beyond the end of fs with len set (should fail)"
+"$FSTRIM" -s $beyond_eofs -l1M $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start = 2^64-1 (should fail)"
+"$FSTRIM" -s $max_64bit $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+echo "[+] Start = 2^64-1 and len is set (should fail)"
+"$FSTRIM" -s $max_64bit -l1M $SCRATCH_MNT
+[ $? -eq 0 ] && status=1
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# All these tests should succeed
+# since the length should be truncated
+
+echo "[+] Default length (should succeed)"
+"$FSTRIM" $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Default length with start set (should succeed)"
+"$FSTRIM" -s10M $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Length beyond the end of fs (should succeed)"
+"$FSTRIM" -l $beyond_eofs $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+echo "[+] Length beyond the end of fs with start set (should succeed)"
+"$FSTRIM" -s10M -l $beyond_eofs $SCRATCH_MNT
+[ $? -ne 0 ] && status=1
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# This is a bit fuzzy, but since the file system is fresh
+# there should be at least (fssize/2) free space to trim.
+# This is supposed to catch wrong FITRIM argument handling
+out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
+bytes=${out%% *}
+
+if [ $bytes -gt $(_math "$fssize*1024") ]; then
+	status=1
+	echo "After the full fs discard $bytes bytes were discarded"\
+	     "however the file system is $(_math "$fssize*1024") bytes long."
+fi
+
+# Btrfs is special and this test does not apply to it
+# It is because btrfs does not have not-yet-used parts of the device
+# mapped and since we got here right after the mkfs, there is not
+# enough free extents in the root tree.
+if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+	status=1
+	echo "After the full fs discard $bytes bytes were discarded"\
+	     "however the file system is $(_math "$fssize*1024") bytes long."
+fi
+
+# Now to catch overflows due to fsblk->allocation group number conversion
+# This is different for every file system and it also apply just to some of
+# them. In order to add check specific for file system you're interested in
+# compute the arguments as you need and make the file system with proper
+# alignment
+
+# (2^32-1) + 2 (this is set to overflow 32bit variable by 2)
+base=$(_math "2^32+1")
+
+case $FSTYP in
+	ext[34])
+		agsize=32768
+		bsize=4096
+		start=$(_math "$base*$agsize*$bsize")
+		len=$start
+		export MKFS_OPTIONS="-F -b $bsize -g $agsize"
+		;;
+	xfs)
+		agsize=65538
+		bsize=4096
+		start=$(_math "$base*$agsize*$bsize")
+		len=$start
+		export MKFS_OPTIONS="-f -d agsize=$(_math "$agsize*$bsize") -b size=$bsize"
+		;;
+	*)
+		# (2^32-1) * 4096 * 65536 == 32bit max size * block size * ag size
+		start=$(_math "(2^32 - 1) * 4096 * 65536")
+		len=$start
+		;;
+esac
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+# It should fail since $start is beyond the end of file system
+"$FSTRIM" -s$start -l10M $SCRATCH_MNT &> /dev/null
+if [ $? -eq 0 ]; then
+	status=1
+	echo "It seems that fs logic handling start"\
+	     "argument overflows"
+fi
+
+_scratch_unmount
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# len should be big enough to cover the whole file system, so if the
+# number of discarded bytes is smaller than file system size/2 then it
+# most likely overflowed
+# Btrfs is special and this test does not apply to it
+# It is because btrfs does not have not-yet-used parts of the device
+# mapped and since we got here right after the mkfs, there is not
+# enough free extents in the root tree.
+out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
+bytes=${out%% *}
+if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
+	status=1
+	echo "It seems that fs logic handling len argument overflows"
+fi
+
+echo "Test done"
+exit
diff --git a/257.out b/257.out
new file mode 100644
index 0000000..5dac8ac
--- /dev/null
+++ b/257.out
@@ -0,0 +1,14 @@
+QA output created by 257
+[+] Start beyond the end of fs (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start beyond the end of fs with len set (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start = 2^64-1 (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Start = 2^64-1 and len is set (should fail)
+fstrim: FSTRIM: Invalid argument
+[+] Default length (should succeed)
+[+] Default length with start set (should succeed)
+[+] Length beyond the end of fs (should succeed)
+[+] Length beyond the end of fs with start set (should succeed)
+Test done
diff --git a/group b/group
index 0c746c8..b742f91 100644
--- a/group
+++ b/group
@@ -370,3 +370,4 @@ deprecated
 254 auto quick
 255 auto quick prealloc
 256 auto quick
+257 auto quick trim
-- 
1.7.4.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

On Mon, 26 Sep 2011, Lukas Czerner wrote:

> This test suppose to validate that file systems are using the fitrim
> arguments right. It checks that the fstrim returns EINVAl in case that
> the start of the range is beyond the end of the file system, and also
> that the fstrim works without an error if the length of the range is
> bigger than the file system (it should be truncated to the file system
> length automatically within the fitrim implementation).
> 
> This test should also catch common problem with overflow of start+len.
> Some file systems (ext4,xfs) had overflow problems in the past so there
> is a specific test for it (for ext4 and xfs) as well as generic test for
> other file systems, but it would be nice if other fs can add their
> specific checks if this problem does apply to them as well.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Agh, it should be v4.

> ---
> v2: add check for fsblock to agno overflow
> v3: Update comments, use bc math
> v4: Update comments, copyright, and use precomputed values
> 
>  257     |  182 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  257.out |   14 +++++
>  group   |    1 +
>  3 files changed, 197 insertions(+), 0 deletions(-)
>  create mode 100755 257
>  create mode 100644 257.out
> 
> diff --git a/257 b/257
> new file mode 100755
> index 0000000..f605309
> --- /dev/null
> +++ b/257
> @@ -0,0 +1,182 @@
> +#!/bin/bash
> +# FS QA Test No. 257
> +#
> +# Purpose of this test is to check FITRIM argument handling to make sure
> +# that the argument processing is right and that it does not overflow.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +
> +owner=lczerner@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=`mktemp -d`
> +status=0
> +trap "exit \$status" 0 1 2 3 15
> +chpid=0
> +mypid=$$
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +FSTRIM="$here/src/fstrim"
> +"$FSTRIM" -l 10M $SCRATCH_MNT &> /dev/null || _notrun "FSTRIM is not supported"
> +
> +fssize=$(df -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk '{print $2}')
> +
> +beyond_eofs=$(_math "$fssize*2048")
> +max_64bit=$(_math "2^64 - 1")
> +
> +# All these tests should return EINVAL
> +# since the start is beyond the end of
> +# the file system
> +
> +echo "[+] Start beyond the end of fs (should fail)"
> +"$FSTRIM" -s $beyond_eofs $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1
> +
> +echo "[+] Start beyond the end of fs with len set (should fail)"
> +"$FSTRIM" -s $beyond_eofs -l1M $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1
> +
> +echo "[+] Start = 2^64-1 (should fail)"
> +"$FSTRIM" -s $max_64bit $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1
> +
> +echo "[+] Start = 2^64-1 and len is set (should fail)"
> +"$FSTRIM" -s $max_64bit -l1M $SCRATCH_MNT
> +[ $? -eq 0 ] && status=1
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# All these tests should succeed
> +# since the length should be truncated
> +
> +echo "[+] Default length (should succeed)"
> +"$FSTRIM" $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Default length with start set (should succeed)"
> +"$FSTRIM" -s10M $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Length beyond the end of fs (should succeed)"
> +"$FSTRIM" -l $beyond_eofs $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +echo "[+] Length beyond the end of fs with start set (should succeed)"
> +"$FSTRIM" -s10M -l $beyond_eofs $SCRATCH_MNT
> +[ $? -ne 0 ] && status=1
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# This is a bit fuzzy, but since the file system is fresh
> +# there should be at least (fssize/2) free space to trim.
> +# This is supposed to catch wrong FITRIM argument handling
> +out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
> +bytes=${out%% *}
> +
> +if [ $bytes -gt $(_math "$fssize*1024") ]; then
> +	status=1
> +	echo "After the full fs discard $bytes bytes were discarded"\
> +	     "however the file system is $(_math "$fssize*1024") bytes long."
> +fi
> +
> +# Btrfs is special and this test does not apply to it
> +# It is because btrfs does not have not-yet-used parts of the device
> +# mapped and since we got here right after the mkfs, there is not
> +# enough free extents in the root tree.
> +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> +	status=1
> +	echo "After the full fs discard $bytes bytes were discarded"\
> +	     "however the file system is $(_math "$fssize*1024") bytes long."
> +fi
> +
> +# Now to catch overflows due to fsblk->allocation group number conversion
> +# This is different for every file system and it also apply just to some of
> +# them. In order to add check specific for file system you're interested in
> +# compute the arguments as you need and make the file system with proper
> +# alignment
> +
> +# (2^32-1) + 2 (this is set to overflow 32bit variable by 2)
> +base=$(_math "2^32+1")
> +
> +case $FSTYP in
> +	ext[34])
> +		agsize=32768
> +		bsize=4096
> +		start=$(_math "$base*$agsize*$bsize")
> +		len=$start
> +		export MKFS_OPTIONS="-F -b $bsize -g $agsize"
> +		;;
> +	xfs)
> +		agsize=65538
> +		bsize=4096
> +		start=$(_math "$base*$agsize*$bsize")
> +		len=$start
> +		export MKFS_OPTIONS="-f -d agsize=$(_math "$agsize*$bsize") -b size=$bsize"
> +		;;
> +	*)
> +		# (2^32-1) * 4096 * 65536 == 32bit max size * block size * ag size
> +		start=$(_math "(2^32 - 1) * 4096 * 65536")
> +		len=$start
> +		;;
> +esac
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +# It should fail since $start is beyond the end of file system
> +"$FSTRIM" -s$start -l10M $SCRATCH_MNT &> /dev/null
> +if [ $? -eq 0 ]; then
> +	status=1
> +	echo "It seems that fs logic handling start"\
> +	     "argument overflows"
> +fi
> +
> +_scratch_unmount
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# len should be big enough to cover the whole file system, so if the
> +# number of discarded bytes is smaller than file system size/2 then it
> +# most likely overflowed
> +# Btrfs is special and this test does not apply to it
> +# It is because btrfs does not have not-yet-used parts of the device
> +# mapped and since we got here right after the mkfs, there is not
> +# enough free extents in the root tree.
> +out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
> +bytes=${out%% *}
> +if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
> +	status=1
> +	echo "It seems that fs logic handling len argument overflows"
> +fi
> +
> +echo "Test done"
> +exit
> diff --git a/257.out b/257.out
> new file mode 100644
> index 0000000..5dac8ac
> --- /dev/null
> +++ b/257.out
> @@ -0,0 +1,14 @@
> +QA output created by 257
> +[+] Start beyond the end of fs (should fail)
> +fstrim: FSTRIM: Invalid argument
> +[+] Start beyond the end of fs with len set (should fail)
> +fstrim: FSTRIM: Invalid argument
> +[+] Start = 2^64-1 (should fail)
> +fstrim: FSTRIM: Invalid argument
> +[+] Start = 2^64-1 and len is set (should fail)
> +fstrim: FSTRIM: Invalid argument
> +[+] Default length (should succeed)
> +[+] Default length with start set (should succeed)
> +[+] Length beyond the end of fs (should succeed)
> +[+] Length beyond the end of fs with start set (should succeed)
> +Test done
> diff --git a/group b/group
> index 0c746c8..b742f91 100644
> --- a/group
> +++ b/group
> @@ -370,3 +370,4 @@ deprecated
>  254 auto quick
>  255 auto quick prealloc
>  256 auto quick
> +257 auto quick trim
> 

-- 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Add test 257: Check proper FITRIM argument handling

[Alex Elder]

On Mon, 2011-09-26 at 09:14 +0200, Lukas Czerner wrote:
> This test suppose to validate that file systems are using the fitrim
> arguments right. It checks that the fstrim returns EINVAl in case that
> the start of the range is beyond the end of the file system, and also
> that the fstrim works without an error if the length of the range is
> bigger than the file system (it should be truncated to the file system
> length automatically within the fitrim implementation).
> 
> This test should also catch common problem with overflow of start+len.
> Some file systems (ext4,xfs) had overflow problems in the past so there
> is a specific test for it (for ext4 and xfs) as well as generic test for
> other file systems, but it would be nice if other fs can add their
> specific checks if this problem does apply to them as well.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

OK, I know I suggested it, and I do like the result, but
there could be a problem with the use of things like
"2^32 - 1" being passed to the _math() function.

The problem lies in the way _math() backs off to try
to use shell built-in arithmetic, which interprets the
'^' as a bitwise XOR operator.  (Note, _math() was
defined in an earlier patch.)

I think the use of "bc" to do certain math operations
has some value, and as such I think the right fix is
just to require "bc" in order for xfstests, or at least
for any that use the _math() function, and drop the
fall-back logic out of the definition of _math().

What do you think?

Assuming we resolve that, this test now looks fine to me.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Add test 257: Check proper FITRIM argument handling

[Lukas Czerner]

On Mon, 26 Sep 2011, Alex Elder wrote:

> On Mon, 2011-09-26 at 09:14 +0200, Lukas Czerner wrote:
> > This test suppose to validate that file systems are using the fitrim
> > arguments right. It checks that the fstrim returns EINVAl in case that
> > the start of the range is beyond the end of the file system, and also
> > that the fstrim works without an error if the length of the range is
> > bigger than the file system (it should be truncated to the file system
> > length automatically within the fitrim implementation).
> > 
> > This test should also catch common problem with overflow of start+len.
> > Some file systems (ext4,xfs) had overflow problems in the past so there
> > is a specific test for it (for ext4 and xfs) as well as generic test for
> > other file systems, but it would be nice if other fs can add their
> > specific checks if this problem does apply to them as well.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> OK, I know I suggested it, and I do like the result, but
> there could be a problem with the use of things like
> "2^32 - 1" being passed to the _math() function.
> 
> The problem lies in the way _math() backs off to try
> to use shell built-in arithmetic, which interprets the
> '^' as a bitwise XOR operator.  (Note, _math() was
> defined in an earlier patch.)
> 
> I think the use of "bc" to do certain math operations
> has some value, and as such I think the right fix is
> just to require "bc" in order for xfstests, or at least
> for any that use the _math() function, and drop the
> fall-back logic out of the definition of _math().
> 
> What do you think?

Yes I had the same concern, but I guess I was just lazy to look at it
:). So if we can require "bc" for xfstests we can simply remove the
fallback. Also maybe adding helper _require_bc, or maybe even better
adding helper _require <whatever> so we can check for <whatever> tool
in any test.

Thanks!
-Lukas


> 
> Assuming we resolve that, this test now looks fine to me.
> 
> Reviewed-by: Alex Elder <aelder@sgi.com>
> 
> 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Add test 257: Check proper FITRIM argument handling

[Alex Elder]

On Mon, 2011-09-26 at 15:52 +0200, Lukas Czerner wrote:
> On Mon, 26 Sep 2011, Alex Elder wrote:
> 
> > On Mon, 2011-09-26 at 09:14 +0200, Lukas Czerner wrote:
. . .
> > I think the use of "bc" to do certain math operations
> > has some value, and as such I think the right fix is
> > just to require "bc" in order for xfstests, or at least
> > for any that use the _math() function, and drop the
> > fall-back logic out of the definition of _math().
> > 
> > What do you think?
> 
> Yes I had the same concern, but I guess I was just lazy to look at it
> :). So if we can require "bc" for xfstests we can simply remove the
> fallback. Also maybe adding helper _require_bc, or maybe even better
> adding helper _require <whatever> so we can check for <whatever> tool
> in any test.

Would you mind re-submitting the first patch (which defined
the _math() function), adding the definition of _require_math
which would be used in any script that uses the _math function?
That would fail if "bc" weren't available.  It seems indirect
but I think _require_math makes more sense in the context of
whoever would be using it than _require_bc would.

And having heard nobody voice objection to the idea I think
we should just go with it.

I can take your other patch and insert the _require_math call
for you, and will verify the result works before committing it.

Thanks.

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v4] commit.rc: Add helper for math operation using bc

[Lukas Czerner]

Sometimes using bash $(()) math might not be enough due to some
limitation (big numbers), so add helper using 'bc' program. For
now the results are only in perfect numbers (as in bash) since this is
all I need for now.

This commit also adds _require_math() helper which should be called by
every test which uses _math() since it requires "bc" to be installed on
the system.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v3: Nothing has changed
v4: Add _require_math helper

 common.rc |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/common.rc b/common.rc
index 35f782b..e948169 100644
--- a/common.rc
+++ b/common.rc
@@ -20,6 +20,26 @@
 #  Mountain View, CA 94043, USA, or: http://www.sgi.com
 #-----------------------------------------------------------------------
 
+BC=$(which bc 2> /dev/null) || BC=
+
+_require_math()
+{
+	if [ -z "$BC" ]; then
+		_notrun "this test requires 'bc' tool for doing math operations"
+	fi
+}
+
+_math()
+{
+	[ $# -le 0 ] && return
+	if [ "$BC" ]; then
+		result=$(LANG=C echo "scale=0; $@" | "$BC" -q 2> /dev/null)
+	else
+		_notrun "this test requires 'bc' tool for doing math operations"
+	fi
+	echo "$result"
+}
+
 dd()
 {
    if [ "$HOSTOS" == "Linux" ]
-- 
1.7.4.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v4] commit.rc: Add helper for math operation using bc

[Alex Elder]

On Mon, 2011-09-26 at 20:45 +0200, Lukas Czerner wrote:
> Sometimes using bash $(()) math might not be enough due to some
> limitation (big numbers), so add helper using 'bc' program. For
> now the results are only in perfect numbers (as in bash) since this is
> all I need for now.
> 
> This commit also adds _require_math() helper which should be called by
> every test which uses _math() since it requires "bc" to be installed on
> the system.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks fine.  Thanks for updating it.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs