* [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