public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB
@ 2012-10-16  9:41 Lukas Czerner
  2012-10-16  9:41 ` [PATCH 2/2] 251: Do not allow step to be zero Lukas Czerner
  2012-10-16 20:16 ` [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Dave Chinner
  0 siblings, 2 replies; 8+ messages in thread
From: Lukas Czerner @ 2012-10-16  9:41 UTC (permalink / raw)
  To: xfs; +Cc: Lukas Czerner

This tests corner case in FITRIM implementation where range size is
smaller than file system block or zero. In this case FITRIM should fail
with EINVAL.

The problem was spotted in xfs and ext4 where in case of length = 0 the
'end' variable underflowed. In case of length smaller than 1 FSB FITRIM
finished successfully, but we really should rather return EINVAL in both
cases.

(This patch has to be applied after 'Use upstream version of fstrim
instead of the local one')

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 286     |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 286.out |    6 ++++++
 group   |    1 +
 3 files changed, 65 insertions(+), 0 deletions(-)
 create mode 100644 286
 create mode 100644 286.out

diff --git a/286 b/286
new file mode 100644
index 0000000..b5122ec
--- /dev/null
+++ b/286
@@ -0,0 +1,58 @@
+#!/bin/bash
+# FS QA Test No. 286
+#
+# This check the FITRIM argument handling in the corner case where length is
+# smaller than block size of zero.
+#
+#-----------------------------------------------------------------------
+# Copyright 2012 (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"
+
+status=0
+trap "exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+
+_require_fstrim
+_require_scratch
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+_test_batched_discard $SCRATCH_MNT || _notrun "FITRIM not supported on $SCRATCH_DEV"
+
+echo "[+] Length is zero (should fail)"
+out=$("$FSTRIM_PROG" -v -o0 -l0 $SCRATCH_MNT 2>&1)
+[ $? -eq 0 ] && status=1
+echo $out | _filter_scratch
+
+echo "[+] Length is smaller than block size (should fail)"
+out=$("$FSTRIM_PROG" -v -o0 -l100 $SCRATCH_MNT 2>&1)
+[ $? -eq 0 ] && status=1
+echo $out | _filter_scratch
+
+echo "Test done"
+exit
diff --git a/286.out b/286.out
new file mode 100644
index 0000000..b164872
--- /dev/null
+++ b/286.out
@@ -0,0 +1,6 @@
+QA output created by 286
+[+] Length is zero (should fail)
+fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+[+] Length is smaller than block size (should fail)
+fstrim: SCRATCH_MNT: FITRIM ioctl failed: Invalid argument
+Test done
diff --git a/group b/group
index 697269b..c1bb04c 100644
--- a/group
+++ b/group
@@ -404,3 +404,4 @@ deprecated
 283 dump ioctl auto quick
 284 auto
 285 auto dump quota quick
+286 auto quick ioctl trim
-- 
1.7.7.6

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] 251: Do not allow step to be zero
  2012-10-16  9:41 [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Lukas Czerner
@ 2012-10-16  9:41 ` Lukas Czerner
  2012-10-16 20:18   ` Dave Chinner
  2012-10-16 20:16 ` [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Dave Chinner
  1 sibling, 1 reply; 8+ messages in thread
From: Lukas Czerner @ 2012-10-16  9:41 UTC (permalink / raw)
  To: xfs; +Cc: Lukas Czerner

Currently 'step' variable which is used as length of the range to
discard can be zero. However it would result in error returned by the
fstrim.

Fix this by forcing 'step' to be at least 4KB.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 251 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/251 b/251
index f46b6e2..373afd0 100755
--- a/251
+++ b/251
@@ -98,7 +98,7 @@ fstrim_loop()
 	mmlen=$(_guess_max_minlen)
 
 	while true ; do
-		step=$((RANDOM*$RANDOM))
+		step=$((RANDOM*$RANDOM+4))
 		minlen=$(((RANDOM*($RANDOM%2+1))%$mmlen))
 		start=$RANDOM
 		if [ $((RANDOM%10)) -gt 7 ]; then
-- 
1.7.7.6

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB
  2012-10-16  9:41 [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Lukas Czerner
  2012-10-16  9:41 ` [PATCH 2/2] 251: Do not allow step to be zero Lukas Czerner
@ 2012-10-16 20:16 ` Dave Chinner
  2012-10-16 20:48   ` Ben Myers
  2012-10-24 20:39   ` Rich Johnston
  1 sibling, 2 replies; 8+ messages in thread
From: Dave Chinner @ 2012-10-16 20:16 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: xfs

On Tue, Oct 16, 2012 at 11:41:33AM +0200, Lukas Czerner wrote:
> This tests corner case in FITRIM implementation where range size is
> smaller than file system block or zero. In this case FITRIM should fail
> with EINVAL.
> 
> The problem was spotted in xfs and ext4 where in case of length = 0 the
> 'end' variable underflowed. In case of length smaller than 1 FSB FITRIM
> finished successfully, but we really should rather return EINVAL in both
> cases.
> 
> (This patch has to be applied after 'Use upstream version of fstrim
> instead of the local one')
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  286     |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  286.out |    6 ++++++

We've already got 286 (and 287), so this should probably be 288...

Otherwise, it looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] 251: Do not allow step to be zero
  2012-10-16  9:41 ` [PATCH 2/2] 251: Do not allow step to be zero Lukas Czerner
@ 2012-10-16 20:18   ` Dave Chinner
  2012-10-24 20:43     ` Rich Johnston
  0 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2012-10-16 20:18 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: xfs

On Tue, Oct 16, 2012 at 11:41:34AM +0200, Lukas Czerner wrote:
> Currently 'step' variable which is used as length of the range to
> discard can be zero. However it would result in error returned by the
> fstrim.
> 
> Fix this by forcing 'step' to be at least 4KB.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks fine. It may not be perfect for large block filesystems (e.g.
on ppc or ia64) but it's better than nothing right now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB
  2012-10-16 20:16 ` [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Dave Chinner
@ 2012-10-16 20:48   ` Ben Myers
  2012-10-24 20:39   ` Rich Johnston
  1 sibling, 0 replies; 8+ messages in thread
From: Ben Myers @ 2012-10-16 20:48 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: xfs

Hey Lukas,

On Wed, Oct 17, 2012 at 07:16:45AM +1100, Dave Chinner wrote:
> On Tue, Oct 16, 2012 at 11:41:33AM +0200, Lukas Czerner wrote:
> > This tests corner case in FITRIM implementation where range size is
> > smaller than file system block or zero. In this case FITRIM should fail
> > with EINVAL.
> > 
> > The problem was spotted in xfs and ext4 where in case of length = 0 the
> > 'end' variable underflowed. In case of length smaller than 1 FSB FITRIM
> > finished successfully, but we really should rather return EINVAL in both
> > cases.
> > 
> > (This patch has to be applied after 'Use upstream version of fstrim
> > instead of the local one')
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  286     |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  286.out |    6 ++++++
> 
> We've already got 286 (and 287), so this should probably be 288...

Unless you've got something else to fix, don't worry about reposting to update
the test number...  We sometimes have collisions and it is just easiest if we
update it at commit time.

Thanks,
	Ben

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB
  2012-10-16 20:16 ` [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Dave Chinner
  2012-10-16 20:48   ` Ben Myers
@ 2012-10-24 20:39   ` Rich Johnston
  2012-10-24 20:44     ` Rich Johnston
  1 sibling, 1 reply; 8+ messages in thread
From: Rich Johnston @ 2012-10-24 20:39 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Lukas Czerner, xfs

Lukas,

On 10/16/2012 03:16 PM, Dave Chinner wrote:
> On Tue, Oct 16, 2012 at 11:41:33AM +0200, Lukas Czerner wrote:
>> This tests corner case in FITRIM implementation where range size is
>> smaller than file system block or zero. In this case FITRIM should fail
>> with EINVAL.
>>
>> The problem was spotted in xfs and ext4 where in case of length = 0 the
>> 'end' variable underflowed. In case of length smaller than 1 FSB FITRIM
>> finished successfully, but we really should rather return EINVAL in both
>> cases.
>>
>> (This patch has to be applied after 'Use upstream version of fstrim
>> instead of the local one')
>>
>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>> ---
>>   286     |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   286.out |    6 ++++++
>
> We've already got 286 (and 287), so this should probably be 288...
>
> Otherwise, it looks good.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>

This has been committed to git://oss.sgi.com/xfs/xfs.git, master and 
for-next branches.

Regards
--Rich

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] 251: Do not allow step to be zero
  2012-10-16 20:18   ` Dave Chinner
@ 2012-10-24 20:43     ` Rich Johnston
  0 siblings, 0 replies; 8+ messages in thread
From: Rich Johnston @ 2012-10-24 20:43 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Lukas Czerner, xfs

Lukas,

On 10/16/2012 03:18 PM, Dave Chinner wrote:
> On Tue, Oct 16, 2012 at 11:41:34AM +0200, Lukas Czerner wrote:
>> Currently 'step' variable which is used as length of the range to
>> discard can be zero. However it would result in error returned by the
>> fstrim.
>>
>> Fix this by forcing 'step' to be at least 4KB.
>>
>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>
> Looks fine. It may not be perfect for large block filesystems (e.g.
> on ppc or ia64) but it's better than nothing right now.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>

This has been committed to git://oss.sgi.com/xfs/cmds/xfstests, master 
and for-next branches.

Regards
--Rich

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB
  2012-10-24 20:39   ` Rich Johnston
@ 2012-10-24 20:44     ` Rich Johnston
  0 siblings, 0 replies; 8+ messages in thread
From: Rich Johnston @ 2012-10-24 20:44 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Lukas Czerner, xfs

On 10/24/2012 03:39 PM, Rich Johnston wrote:
> Lukas,
>
> On 10/16/2012 03:16 PM, Dave Chinner wrote:
>> On Tue, Oct 16, 2012 at 11:41:33AM +0200, Lukas Czerner wrote:
>>> This tests corner case in FITRIM implementation where range size is
>>> smaller than file system block or zero. In this case FITRIM should fail
>>> with EINVAL.
>>>
>>> The problem was spotted in xfs and ext4 where in case of length = 0 the
>>> 'end' variable underflowed. In case of length smaller than 1 FSB FITRIM
>>> finished successfully, but we really should rather return EINVAL in both
>>> cases.
>>>
>>> (This patch has to be applied after 'Use upstream version of fstrim
>>> instead of the local one')
>>>
>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>>> ---
>>>   286     |   58
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   286.out |    6 ++++++
>>
>> We've already got 286 (and 287), so this should probably be 288...
>>
>> Otherwise, it looks good.
>>
>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>>
>
> This has been committed to git://oss.sgi.com/xfs/xfs.git, master and
> for-next branches.
>
> Regards
> --Rich
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

My apologies, this has been committed to 
git://oss.sgi.com/xfs/cmds/xfstests, master and for-next branches.

Regards
--Rich

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-10-24 20:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-16  9:41 [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Lukas Czerner
2012-10-16  9:41 ` [PATCH 2/2] 251: Do not allow step to be zero Lukas Czerner
2012-10-16 20:18   ` Dave Chinner
2012-10-24 20:43     ` Rich Johnston
2012-10-16 20:16 ` [PATCH 1/2] 286: Test FITRIM where length is smaller than FSB Dave Chinner
2012-10-16 20:48   ` Ben Myers
2012-10-24 20:39   ` Rich Johnston
2012-10-24 20:44     ` Rich Johnston

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox