| Message ID | 20161108091515.20867-1-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: > Due to the fact that btrfs uses different max extent size for > compressed and non-compressed write, it calculates metadata reservation > incorrectly. > > This can leads to false ENOSPC alert for compressed write. > > This test case will check it by using small fs and large nodesize, and > do parallel compressed write to trigger it. > > The fix is not merged and may change in the future, but the function is > good enough: > btrfs: improve inode's outstanding_extents computation > btrfs: fix false enospc for compression > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > tests/btrfs/132 | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/132.out | 2 ++ > tests/btrfs/group | 1 + > 3 files changed, 92 insertions(+) > create mode 100755 tests/btrfs/132 > create mode 100644 tests/btrfs/132.out > > diff --git a/tests/btrfs/132 b/tests/btrfs/132 > new file mode 100755 > index 0000000..95c21ea > --- /dev/null > +++ b/tests/btrfs/132 > @@ -0,0 +1,89 @@ > +#! /bin/bash > +# FS QA Test 132 > +# > +# Check if false ENOSPC will happen when parallel buffer write happens > +# The problem is caused by incorrect metadata reservation. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2016 Fujitsu. All Rights Reserved. > +# > +# 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 > +#----------------------------------------------------------------------- > +# > + > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +_cleanup() > +{ > + cd / > + rm -f $tmp.* > +} > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs btrfs > +_supported_os Linux > +_require_scratch > + > +# Use small filesystem with maximum nodesize. > +# Since the false ENOSPC happens due to incorrect metadata reservation, > +# larger nodesize and small fs will make it much easier to reproduce > +_scratch_mkfs -b 512M -n 64k >> $seqres.full 2>&1 How about override MKFS_OPTIONS and call _scratch_mkfs_sized? e.g. export MKFS_OPTIONS="-n 64k" _scratch_mkfs_sized $((512 * 1024 * 1024)) > +_scratch_mount "-o compress" > + > +sleep_time=$(($TIME_FACTOR * 15)) > +loop_writer() > +{ > + offset=$1 > + len=$2 > + file=$3 > + > + while true; > + do > + # Only need stderr, and we need to specify small block size > + # but still trigger compression > + xfs_io -c "pwrite -b 8K $offset $len" $file 2>&1 > /dev/null Use $XFS_IO_PROG here. > + done > +} > + > +touch $SCRATCH_MNT/testfile > + > +# Start 2 writter writting into the same file > +# The file is 142M, which is smaller than 1/2 of the filesystem, > +# so no other cause can lead to ENOSPC. > +loop_writer 0 128M $SCRATCH_MNT/testfile & > +loop_writer 128M 16M $SCRATCH_MNT/testfile & > + > +sleep $sleep_time > + > +kill 0 > +sleep This is not working for me, even I removed the suspicious "sleep", seems "132" itself was killed too. I'd save pids of background jobs and kill the pids and call wait to make sure all child processes exit running. e.g. loop_writer ... & pids=$! loop_writer ... & pids="$pids $!" sleep $sleep_time kill $pids wait And you're missing 'echo "Silence is golden"' in the test. Thanks, Eryu > + > +status=0 > +exit > diff --git a/tests/btrfs/132.out b/tests/btrfs/132.out > new file mode 100644 > index 0000000..b096312 > --- /dev/null > +++ b/tests/btrfs/132.out > @@ -0,0 +1,2 @@ > +QA output created by 132 > +Silence is golden > diff --git a/tests/btrfs/group b/tests/btrfs/group > index c090604..ec8ad80 100644 > --- a/tests/btrfs/group > +++ b/tests/btrfs/group > @@ -134,3 +134,4 @@ > 129 auto quick send > 130 auto clone send > 131 auto quick > +132 auto compress > -- > 2.7.4 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/08/2016 06:58 PM, Eryu Guan wrote: > On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: >> Due to the fact that btrfs uses different max extent size for >> compressed and non-compressed write, it calculates metadata reservation >> incorrectly. >> >> This can leads to false ENOSPC alert for compressed write. >> >> This test case will check it by using small fs and large nodesize, and >> do parallel compressed write to trigger it. >> >> The fix is not merged and may change in the future, but the function is >> good enough: >> btrfs: improve inode's outstanding_extents computation >> btrfs: fix false enospc for compression >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> tests/btrfs/132 | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/btrfs/132.out | 2 ++ >> tests/btrfs/group | 1 + >> 3 files changed, 92 insertions(+) >> create mode 100755 tests/btrfs/132 >> create mode 100644 tests/btrfs/132.out >> >> diff --git a/tests/btrfs/132 b/tests/btrfs/132 >> new file mode 100755 >> index 0000000..95c21ea >> --- /dev/null >> +++ b/tests/btrfs/132 >> @@ -0,0 +1,89 @@ >> +#! /bin/bash >> +# FS QA Test 132 >> +# >> +# Check if false ENOSPC will happen when parallel buffer write happens >> +# The problem is caused by incorrect metadata reservation. >> +# >> +#----------------------------------------------------------------------- >> +# Copyright (c) 2016 Fujitsu. All Rights Reserved. >> +# >> +# 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 >> +#----------------------------------------------------------------------- >> +# >> + >> +seq=`basename $0` >> +seqres=$RESULT_DIR/$seq >> +echo "QA output created by $seq" >> + >> +here=`pwd` >> +tmp=/tmp/$$ >> +status=1 # failure is the default! >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +_cleanup() >> +{ >> + cd / >> + rm -f $tmp.* >> +} >> + >> +# get standard environment, filters and checks >> +. ./common/rc >> +. ./common/filter >> + >> +# remove previous $seqres.full before test >> +rm -f $seqres.full >> + >> +# real QA test starts here >> + >> +# Modify as appropriate. >> +_supported_fs btrfs >> +_supported_os Linux >> +_require_scratch >> + >> +# Use small filesystem with maximum nodesize. >> +# Since the false ENOSPC happens due to incorrect metadata reservation, >> +# larger nodesize and small fs will make it much easier to reproduce >> +_scratch_mkfs -b 512M -n 64k >> $seqres.full 2>&1 > > How about override MKFS_OPTIONS and call _scratch_mkfs_sized? e.g. > > export MKFS_OPTIONS="-n 64k" > _scratch_mkfs_sized $((512 * 1024 * 1024)) Overriding MKFS_OPTIONS makes us unable to parse more mkfs options, for example to test some incompatible features. So I'd just parse this mkfs options as extra options. > >> +_scratch_mount "-o compress" >> + >> +sleep_time=$(($TIME_FACTOR * 15)) >> +loop_writer() >> +{ >> + offset=$1 >> + len=$2 >> + file=$3 >> + >> + while true; >> + do >> + # Only need stderr, and we need to specify small block size >> + # but still trigger compression >> + xfs_io -c "pwrite -b 8K $offset $len" $file 2>&1 > /dev/null > > Use $XFS_IO_PROG here. Oh, I forgot it again... > >> + done >> +} >> + >> +touch $SCRATCH_MNT/testfile >> + >> +# Start 2 writter writting into the same file >> +# The file is 142M, which is smaller than 1/2 of the filesystem, >> +# so no other cause can lead to ENOSPC. >> +loop_writer 0 128M $SCRATCH_MNT/testfile & >> +loop_writer 128M 16M $SCRATCH_MNT/testfile & >> + >> +sleep $sleep_time >> + >> +kill 0 >> +sleep Oh, I thought I typed "wait" not "sleep". > > This is not working for me, even I removed the suspicious "sleep", seems > "132" itself was killed too. > > I'd save pids of background jobs and kill the pids and call wait to make > sure all child processes exit running. e.g. > > loop_writer ... & > pids=$! > loop_writer ... & > pids="$pids $!" > > sleep $sleep_time > kill $pids > wait > > And you're missing 'echo "Silence is golden"' in the test. I'll update them soon. Thanks, Qu > > Thanks, > Eryu > >> + >> +status=0 >> +exit >> diff --git a/tests/btrfs/132.out b/tests/btrfs/132.out >> new file mode 100644 >> index 0000000..b096312 >> --- /dev/null >> +++ b/tests/btrfs/132.out >> @@ -0,0 +1,2 @@ >> +QA output created by 132 >> +Silence is golden >> diff --git a/tests/btrfs/group b/tests/btrfs/group >> index c090604..ec8ad80 100644 >> --- a/tests/btrfs/group >> +++ b/tests/btrfs/group >> @@ -134,3 +134,4 @@ >> 129 auto quick send >> 130 auto clone send >> 131 auto quick >> +132 auto compress >> -- >> 2.7.4 >> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe fstests" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: > > > At 11/08/2016 06:58 PM, Eryu Guan wrote: > > On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: > > > Due to the fact that btrfs uses different max extent size for > > > compressed and non-compressed write, it calculates metadata reservation > > > incorrectly. > > > > > > This can leads to false ENOSPC alert for compressed write. > > > > > > This test case will check it by using small fs and large nodesize, and > > > do parallel compressed write to trigger it. > > > > > > The fix is not merged and may change in the future, but the function is > > > good enough: > > > btrfs: improve inode's outstanding_extents computation > > > btrfs: fix false enospc for compression > > > > > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > > --- [snip] > > > + > > > +# Use small filesystem with maximum nodesize. > > > +# Since the false ENOSPC happens due to incorrect metadata reservation, > > > +# larger nodesize and small fs will make it much easier to reproduce > > > +_scratch_mkfs -b 512M -n 64k >> $seqres.full 2>&1 > > > > How about override MKFS_OPTIONS and call _scratch_mkfs_sized? e.g. > > > > export MKFS_OPTIONS="-n 64k" > > _scratch_mkfs_sized $((512 * 1024 * 1024)) > > Overriding MKFS_OPTIONS makes us unable to parse more mkfs options, for > example to test some incompatible features. > > So I'd just parse this mkfs options as extra options. We should use helpers and not open-code when helpers are available. So we should really use _scratch_mkfs_sized here. And "-n 64k" is only to make bug easier to reproduce, but I don't think it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on my x86_64 test vm, even I removed the "-n 64k" mkfs option from the test. So I'd say remove "-n 64k" and test whatever mkfs options user specified. > > > > > +_scratch_mount "-o compress" And compress mount option is not necessary to me either. btrfs compress is a commonly tested configuration, there's no need to force the compress option in the test. So we can test other configurations too. Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/09/2016 05:43 PM, Eryu Guan wrote: > On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: >> >> >> At 11/08/2016 06:58 PM, Eryu Guan wrote: >>> On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: >>>> Due to the fact that btrfs uses different max extent size for >>>> compressed and non-compressed write, it calculates metadata reservation >>>> incorrectly. >>>> >>>> This can leads to false ENOSPC alert for compressed write. >>>> >>>> This test case will check it by using small fs and large nodesize, and >>>> do parallel compressed write to trigger it. >>>> >>>> The fix is not merged and may change in the future, but the function is >>>> good enough: >>>> btrfs: improve inode's outstanding_extents computation >>>> btrfs: fix false enospc for compression >>>> >>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>>> --- > [snip] >>>> + >>>> +# Use small filesystem with maximum nodesize. >>>> +# Since the false ENOSPC happens due to incorrect metadata reservation, >>>> +# larger nodesize and small fs will make it much easier to reproduce >>>> +_scratch_mkfs -b 512M -n 64k >> $seqres.full 2>&1 >>> >>> How about override MKFS_OPTIONS and call _scratch_mkfs_sized? e.g. >>> >>> export MKFS_OPTIONS="-n 64k" >>> _scratch_mkfs_sized $((512 * 1024 * 1024)) >> >> Overriding MKFS_OPTIONS makes us unable to parse more mkfs options, for >> example to test some incompatible features. >> >> So I'd just parse this mkfs options as extra options. > > We should use helpers and not open-code when helpers are available. So > we should really use _scratch_mkfs_sized here. > > And "-n 64k" is only to make bug easier to reproduce, but I don't think > it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on > my x86_64 test vm, even I removed the "-n 64k" mkfs option from the > test. > > So I'd say remove "-n 64k" and test whatever mkfs options user > specified. I really don't like the idea to allow user to override any mount option to reduce the possibility. If using 4K nodesize, the possibility to trigger the bug will be quite low. (default is 16K, so you still have chance to reproduce the bug) And further more, this testcase is not a generic test, but a regression/pinpoint test to expose one specific bug. So I'll put anything to improve the possibility and won't allow user options to override it. > >>> >>>> +_scratch_mount "-o compress" > > And compress mount option is not necessary to me either. btrfs compress > is a commonly tested configuration, there's no need to force the > compress option in the test. So we can test other configurations too. As stated above, this is a regression test, not a generic test. So I still need the "-o compress" mount option. We shouldn't dependent on user to specify any special mount option, since tester/user are never reliable. Further more, there are already lots of test cases specifying their own mount option. So I don't see there is anything wrong using specified mount option. Thanks, Qu > > Thanks, > Eryu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 08:34:20AM +0800, Qu Wenruo wrote: > At 11/09/2016 05:43 PM, Eryu Guan wrote: > >On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: > >>At 11/08/2016 06:58 PM, Eryu Guan wrote: > >>>On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: > >We should use helpers and not open-code when helpers are available. So > >we should really use _scratch_mkfs_sized here. > > > >And "-n 64k" is only to make bug easier to reproduce, but I don't think > >it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on > >my x86_64 test vm, even I removed the "-n 64k" mkfs option from the > >test. > > > >So I'd say remove "-n 64k" and test whatever mkfs options user > >specified. > > I really don't like the idea to allow user to override any mount > option to reduce the possibility. That's not the point. Overriding mount options reduces test coverage because it limits the test to only the exact configuration that reproduced the bug that was seen. If a user is testing a specific configuration, then we really want to run the test on that config. > And further more, this testcase is not a generic test, but a > regression/pinpoint test to expose one specific bug. If you want to make sure that the bug doesn't return, then you need to run the /entire test suite/ with the configuration that exposes the problem. You wouldn't be suggesting this specific set of mount options if users weren't using that configuration. Hence you really need to run the entire test suite with that configuration to make sure you haven't broken those user's systems.... And, yes, I test lots of different XFS configurations in their entirity every day on every change I make or review, so I'm not suggesting that you should do anything I don't already do. Cheers, Dave.
At 11/10/2016 10:19 AM, Dave Chinner wrote: > On Thu, Nov 10, 2016 at 08:34:20AM +0800, Qu Wenruo wrote: >> At 11/09/2016 05:43 PM, Eryu Guan wrote: >>> On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: >>>> At 11/08/2016 06:58 PM, Eryu Guan wrote: >>>>> On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: >>> We should use helpers and not open-code when helpers are available. So >>> we should really use _scratch_mkfs_sized here. >>> >>> And "-n 64k" is only to make bug easier to reproduce, but I don't think >>> it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on >>> my x86_64 test vm, even I removed the "-n 64k" mkfs option from the >>> test. >>> >>> So I'd say remove "-n 64k" and test whatever mkfs options user >>> specified. >> >> I really don't like the idea to allow user to override any mount >> option to reduce the possibility. > > That's not the point. Overriding mount options reduces test coverage > because it limits the test to only the exact configuration that > reproduced the bug that was seen. If a user is testing a specific > configuration, then we really want to run the test on that config. > >> And further more, this testcase is not a generic test, but a >> regression/pinpoint test to expose one specific bug. > > If you want to make sure that the bug doesn't return, then you need > to run the /entire test suite/ with the configuration that exposes > the problem. You wouldn't be suggesting this specific set of mount > options if users weren't using that configuration. Hence you really > need to run the entire test suite with that configuration to make > sure you haven't broken those user's systems.... > > And, yes, I test lots of different XFS configurations in their > entirity every day on every change I make or review, so I'm not > suggesting that you should do anything I don't already do. OK, most of your points makes sense. I'll update the case. And I want to make it clear, doesn it mean, newly submitted test case shouldn't specify any mkfs/mount option? Another concern is, I'm not sure if there is anyone really runs all btrfs mount and mkfs options on the entire test suite. (The same is for developers who submits generic test cases) Or there won't be so many generic or even btrfs specific test cases causing false alert or exposing new bugs of btrfs. Although this is problem of btrfs. :( Thanks, Qu > > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 10:42:36AM +0800, Qu Wenruo wrote: > > > At 11/10/2016 10:19 AM, Dave Chinner wrote: > > On Thu, Nov 10, 2016 at 08:34:20AM +0800, Qu Wenruo wrote: > > > At 11/09/2016 05:43 PM, Eryu Guan wrote: > > > > On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: > > > > > At 11/08/2016 06:58 PM, Eryu Guan wrote: > > > > > > On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: > > > > We should use helpers and not open-code when helpers are available. So > > > > we should really use _scratch_mkfs_sized here. > > > > > > > > And "-n 64k" is only to make bug easier to reproduce, but I don't think > > > > it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on > > > > my x86_64 test vm, even I removed the "-n 64k" mkfs option from the > > > > test. > > > > > > > > So I'd say remove "-n 64k" and test whatever mkfs options user > > > > specified. > > > > > > I really don't like the idea to allow user to override any mount > > > option to reduce the possibility. > > > > That's not the point. Overriding mount options reduces test coverage > > because it limits the test to only the exact configuration that > > reproduced the bug that was seen. If a user is testing a specific > > configuration, then we really want to run the test on that config. > > > > > And further more, this testcase is not a generic test, but a > > > regression/pinpoint test to expose one specific bug. > > > > If you want to make sure that the bug doesn't return, then you need > > to run the /entire test suite/ with the configuration that exposes > > the problem. You wouldn't be suggesting this specific set of mount > > options if users weren't using that configuration. Hence you really > > need to run the entire test suite with that configuration to make > > sure you haven't broken those user's systems.... > > > > And, yes, I test lots of different XFS configurations in their > > entirity every day on every change I make or review, so I'm not > > suggesting that you should do anything I don't already do. > > OK, most of your points makes sense. > I'll update the case. > > And I want to make it clear, doesn it mean, newly submitted test case > shouldn't specify any mkfs/mount option? My understanding is that if test needs a extremely rare configuration to hit a bug, then it should specify mkfs/mount options, e.g. xfs/297 tests a bug in XFS only in very specific configuration: _scratch_mkfs_xfs -d agcount=16,su=256k,sw=12 -l su=256k,size=5120b >/dev/null 2>&1 But if the bug can be reproduced by a commonly tested configuration, e.g. btrfs with compress enabled, we should keep test coverage as large as possible, not limit it to a certain config. The purpose is to not lose any test coverage if possible, unless you have a good reason to do so. > > Another concern is, I'm not sure if there is anyone really runs all btrfs > mount and mkfs options on the entire test suite. > (The same is for developers who submits generic test cases) It's not realistic to run full matrix of conbinations of mkfs and mount options, but the commonly used configurations should be tested. I think this also depends, if you care about a certain configuration, or a distro provides full support to a certain configuration and you work for the company behind it, as a developer or tester you should really test it. e.g. (as a negative example) I rarely test ext4 with "data=journal" mount option, because Red Hat doesn't support this configuration :) Thanks, Eryu > > Or there won't be so many generic or even btrfs specific test cases causing > false alert or exposing new bugs of btrfs. > > Although this is problem of btrfs. :( > > Thanks, > Qu > > > > > Cheers, > > > > Dave. > > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 11:24:34AM +0800, Eryu Guan wrote: > On Thu, Nov 10, 2016 at 10:42:36AM +0800, Qu Wenruo wrote: > > > > > > At 11/10/2016 10:19 AM, Dave Chinner wrote: > > > On Thu, Nov 10, 2016 at 08:34:20AM +0800, Qu Wenruo wrote: > > > > At 11/09/2016 05:43 PM, Eryu Guan wrote: > > > > > On Wed, Nov 09, 2016 at 08:24:38AM +0800, Qu Wenruo wrote: > > > > > > At 11/08/2016 06:58 PM, Eryu Guan wrote: > > > > > > > On Tue, Nov 08, 2016 at 05:15:15PM +0800, Qu Wenruo wrote: > > > > > We should use helpers and not open-code when helpers are available. So > > > > > we should really use _scratch_mkfs_sized here. > > > > > > > > > > And "-n 64k" is only to make bug easier to reproduce, but I don't think > > > > > it's necessary. In my testings, 50%-75% runs hit the ENOSPC failure on > > > > > my x86_64 test vm, even I removed the "-n 64k" mkfs option from the > > > > > test. > > > > > > > > > > So I'd say remove "-n 64k" and test whatever mkfs options user > > > > > specified. > > > > > > > > I really don't like the idea to allow user to override any mount > > > > option to reduce the possibility. > > > > > > That's not the point. Overriding mount options reduces test coverage > > > because it limits the test to only the exact configuration that > > > reproduced the bug that was seen. If a user is testing a specific > > > configuration, then we really want to run the test on that config. > > > > > > > And further more, this testcase is not a generic test, but a > > > > regression/pinpoint test to expose one specific bug. > > > > > > If you want to make sure that the bug doesn't return, then you need > > > to run the /entire test suite/ with the configuration that exposes > > > the problem. You wouldn't be suggesting this specific set of mount > > > options if users weren't using that configuration. Hence you really > > > need to run the entire test suite with that configuration to make > > > sure you haven't broken those user's systems.... > > > > > > And, yes, I test lots of different XFS configurations in their > > > entirity every day on every change I make or review, so I'm not > > > suggesting that you should do anything I don't already do. > > > > OK, most of your points makes sense. > > I'll update the case. > > > > And I want to make it clear, doesn it mean, newly submitted test case > > shouldn't specify any mkfs/mount option? > > My understanding is that if test needs a extremely rare configuration to > hit a bug, then it should specify mkfs/mount options, e.g. xfs/297 tests > a bug in XFS only in very specific configuration: > > _scratch_mkfs_xfs -d agcount=16,su=256k,sw=12 -l su=256k,size=5120b >/dev/null 2>&1 Yes, but there's a key reason we can do this for /XFS/. I point this out every time this comes up: We can do this with /XFS/ because the *implementation of _scratch_mkfs_xfs()* has a fallback strategy on failure. That is, _scratch_mkfs_xfs will first try to make the filesystems with $MKFS_OPTIONS + $TEST_SUPPLIED_OPTIONS. If that fails, it then runs mkfs with just $TEST_SUPPLIED_OPTIONS. IOWs, XFS always tries to use the global options, and only removes them when there is a problem combining them with $TEST_SUPPLIED_OPTIONS. This is how custom test options should be applied for all filesystems, not just XFS. It gets rid of the need for any test to care about MKFS_OPTIONS. What we are also missing is that we need to apply this process to scratch mount options as we don't do it for any filesystem. That will get rid of the need for tests to care about what is in MOUNT_OPTIONS, too... Cheers, Dave.
diff --git a/tests/btrfs/132 b/tests/btrfs/132 new file mode 100755 index 0000000..95c21ea --- /dev/null +++ b/tests/btrfs/132 @@ -0,0 +1,89 @@ +#! /bin/bash +# FS QA Test 132 +# +# Check if false ENOSPC will happen when parallel buffer write happens +# The problem is caused by incorrect metadata reservation. +# +#----------------------------------------------------------------------- +# Copyright (c) 2016 Fujitsu. All Rights Reserved. +# +# 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 +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs btrfs +_supported_os Linux +_require_scratch + +# Use small filesystem with maximum nodesize. +# Since the false ENOSPC happens due to incorrect metadata reservation, +# larger nodesize and small fs will make it much easier to reproduce +_scratch_mkfs -b 512M -n 64k >> $seqres.full 2>&1 +_scratch_mount "-o compress" + +sleep_time=$(($TIME_FACTOR * 15)) +loop_writer() +{ + offset=$1 + len=$2 + file=$3 + + while true; + do + # Only need stderr, and we need to specify small block size + # but still trigger compression + xfs_io -c "pwrite -b 8K $offset $len" $file 2>&1 > /dev/null + done +} + +touch $SCRATCH_MNT/testfile + +# Start 2 writter writting into the same file +# The file is 142M, which is smaller than 1/2 of the filesystem, +# so no other cause can lead to ENOSPC. +loop_writer 0 128M $SCRATCH_MNT/testfile & +loop_writer 128M 16M $SCRATCH_MNT/testfile & + +sleep $sleep_time + +kill 0 +sleep + +status=0 +exit diff --git a/tests/btrfs/132.out b/tests/btrfs/132.out new file mode 100644 index 0000000..b096312 --- /dev/null +++ b/tests/btrfs/132.out @@ -0,0 +1,2 @@ +QA output created by 132 +Silence is golden diff --git a/tests/btrfs/group b/tests/btrfs/group index c090604..ec8ad80 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -134,3 +134,4 @@ 129 auto quick send 130 auto clone send 131 auto quick +132 auto compress
Due to the fact that btrfs uses different max extent size for compressed and non-compressed write, it calculates metadata reservation incorrectly. This can leads to false ENOSPC alert for compressed write. This test case will check it by using small fs and large nodesize, and do parallel compressed write to trigger it. The fix is not merged and may change in the future, but the function is good enough: btrfs: improve inode's outstanding_extents computation btrfs: fix false enospc for compression Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- tests/btrfs/132 | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/132.out | 2 ++ tests/btrfs/group | 1 + 3 files changed, 92 insertions(+) create mode 100755 tests/btrfs/132 create mode 100644 tests/btrfs/132.out