| Message ID | 20170523080205.12556-1-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: > [BUG] > If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 > generic/143 and generic/154, it will cause false alert like: > cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" should fix your problem. Yeah, MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS (and almost all other global variables) are not documented properly.. Thanks, Eryu > > [REASON] > It is caused by _test_cycle_mount function, which unmount test device, > but when trying to re-mount it again using _test_mount(), we don't pass > $MOUNT_OPTIONS. > > So this makes mount options differs between _test_cycle_mount(). > > And btrfs doesn't allow different csum flags between reflink source and > destination inodes, so it returns -EINVAL for reflink operation. > > [FIX] > Fix it by passing $MOUNT_OPTIONS to _test_mount(), so that > _test_cycle_mount() won't cause different mount options. > So btrfs with "-o nodatasum" mount option can pass generic/14[23] > and generic/154 without false alert. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > common/rc | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/common/rc b/common/rc > index ba215961..a591907c 100644 > --- a/common/rc > +++ b/common/rc > @@ -522,7 +522,8 @@ _test_mount() > return $? > fi > _test_options mount > - _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > + _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS \ > + $MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > } > > _test_unmount() > -- > 2.13.0 > > > > -- > 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 05/23/2017 07:13 PM, Eryu Guan wrote: > On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: >> [BUG] >> If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 >> generic/143 and generic/154, it will cause false alert like: >> cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument > > MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test > dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" > should fix your problem. Nope, the problem is the inconsistent of TEST_MNT setup. As I described, the TEST_MNT is setup *with* MOUNT_OPTIONS for the 1st time, maybe by "check" script. But _test_cycle_mount() uses TEST_FS_MOUNT_OPTS otherthan MOUNT_OPTIONS, and leads to the problem. If using TEST_FS_MOUNT_OPTS, then 1st setup should also use TEST_FS_MOUNT_OPTS. Thanks, Qu > > Yeah, MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS (and almost all other global > variables) are not documented properly.. > > Thanks, > Eryu > >> >> [REASON] >> It is caused by _test_cycle_mount function, which unmount test device, >> but when trying to re-mount it again using _test_mount(), we don't pass >> $MOUNT_OPTIONS. >> >> So this makes mount options differs between _test_cycle_mount(). >> >> And btrfs doesn't allow different csum flags between reflink source and >> destination inodes, so it returns -EINVAL for reflink operation. >> >> [FIX] >> Fix it by passing $MOUNT_OPTIONS to _test_mount(), so that >> _test_cycle_mount() won't cause different mount options. >> So btrfs with "-o nodatasum" mount option can pass generic/14[23] >> and generic/154 without false alert. >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> common/rc | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/common/rc b/common/rc >> index ba215961..a591907c 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -522,7 +522,8 @@ _test_mount() >> return $? >> fi >> _test_options mount >> - _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR >> + _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS \ >> + $MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR >> } >> >> _test_unmount() >> -- >> 2.13.0 >> >> >> >> -- >> 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 Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: > > > At 05/23/2017 07:13 PM, Eryu Guan wrote: > > On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: > > > [BUG] > > > If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 > > > generic/143 and generic/154, it will cause false alert like: > > > cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument > > > > MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test > > dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" > > should fix your problem. > > Nope, the problem is the inconsistent of TEST_MNT setup. It does fix the failure for me, did I miss anything? # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o nodatasum" ./check generic/142 generic/143 generic/154 FSTYP -- btrfs PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 MKFS_OPTIONS -- /dev/sda6 MOUNT_OPTIONS -- -o nodatasum -o context=system_u:object_r:root_t:s0 /dev/sda6 /mnt/testarea/scratch generic/142 2s ... 1s generic/143 18s generic/154 1s Ran: generic/142 generic/143 generic/154 Passed all 3 tests Thanks, Eryu > > As I described, the TEST_MNT is setup *with* MOUNT_OPTIONS for the 1st time, > maybe by "check" script. > > But _test_cycle_mount() uses TEST_FS_MOUNT_OPTS otherthan MOUNT_OPTIONS, and > leads to the problem. > > If using TEST_FS_MOUNT_OPTS, then 1st setup should also use > TEST_FS_MOUNT_OPTS. > > Thanks, > Qu > > > > > Yeah, MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS (and almost all other global > > variables) are not documented properly.. > > > > Thanks, > > Eryu > > > > > > > > [REASON] > > > It is caused by _test_cycle_mount function, which unmount test device, > > > but when trying to re-mount it again using _test_mount(), we don't pass > > > $MOUNT_OPTIONS. > > > > > > So this makes mount options differs between _test_cycle_mount(). > > > > > > And btrfs doesn't allow different csum flags between reflink source and > > > destination inodes, so it returns -EINVAL for reflink operation. > > > > > > [FIX] > > > Fix it by passing $MOUNT_OPTIONS to _test_mount(), so that > > > _test_cycle_mount() won't cause different mount options. > > > So btrfs with "-o nodatasum" mount option can pass generic/14[23] > > > and generic/154 without false alert. > > > > > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > > --- > > > common/rc | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/common/rc b/common/rc > > > index ba215961..a591907c 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -522,7 +522,8 @@ _test_mount() > > > return $? > > > fi > > > _test_options mount > > > - _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > > > + _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS \ > > > + $MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > > > } > > > _test_unmount() > > > -- > > > 2.13.0 > > > > > > > > > > > > -- > > > 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 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 05/24/2017 12:24 PM, Eryu Guan wrote: > On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: >> >> >> At 05/23/2017 07:13 PM, Eryu Guan wrote: >>> On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: >>>> [BUG] >>>> If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 >>>> generic/143 and generic/154, it will cause false alert like: >>>> cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument >>> >>> MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test >>> dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" >>> should fix your problem. >> >> Nope, the problem is the inconsistent of TEST_MNT setup. > > It does fix the failure for me, did I miss anything? > > # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o nodatasum" ./check generic/142 generic/143 generic/154 > FSTYP -- btrfs > PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 > MKFS_OPTIONS -- /dev/sda6 > MOUNT_OPTIONS -- -o nodatasum -o context=system_u:object_r:root_t:s0 /dev/sda6 /mnt/testarea/scratch > > generic/142 2s ... 1s > generic/143 18s > generic/154 1s > Ran: generic/142 generic/143 generic/154 > Passed all 3 tests > But if you only export MOUNT_OPTIONS, it will fail, due to the different mount options between test_cycle_mount(). To make it clear: If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and test_cycle_mount should follow TEST_FS_MOUNT_OPTS. If it follows MOUNT_OPTIONS, then both. Not one follows MOUNT_OPTIONS and the other follows TEST_FS_MOUNT_OPTS. THanks, Qu > Thanks, > Eryu > >> >> As I described, the TEST_MNT is setup *with* MOUNT_OPTIONS for the 1st time, >> maybe by "check" script. >> >> But _test_cycle_mount() uses TEST_FS_MOUNT_OPTS otherthan MOUNT_OPTIONS, and >> leads to the problem. >> >> If using TEST_FS_MOUNT_OPTS, then 1st setup should also use >> TEST_FS_MOUNT_OPTS. >> >> Thanks, >> Qu >> >>> >>> Yeah, MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS (and almost all other global >>> variables) are not documented properly.. >>> >>> Thanks, >>> Eryu >>> >>>> >>>> [REASON] >>>> It is caused by _test_cycle_mount function, which unmount test device, >>>> but when trying to re-mount it again using _test_mount(), we don't pass >>>> $MOUNT_OPTIONS. >>>> >>>> So this makes mount options differs between _test_cycle_mount(). >>>> >>>> And btrfs doesn't allow different csum flags between reflink source and >>>> destination inodes, so it returns -EINVAL for reflink operation. >>>> >>>> [FIX] >>>> Fix it by passing $MOUNT_OPTIONS to _test_mount(), so that >>>> _test_cycle_mount() won't cause different mount options. >>>> So btrfs with "-o nodatasum" mount option can pass generic/14[23] >>>> and generic/154 without false alert. >>>> >>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>>> --- >>>> common/rc | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index ba215961..a591907c 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -522,7 +522,8 @@ _test_mount() >>>> return $? >>>> fi >>>> _test_options mount >>>> - _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR >>>> + _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS \ >>>> + $MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR >>>> } >>>> _test_unmount() >>>> -- >>>> 2.13.0 >>>> >>>> >>>> >>>> -- >>>> 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 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 Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: > > > At 05/24/2017 12:24 PM, Eryu Guan wrote: > > On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: > > > > > > > > > At 05/23/2017 07:13 PM, Eryu Guan wrote: > > > > On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: > > > > > [BUG] > > > > > If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 > > > > > generic/143 and generic/154, it will cause false alert like: > > > > > cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument > > > > > > > > MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test > > > > dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" > > > > should fix your problem. > > > > > > Nope, the problem is the inconsistent of TEST_MNT setup. > > > > It does fix the failure for me, did I miss anything? > > > > # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o nodatasum" ./check generic/142 generic/143 generic/154 > > FSTYP -- btrfs > > PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 > > MKFS_OPTIONS -- /dev/sda6 > > MOUNT_OPTIONS -- -o nodatasum -o context=system_u:object_r:root_t:s0 /dev/sda6 /mnt/testarea/scratch > > > > generic/142 2s ... 1s > > generic/143 18s > > generic/154 1s > > Ran: generic/142 generic/143 generic/154 > > Passed all 3 tests > > > > But if you only export MOUNT_OPTIONS, it will fail, due to the different > mount options between test_cycle_mount(). That's correct. Sorry, I didn't make it clear in my first reply. I meant that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to "-onodatasum", for both test dev and scratch dev. > > To make it clear: > If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and > test_cycle_mount should follow TEST_FS_MOUNT_OPTS. _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter which mount it is. 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 05/24/2017 01:08 PM, Eryu Guan wrote: > On Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: >> >> >> At 05/24/2017 12:24 PM, Eryu Guan wrote: >>> On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: >>>> >>>> >>>> At 05/23/2017 07:13 PM, Eryu Guan wrote: >>>>> On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: >>>>>> [BUG] >>>>>> If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 >>>>>> generic/143 and generic/154, it will cause false alert like: >>>>>> cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument >>>>> >>>>> MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for test >>>>> dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" >>>>> should fix your problem. >>>> >>>> Nope, the problem is the inconsistent of TEST_MNT setup. >>> >>> It does fix the failure for me, did I miss anything? >>> >>> # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o nodatasum" ./check generic/142 generic/143 generic/154 >>> FSTYP -- btrfs >>> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 >>> MKFS_OPTIONS -- /dev/sda6 >>> MOUNT_OPTIONS -- -o nodatasum -o context=system_u:object_r:root_t:s0 /dev/sda6 /mnt/testarea/scratch >>> >>> generic/142 2s ... 1s >>> generic/143 18s >>> generic/154 1s >>> Ran: generic/142 generic/143 generic/154 >>> Passed all 3 tests >>> >> >> But if you only export MOUNT_OPTIONS, it will fail, due to the different >> mount options between test_cycle_mount(). > > That's correct. Sorry, I didn't make it clear in my first reply. I meant > that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to > "-onodatasum", for both test dev and scratch dev. That's just a workaround, not a root fix. Not to mention quite a lot test cases lose its coverage. As after _test_cycle_mount(), they are just testing default mount option. > >> >> To make it clear: >> If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and >> test_cycle_mount should follow TEST_FS_MOUNT_OPTS. > > _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter > which mount it is. While test mount setup by check script follows MOUNT_OPTIONS. Just check the following very basic test script: ------ #! /bin/bash # FS QA Test 010 # # what am I here for? # 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 generic _supported_os Linux _require_test mount > $tmp.mount1 _test_cycle_mount mount > $tmp.mount2 diff $tmp.mount1 $tmp.mount2 echo "Silence is golden" # success, all done status=0 exit ------ 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
At 05/24/2017 01:16 PM, Qu Wenruo wrote: > > > At 05/24/2017 01:08 PM, Eryu Guan wrote: >> On Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: >>> >>> >>> At 05/24/2017 12:24 PM, Eryu Guan wrote: >>>> On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: >>>>> >>>>> >>>>> At 05/23/2017 07:13 PM, Eryu Guan wrote: >>>>>> On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: >>>>>>> [BUG] >>>>>>> If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 >>>>>>> generic/143 and generic/154, it will cause false alert like: >>>>>>> cp: failed to clone '/mnt/test/test-154/file2' from >>>>>>> '/mnt/test/test-154/file1': Invalid argument >>>>>> >>>>>> MOUNT_OPTIONS is for scratch mount, and TEST_FS_MOUNT_OPTS is for >>>>>> test >>>>>> dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" >>>>>> should fix your problem. >>>>> >>>>> Nope, the problem is the inconsistent of TEST_MNT setup. >>>> >>>> It does fix the failure for me, did I miss anything? >>>> >>>> # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o nodatasum" >>>> ./check generic/142 generic/143 generic/154 >>>> FSTYP -- btrfs >>>> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 >>>> MKFS_OPTIONS -- /dev/sda6 >>>> MOUNT_OPTIONS -- -o nodatasum -o context=system_u:object_r:root_t:s0 >>>> /dev/sda6 /mnt/testarea/scratch >>>> >>>> generic/142 2s ... 1s >>>> generic/143 18s >>>> generic/154 1s >>>> Ran: generic/142 generic/143 generic/154 >>>> Passed all 3 tests >>>> >>> >>> But if you only export MOUNT_OPTIONS, it will fail, due to the different >>> mount options between test_cycle_mount(). >> >> That's correct. Sorry, I didn't make it clear in my first reply. I meant >> that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to >> "-onodatasum", for both test dev and scratch dev. > > That's just a workaround, not a root fix. > > Not to mention quite a lot test cases lose its coverage. > As after _test_cycle_mount(), they are just testing default mount option. > >> >>> >>> To make it clear: >>> If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and >>> test_cycle_mount should follow TEST_FS_MOUNT_OPTS. >> >> _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter >> which mount it is. > > While test mount setup by check script follows MOUNT_OPTIONS. Well, $MOUNT_OPTIONS is appended by check_generic_filsystem() or check_btrfs_filesystem() for btrfs. Which will remount the device with $MOUNT_OPTIONS. Further more, this function doesn't care if it's scratch device or test device, just use MOUNT_OPTIONS. So either we split it for scratch and test device, and follows different mount options, or just like my submitted patch, to make _test_mount() to follow MOUNT_OPTIONS. Thanks, Qu > > Just check the following very basic test script: > ------ > #! /bin/bash > # FS QA Test 010 > # > # what am I here for? > # > 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 generic > _supported_os Linux > _require_test > > mount > $tmp.mount1 > _test_cycle_mount > mount > $tmp.mount2 > > diff $tmp.mount1 $tmp.mount2 > > echo "Silence is golden" > # success, all done > status=0 > exit > > ------ > > 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 Wed, May 24, 2017 at 03:58:11PM +0800, Qu Wenruo wrote: > > > At 05/24/2017 01:16 PM, Qu Wenruo wrote: > > > > > > At 05/24/2017 01:08 PM, Eryu Guan wrote: > > > On Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: > > > > > > > > > > > > At 05/24/2017 12:24 PM, Eryu Guan wrote: > > > > > On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: > > > > > > > > > > > > > > > > > > At 05/23/2017 07:13 PM, Eryu Guan wrote: > > > > > > > On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: > > > > > > > > [BUG] > > > > > > > > If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 > > > > > > > > generic/143 and generic/154, it will cause false alert like: > > > > > > > > cp: failed to clone '/mnt/test/test-154/file2' > > > > > > > > from '/mnt/test/test-154/file1': Invalid > > > > > > > > argument > > > > > > > > > > > > > > MOUNT_OPTIONS is for scratch mount, and > > > > > > > TEST_FS_MOUNT_OPTS is for test > > > > > > > dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" > > > > > > > should fix your problem. > > > > > > > > > > > > Nope, the problem is the inconsistent of TEST_MNT setup. > > > > > > > > > > It does fix the failure for me, did I miss anything? > > > > > > > > > > # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o > > > > > nodatasum" ./check generic/142 generic/143 generic/154 > > > > > FSTYP -- btrfs > > > > > PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 > > > > > MKFS_OPTIONS -- /dev/sda6 > > > > > MOUNT_OPTIONS -- -o nodatasum -o > > > > > context=system_u:object_r:root_t:s0 /dev/sda6 > > > > > /mnt/testarea/scratch > > > > > > > > > > generic/142 2s ... 1s > > > > > generic/143 18s > > > > > generic/154 1s > > > > > Ran: generic/142 generic/143 generic/154 > > > > > Passed all 3 tests > > > > > > > > > > > > > But if you only export MOUNT_OPTIONS, it will fail, due to the different > > > > mount options between test_cycle_mount(). > > > > > > That's correct. Sorry, I didn't make it clear in my first reply. I meant > > > that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to > > > "-onodatasum", for both test dev and scratch dev. > > > > That's just a workaround, not a root fix. > > > > Not to mention quite a lot test cases lose its coverage. > > As after _test_cycle_mount(), they are just testing default mount option. > > > > > > > > > > > > > To make it clear: > > > > If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and > > > > test_cycle_mount should follow TEST_FS_MOUNT_OPTS. > > > > > > _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter > > > which mount it is. > > > > While test mount setup by check script follows MOUNT_OPTIONS. > > Well, $MOUNT_OPTIONS is appended by check_generic_filsystem() or > check_btrfs_filesystem() for btrfs. Yeah, you're right, I found it too. The _check_<fs>_filesystem() mounts with MOUNT_OPTIONS unconditionally. > > Which will remount the device with $MOUNT_OPTIONS. > Further more, this function doesn't care if it's scratch device or test > device, just use MOUNT_OPTIONS. > > So either we split it for scratch and test device, and follows different > mount options, > or just like my submitted patch, to make _test_mount() to follow > MOUNT_OPTIONS. Fixing _check_<fs>_filesystem() is the correct way. And I guess we can refactor out a common function and call it in _check_[xfs|btrfs|generic]_filesystem. Thanks for looking into this! Eryu > > Thanks, > Qu > > > > > Just check the following very basic test script: > > ------ > > #! /bin/bash > > # FS QA Test 010 > > # > > # what am I here for? > > # > > 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 generic > > _supported_os Linux > > _require_test > > > > mount > $tmp.mount1 > > _test_cycle_mount > > mount > $tmp.mount2 > > > > diff $tmp.mount1 $tmp.mount2 > > > > echo "Silence is golden" > > # success, all done > > status=0 > > exit > > > > ------ > > > > 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
At 05/24/2017 05:22 PM, Eryu Guan wrote: > On Wed, May 24, 2017 at 03:58:11PM +0800, Qu Wenruo wrote: >> >> >> At 05/24/2017 01:16 PM, Qu Wenruo wrote: >>> >>> >>> At 05/24/2017 01:08 PM, Eryu Guan wrote: >>>> On Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: >>>>> >>>>> >>>>> At 05/24/2017 12:24 PM, Eryu Guan wrote: >>>>>> On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: >>>>>>> >>>>>>> >>>>>>> At 05/23/2017 07:13 PM, Eryu Guan wrote: >>>>>>>> On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: >>>>>>>>> [BUG] >>>>>>>>> If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 >>>>>>>>> generic/143 and generic/154, it will cause false alert like: >>>>>>>>> cp: failed to clone '/mnt/test/test-154/file2' >>>>>>>>> from '/mnt/test/test-154/file1': Invalid >>>>>>>>> argument >>>>>>>> >>>>>>>> MOUNT_OPTIONS is for scratch mount, and >>>>>>>> TEST_FS_MOUNT_OPTS is for test >>>>>>>> dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" >>>>>>>> should fix your problem. >>>>>>> >>>>>>> Nope, the problem is the inconsistent of TEST_MNT setup. >>>>>> >>>>>> It does fix the failure for me, did I miss anything? >>>>>> >>>>>> # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o >>>>>> nodatasum" ./check generic/142 generic/143 generic/154 >>>>>> FSTYP -- btrfs >>>>>> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 >>>>>> MKFS_OPTIONS -- /dev/sda6 >>>>>> MOUNT_OPTIONS -- -o nodatasum -o >>>>>> context=system_u:object_r:root_t:s0 /dev/sda6 >>>>>> /mnt/testarea/scratch >>>>>> >>>>>> generic/142 2s ... 1s >>>>>> generic/143 18s >>>>>> generic/154 1s >>>>>> Ran: generic/142 generic/143 generic/154 >>>>>> Passed all 3 tests >>>>>> >>>>> >>>>> But if you only export MOUNT_OPTIONS, it will fail, due to the different >>>>> mount options between test_cycle_mount(). >>>> >>>> That's correct. Sorry, I didn't make it clear in my first reply. I meant >>>> that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to >>>> "-onodatasum", for both test dev and scratch dev. >>> >>> That's just a workaround, not a root fix. >>> >>> Not to mention quite a lot test cases lose its coverage. >>> As after _test_cycle_mount(), they are just testing default mount option. >>> >>>> >>>>> >>>>> To make it clear: >>>>> If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and >>>>> test_cycle_mount should follow TEST_FS_MOUNT_OPTS. >>>> >>>> _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter >>>> which mount it is. >>> >>> While test mount setup by check script follows MOUNT_OPTIONS. >> >> Well, $MOUNT_OPTIONS is appended by check_generic_filsystem() or >> check_btrfs_filesystem() for btrfs. > > Yeah, you're right, I found it too. The _check_<fs>_filesystem() mounts > with MOUNT_OPTIONS unconditionally. > >> >> Which will remount the device with $MOUNT_OPTIONS. >> Further more, this function doesn't care if it's scratch device or test >> device, just use MOUNT_OPTIONS. >> >> So either we split it for scratch and test device, and follows different >> mount options, >> or just like my submitted patch, to make _test_mount() to follow >> MOUNT_OPTIONS. > > Fixing _check_<fs>_filesystem() is the correct way. And I guess we can > refactor out a common function and call it in > _check_[xfs|btrfs|generic]_filesystem. BTW, I'm wondering if we should still follow the old "TEST_FS_MOUNT_OPTIONS". I wonder split MOUNT_OPTIONS and TEST_FS_MOUNT_OPTIONS may reduce the test coverage if tester is not aware of these mount options. Thanks, Qu > > Thanks for looking into this! > > Eryu > >> >> Thanks, >> Qu >> >>> >>> Just check the following very basic test script: >>> ------ >>> #! /bin/bash >>> # FS QA Test 010 >>> # >>> # what am I here for? >>> # >>> 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 generic >>> _supported_os Linux >>> _require_test >>> >>> mount > $tmp.mount1 >>> _test_cycle_mount >>> mount > $tmp.mount2 >>> >>> diff $tmp.mount1 $tmp.mount2 >>> >>> echo "Silence is golden" >>> # success, all done >>> status=0 >>> exit >>> >>> ------ >>> >>> 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 Wed, May 24, 2017 at 05:27:24PM +0800, Qu Wenruo wrote: > > > At 05/24/2017 05:22 PM, Eryu Guan wrote: > > On Wed, May 24, 2017 at 03:58:11PM +0800, Qu Wenruo wrote: > > > > > > > > > At 05/24/2017 01:16 PM, Qu Wenruo wrote: > > > > > > > > > > > > At 05/24/2017 01:08 PM, Eryu Guan wrote: > > > > > On Wed, May 24, 2017 at 12:28:34PM +0800, Qu Wenruo wrote: > > > > > > > > > > > > > > > > > > At 05/24/2017 12:24 PM, Eryu Guan wrote: > > > > > > > On Wed, May 24, 2017 at 08:22:25AM +0800, Qu Wenruo wrote: > > > > > > > > > > > > > > > > > > > > > > > > At 05/23/2017 07:13 PM, Eryu Guan wrote: > > > > > > > > > On Tue, May 23, 2017 at 04:02:05PM +0800, Qu Wenruo wrote: > > > > > > > > > > [BUG] > > > > > > > > > > If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 > > > > > > > > > > generic/143 and generic/154, it will cause false alert like: > > > > > > > > > > cp: failed to clone '/mnt/test/test-154/file2' > > > > > > > > > > from '/mnt/test/test-154/file1': Invalid > > > > > > > > > > argument > > > > > > > > > > > > > > > > > > MOUNT_OPTIONS is for scratch mount, and > > > > > > > > > TEST_FS_MOUNT_OPTS is for test > > > > > > > > > dev mount, so I think setting TEST_FS_MOUNT_OPTS to "-o nodatasum" > > > > > > > > > should fix your problem. > > > > > > > > > > > > > > > > Nope, the problem is the inconsistent of TEST_MNT setup. > > > > > > > > > > > > > > It does fix the failure for me, did I miss anything? > > > > > > > > > > > > > > # MOUNT_OPTIONS="-o nodatasum" TEST_FS_MOUNT_OPTS="-o > > > > > > > nodatasum" ./check generic/142 generic/143 generic/154 > > > > > > > FSTYP -- btrfs > > > > > > > PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.12.0-rc1 > > > > > > > MKFS_OPTIONS -- /dev/sda6 > > > > > > > MOUNT_OPTIONS -- -o nodatasum -o > > > > > > > context=system_u:object_r:root_t:s0 /dev/sda6 > > > > > > > /mnt/testarea/scratch > > > > > > > > > > > > > > generic/142 2s ... 1s > > > > > > > generic/143 18s > > > > > > > generic/154 1s > > > > > > > Ran: generic/142 generic/143 generic/154 > > > > > > > Passed all 3 tests > > > > > > > > > > > > > > > > > > > But if you only export MOUNT_OPTIONS, it will fail, due to the different > > > > > > mount options between test_cycle_mount(). > > > > > > > > > > That's correct. Sorry, I didn't make it clear in my first reply. I meant > > > > > that you should set both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS to > > > > > "-onodatasum", for both test dev and scratch dev. > > > > > > > > That's just a workaround, not a root fix. > > > > > > > > Not to mention quite a lot test cases lose its coverage. > > > > As after _test_cycle_mount(), they are just testing default mount option. > > > > > > > > > > > > > > > > > > > > > To make it clear: > > > > > > If test mount follows TEST_FS_MOUNT_OPTS, then both the first mount and > > > > > > test_cycle_mount should follow TEST_FS_MOUNT_OPTS. > > > > > > > > > > _test_mount does follow TEST_FS_MOUNT_OPTS, not MOUNT_OPTIONS, no matter > > > > > which mount it is. > > > > > > > > While test mount setup by check script follows MOUNT_OPTIONS. > > > > > > Well, $MOUNT_OPTIONS is appended by check_generic_filsystem() or > > > check_btrfs_filesystem() for btrfs. > > > > Yeah, you're right, I found it too. The _check_<fs>_filesystem() mounts > > with MOUNT_OPTIONS unconditionally. > > > > > > > > Which will remount the device with $MOUNT_OPTIONS. > > > Further more, this function doesn't care if it's scratch device or test > > > device, just use MOUNT_OPTIONS. > > > > > > So either we split it for scratch and test device, and follows different > > > mount options, > > > or just like my submitted patch, to make _test_mount() to follow > > > MOUNT_OPTIONS. > > > > Fixing _check_<fs>_filesystem() is the correct way. And I guess we can > > refactor out a common function and call it in > > _check_[xfs|btrfs|generic]_filesystem. > > BTW, I'm wondering if we should still follow the old > "TEST_FS_MOUNT_OPTIONS". > > I wonder split MOUNT_OPTIONS and TEST_FS_MOUNT_OPTIONS may reduce the test > coverage if tester is not aware of these mount options. There're tests require different mount options between test dev and scratch, though it's a really rare case, one example is generic/413. And I think separated mount options provide us flexibility in testing too. IMO, at this stage the correct thing to do is to make all these global variables well documented. I'll see if I have some free cycles to do so (but I'm not a native English speaker, I'm afraid the document will be really hard to understand :) Thanks, Eryu > > Thanks, > Qu > > > > > Thanks for looking into this! > > > > Eryu > > > > > > > > Thanks, > > > Qu > > > > > > > > > > > Just check the following very basic test script: > > > > ------ > > > > #! /bin/bash > > > > # FS QA Test 010 > > > > # > > > > # what am I here for? > > > > # > > > > 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 generic > > > > _supported_os Linux > > > > _require_test > > > > > > > > mount > $tmp.mount1 > > > > _test_cycle_mount > > > > mount > $tmp.mount2 > > > > > > > > diff $tmp.mount1 $tmp.mount2 > > > > > > > > echo "Silence is golden" > > > > # success, all done > > > > status=0 > > > > exit > > > > > > > > ------ > > > > > > > > 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 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
diff --git a/common/rc b/common/rc index ba215961..a591907c 100644 --- a/common/rc +++ b/common/rc @@ -522,7 +522,8 @@ _test_mount() return $? fi _test_options mount - _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR + _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS \ + $MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR } _test_unmount()
[BUG] If using MOUNT_OPTIONS="-o nodatasum" and btrfs to run genierc/142 generic/143 and generic/154, it will cause false alert like: cp: failed to clone '/mnt/test/test-154/file2' from '/mnt/test/test-154/file1': Invalid argument [REASON] It is caused by _test_cycle_mount function, which unmount test device, but when trying to re-mount it again using _test_mount(), we don't pass $MOUNT_OPTIONS. So this makes mount options differs between _test_cycle_mount(). And btrfs doesn't allow different csum flags between reflink source and destination inodes, so it returns -EINVAL for reflink operation. [FIX] Fix it by passing $MOUNT_OPTIONS to _test_mount(), so that _test_cycle_mount() won't cause different mount options. So btrfs with "-o nodatasum" mount option can pass generic/14[23] and generic/154 without false alert. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- common/rc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)