* [PATCH] 260: Add another corner case where length is zero
@ 2012-10-09 12:16 Lukas Czerner
2012-10-09 13:39 ` Carlos Maiolino
2012-10-09 19:40 ` Dave Chinner
0 siblings, 2 replies; 6+ messages in thread
From: Lukas Czerner @ 2012-10-09 12:16 UTC (permalink / raw)
To: xfs; +Cc: Lukas Czerner
This commit adds another corner case to test FITRIM argument handling.
In this case we set length to zero and we expect the number of discarded
bytes to be obviously zero, however we've had bug in both ext4 and xfs
where the internal variable would underflow. This test case will be able
to catch that in future.
Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
260 | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/260 b/260
index b005cd3..2ade9b3 100755
--- a/260
+++ b/260
@@ -98,6 +98,19 @@ _scratch_unmount
_scratch_mkfs >/dev/null 2>&1
_scratch_mount
+# This should return zero
+out=$("$FSTRIM" -v -s0 -l0 $SCRATCH_MNT)
+bytes=${out%% *}
+if [ $bytes -ne 0 ]; then
+ status=1
+ echo "We requested zero bytes to be discarded, however $bytes bytes"\
+ "were discarded instead. Length handling probably underflowed."
+fi
+
+_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
--
1.7.7.6
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 260: Add another corner case where length is zero
2012-10-09 12:16 [PATCH] 260: Add another corner case where length is zero Lukas Czerner
@ 2012-10-09 13:39 ` Carlos Maiolino
2012-10-09 19:40 ` Dave Chinner
1 sibling, 0 replies; 6+ messages in thread
From: Carlos Maiolino @ 2012-10-09 13:39 UTC (permalink / raw)
To: xfs
On Tue, Oct 09, 2012 at 02:16:52PM +0200, Lukas Czerner wrote:
> This commit adds another corner case to test FITRIM argument handling.
> In this case we set length to zero and we expect the number of discarded
> bytes to be obviously zero, however we've had bug in both ext4 and xfs
> where the internal variable would underflow. This test case will be able
> to catch that in future.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
> 260 | 13 +++++++++++++
> 1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/260 b/260
> index b005cd3..2ade9b3 100755
> --- a/260
> +++ b/260
> @@ -98,6 +98,19 @@ _scratch_unmount
> _scratch_mkfs >/dev/null 2>&1
> _scratch_mount
>
> +# This should return zero
> +out=$("$FSTRIM" -v -s0 -l0 $SCRATCH_MNT)
> +bytes=${out%% *}
> +if [ $bytes -ne 0 ]; then
> + status=1
> + echo "We requested zero bytes to be discarded, however $bytes bytes"\
> + "were discarded instead. Length handling probably underflowed."
> +fi
> +
> +_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
> --
> 1.7.7.6
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
--
--Carlos
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 260: Add another corner case where length is zero
2012-10-09 12:16 [PATCH] 260: Add another corner case where length is zero Lukas Czerner
2012-10-09 13:39 ` Carlos Maiolino
@ 2012-10-09 19:40 ` Dave Chinner
2012-10-09 21:23 ` Lukáš Czerner
1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-09 19:40 UTC (permalink / raw)
To: Lukas Czerner; +Cc: xfs
On Tue, Oct 09, 2012 at 02:16:52PM +0200, Lukas Czerner wrote:
> This commit adds another corner case to test FITRIM argument handling.
> In this case we set length to zero and we expect the number of discarded
> bytes to be obviously zero, however we've had bug in both ext4 and xfs
> where the internal variable would underflow. This test case will be able
> to catch that in future.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
I'd create another test for this, rather than making 260 suddenly
fail for everyone....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 260: Add another corner case where length is zero
2012-10-09 19:40 ` Dave Chinner
@ 2012-10-09 21:23 ` Lukáš Czerner
2012-10-09 21:39 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Lukáš Czerner @ 2012-10-09 21:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: Lukas Czerner, xfs
On Wed, 10 Oct 2012, Dave Chinner wrote:
> Date: Wed, 10 Oct 2012 06:40:19 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: xfs@oss.sgi.com
> Subject: Re: [PATCH] 260: Add another corner case where length is zero
>
> On Tue, Oct 09, 2012 at 02:16:52PM +0200, Lukas Czerner wrote:
> > This commit adds another corner case to test FITRIM argument handling.
> > In this case we set length to zero and we expect the number of discarded
> > bytes to be obviously zero, however we've had bug in both ext4 and xfs
> > where the internal variable would underflow. This test case will be able
> > to catch that in future.
> >
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>
> I'd create another test for this, rather than making 260 suddenly
> fail for everyone....
Hmm, I am not sure what is the point. I've created 260 exactly for
this reason of testing FITRIM argument handling and it already
contains number of tests like this one. I am not strongly against
having this in separate test, however it seems rather unnecessary to
me.
Thanks!
-Lukas
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 260: Add another corner case where length is zero
2012-10-09 21:23 ` Lukáš Czerner
@ 2012-10-09 21:39 ` Dave Chinner
2012-10-10 6:53 ` Lukáš Czerner
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-09 21:39 UTC (permalink / raw)
To: Lukáš Czerner; +Cc: xfs
On Tue, Oct 09, 2012 at 11:23:13PM +0200, Lukáš Czerner wrote:
> On Wed, 10 Oct 2012, Dave Chinner wrote:
>
> > Date: Wed, 10 Oct 2012 06:40:19 +1100
> > From: Dave Chinner <david@fromorbit.com>
> > To: Lukas Czerner <lczerner@redhat.com>
> > Cc: xfs@oss.sgi.com
> > Subject: Re: [PATCH] 260: Add another corner case where length is zero
> >
> > On Tue, Oct 09, 2012 at 02:16:52PM +0200, Lukas Czerner wrote:
> > > This commit adds another corner case to test FITRIM argument handling.
> > > In this case we set length to zero and we expect the number of discarded
> > > bytes to be obviously zero, however we've had bug in both ext4 and xfs
> > > where the internal variable would underflow. This test case will be able
> > > to catch that in future.
> > >
> > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> >
> > I'd create another test for this, rather than making 260 suddenly
> > fail for everyone....
>
> Hmm, I am not sure what is the point. I've created 260 exactly for
> this reason of testing FITRIM argument handling and it already
> contains number of tests like this one. I am not strongly against
> having this in separate test, however it seems rather unnecessary to
> me.
IT's a regression test - it's only supposed to start failing when
the kernel functionality is broken. That is, someone who is
tracking failures over time will suddenly see a new failure in 260
and wonder what kernel code broke, when in fact nothing was changed
in the kernel code. IOWs, changing the test invalidates all past
history of running the test, and that in turn breaks historic
regression tracking metrics...
This is why we historically have avoided changing existing tests and
instead wrote new tests, no matter how similar the functionality
between the old and new tests are.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 260: Add another corner case where length is zero
2012-10-09 21:39 ` Dave Chinner
@ 2012-10-10 6:53 ` Lukáš Czerner
0 siblings, 0 replies; 6+ messages in thread
From: Lukáš Czerner @ 2012-10-10 6:53 UTC (permalink / raw)
To: Dave Chinner; +Cc: Lukáš Czerner, xfs
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2232 bytes --]
On Wed, 10 Oct 2012, Dave Chinner wrote:
> Date: Wed, 10 Oct 2012 08:39:13 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: xfs@oss.sgi.com
> Subject: Re: [PATCH] 260: Add another corner case where length is zero
>
> On Tue, Oct 09, 2012 at 11:23:13PM +0200, Lukáš Czerner wrote:
> > On Wed, 10 Oct 2012, Dave Chinner wrote:
> >
> > > Date: Wed, 10 Oct 2012 06:40:19 +1100
> > > From: Dave Chinner <david@fromorbit.com>
> > > To: Lukas Czerner <lczerner@redhat.com>
> > > Cc: xfs@oss.sgi.com
> > > Subject: Re: [PATCH] 260: Add another corner case where length is zero
> > >
> > > On Tue, Oct 09, 2012 at 02:16:52PM +0200, Lukas Czerner wrote:
> > > > This commit adds another corner case to test FITRIM argument handling.
> > > > In this case we set length to zero and we expect the number of discarded
> > > > bytes to be obviously zero, however we've had bug in both ext4 and xfs
> > > > where the internal variable would underflow. This test case will be able
> > > > to catch that in future.
> > > >
> > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > >
> > > I'd create another test for this, rather than making 260 suddenly
> > > fail for everyone....
> >
> > Hmm, I am not sure what is the point. I've created 260 exactly for
> > this reason of testing FITRIM argument handling and it already
> > contains number of tests like this one. I am not strongly against
> > having this in separate test, however it seems rather unnecessary to
> > me.
>
> IT's a regression test - it's only supposed to start failing when
> the kernel functionality is broken. That is, someone who is
> tracking failures over time will suddenly see a new failure in 260
> and wonder what kernel code broke, when in fact nothing was changed
> in the kernel code. IOWs, changing the test invalidates all past
> history of running the test, and that in turn breaks historic
> regression tracking metrics...
>
> This is why we historically have avoided changing existing tests and
> instead wrote new tests, no matter how similar the functionality
> between the old and new tests are.
>
> Cheers,
>
> Dave.
>
Fair enough. I'll create a separate test for this.
Thanks!
-Lukas
[-- Attachment #2: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-10 6:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-09 12:16 [PATCH] 260: Add another corner case where length is zero Lukas Czerner
2012-10-09 13:39 ` Carlos Maiolino
2012-10-09 19:40 ` Dave Chinner
2012-10-09 21:23 ` Lukáš Czerner
2012-10-09 21:39 ` Dave Chinner
2012-10-10 6:53 ` Lukáš Czerner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox