Re: [PATCH] fstests: common: Make _test_mount to include MOUNT_OPTIONS to allow consistent _test_cycle_mount

[Eryu Guan <eguan@redhat.com>]

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