The new behavior of DAX on xfs/ext4 has been merged into main kernel tree/ext4-dax branch so it is time for fstests to support new behavior of DAX. 1) Refactor common functions and take use of them. 2) Move and update xfs/260. 3) Add two new tests to verify some features. References: https://lkml.org/lkml/2019/10/20/96 https://lkml.org/lkml/2020/5/28/949 Xiao Yang (7): common/rc: Introduce new helpers for DAX mount options and FS_XFLAG_DAX fstests: Use _require_scratch_dax_mountopt() and _require_dax_iflag() generic/223: Don't clear all mkfs options for _scratch_mkfs_geom() roughly generic/413, xfs/260: Improve format operation for PMD fault testing xfs/260: Move and update xfs/260 generic: Verify if statx() can qurey S_DAX flag on regular file correctly generic: Verify the inheritance behavior of FS_XFLAG_DAX flag in various combinations common/rc | 47 ++++++-- tests/ext4/030 | 2 +- tests/ext4/031 | 4 +- tests/generic/223 | 1 - tests/generic/413 | 12 +- tests/generic/462 | 2 +- tests/{xfs/260 => generic/603} | 74 ++++++------ tests/generic/603.out | 2 + tests/generic/604 | 100 +++++++++++++++++ tests/generic/604.out | 2 + tests/generic/605 | 199 +++++++++++++++++++++++++++++++++ tests/generic/605.out | 2 + tests/generic/group | 3 + tests/xfs/260.out | 2 - tests/xfs/group | 1 - 15 files changed, 390 insertions(+), 63 deletions(-) rename tests/{xfs/260 => generic/603} (53%) create mode 100644 tests/generic/603.out create mode 100644 tests/generic/604 create mode 100644 tests/generic/604.out create mode 100644 tests/generic/605 create mode 100644 tests/generic/605.out delete mode 100644 tests/xfs/260.out -- 2.21.0
1) _require_scratch_dax_mountopt() checks both old and new DAX mount option 2) _require_dax_iflag() checks FS_XFLAG_DAX Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- V5->V6: Simplify _require_scratch_dax_mountopt because it is enough to only check dax/dax=always mount option. See the following reasons: 1) we cannot detect if underlying device supports dax by mounting dax=inode or dax=never. 2) dax=always, dax=inode, dax=never are always introduced together. common/rc | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/common/rc b/common/rc index f17b19f2..aeec1f11 100644 --- a/common/rc +++ b/common/rc @@ -3188,6 +3188,28 @@ _require_scratch_dax() _scratch_unmount } +# Only accept dax/dax=always mount option becasue dax=always, dax=inode +# and dax=never are always introduced together. +_require_scratch_dax_mountopt() +{ + local mountopt=$1 + + _require_scratch + _scratch_mkfs > /dev/null 2>&1 + _try_scratch_mount "-o $mountopt" > /dev/null 2>&1 || \ + _notrun "mount $SCRATCH_DEV with $mountopt failed" + + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" + + _scratch_unmount +} + +_require_dax_iflag() +{ + _require_xfs_io_command "chattr" "x" +} + # Does norecovery support by this fs? _require_norecovery() { -- 2.21.0
1) Make related tests use _require_scratch_dax_mountopt() and _require_dax_iflag() 2) Remove unused _require_scratch_dax() Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> --- V5->V6: Merge the third patch into the second patch. common/rc | 13 ------------- tests/ext4/030 | 2 +- tests/ext4/031 | 4 ++-- tests/generic/413 | 2 +- tests/generic/462 | 2 +- tests/xfs/260 | 4 ++-- 6 files changed, 7 insertions(+), 20 deletions(-) diff --git a/common/rc b/common/rc index aeec1f11..6c908f2e 100644 --- a/common/rc +++ b/common/rc @@ -3175,19 +3175,6 @@ _require_scratch_shutdown() } # Does dax mount option work on this dev/fs? -_require_scratch_dax() -{ - _require_scratch - _scratch_mkfs > /dev/null 2>&1 - _try_scratch_mount -o dax || \ - _notrun "mount $SCRATCH_DEV with dax failed" - # Check options to be sure. XFS ignores dax option - # and goes on if dev underneath does not support dax. - _fs_options $SCRATCH_DEV | grep -qw "dax" || \ - _notrun "$SCRATCH_DEV $FSTYP does not support -o dax" - _scratch_unmount -} - # Only accept dax/dax=always mount option becasue dax=always, dax=inode # and dax=never are always introduced together. _require_scratch_dax_mountopt() diff --git a/tests/ext4/030 b/tests/ext4/030 index 93bbca86..fb5cf451 100755 --- a/tests/ext4/030 +++ b/tests/ext4/030 @@ -33,7 +33,7 @@ rm -f $seqres.full # Modify as appropriate. _supported_os Linux _supported_fs ext4 -_require_scratch_dax +_require_scratch_dax_mountopt "dax" _require_test_program "t_ext4_dax_journal_corruption" _require_command "$CHATTR_PROG" chattr diff --git a/tests/ext4/031 b/tests/ext4/031 index dc58214e..20e2fab7 100755 --- a/tests/ext4/031 +++ b/tests/ext4/031 @@ -37,7 +37,7 @@ MOUNT_OPTIONS="" # Modify as appropriate. _supported_os Linux _supported_fs ext4 -_require_scratch_dax +_require_scratch_dax_mountopt "dax" _require_test_program "t_ext4_dax_inline_corruption" _require_scratch_ext4_feature "inline_data" @@ -56,7 +56,7 @@ _scratch_unmount >> $seqres.full 2>&1 _try_scratch_mount "-o dax" >> $seqres.full 2>&1 if [[ $? != 0 ]]; then - # _require_scratch_dax already verified that we could mount with DAX. + # _require_scratch_dax_mountopt already verified that we could mount with DAX. # Failure here is expected because we have inline data. echo "Silence is golden" status=0 diff --git a/tests/generic/413 b/tests/generic/413 index 1ce89aff..19e1b926 100755 --- a/tests/generic/413 +++ b/tests/generic/413 @@ -31,7 +31,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_test -_require_scratch_dax +_require_scratch_dax_mountopt "dax" _require_test_program "feature" _require_test_program "t_mmap_dio" _require_xfs_io_command "falloc" diff --git a/tests/generic/462 b/tests/generic/462 index 1ab6cadc..4a940239 100755 --- a/tests/generic/462 +++ b/tests/generic/462 @@ -37,7 +37,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_test -_require_scratch_dax +_require_scratch_dax_mountopt "dax" _require_test_program "t_mmap_write_ro" # running by unpriviliged user is not necessary to reproduce # this bug, just trying to test more. diff --git a/tests/xfs/260 b/tests/xfs/260 index 3464ffef..bcdc6041 100755 --- a/tests/xfs/260 +++ b/tests/xfs/260 @@ -30,10 +30,10 @@ rm -f $seqres.full _supported_fs xfs _supported_os Linux -_require_scratch_dax +_require_scratch_dax_mountopt "dax" _require_test_program "feature" _require_test_program "t_mmap_dio" -_require_xfs_io_command "chattr" "x" +_require_dax_iflag _require_xfs_io_command "falloc" prep_files() -- 2.21.0
ext4 can accept the last one if the same mkfs options are passed but xfs cannot accept the same mkfs options and reports "xxx option is respecified" error. I prefer to override the same mkfs option which is defined in MKFS_OPTION so that we can have a chance to pass other mkfs options to _scratch_mkfs_geom(). Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- common/rc | 14 +++++++++++++- tests/generic/223 | 1 - 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/common/rc b/common/rc index 6c908f2e..567cf83b 100644 --- a/common/rc +++ b/common/rc @@ -1051,7 +1051,19 @@ _scratch_mkfs_geom() case $FSTYP in xfs) - MKFS_OPTIONS+=" -b size=$blocksize, -d su=$sunit_bytes,sw=$swidth_mult" + if echo "$MKFS_OPTIONS" | egrep -q "b?size="; then + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r "s/(b?size=)[0-9]+/\1$blocksize/") + else + MKFS_OPTIONS+=" -b size=$blocksize" + fi + + if echo "$MKFS_OPTIONS" | egrep -q "(su|sunit|sw|swidth)="; then + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r \ + -e "s/(su|sunit)=[0-9kmg]+/su=$sunit_bytes/" \ + -e "s/(sw|swidth)=[0-9kmg]+/sw=$swidth_mult/") + else + MKFS_OPTIONS+=" -d su=$sunit_bytes,sw=$swidth_mult" + fi ;; ext4|ext4dev) MKFS_OPTIONS+=" -b $blocksize -E stride=$sunit_blocks,stripe_width=$swidth_blocks" diff --git a/tests/generic/223 b/tests/generic/223 index 6cfd00dd..ba7c9a44 100755 --- a/tests/generic/223 +++ b/tests/generic/223 @@ -41,7 +41,6 @@ for SUNIT_K in 8 16 32 64 128; do let SUNIT_BLOCKS=$SUNIT_BYTES/$BLOCKSIZE echo "=== mkfs with su $SUNIT_BLOCKS blocks x 4 ===" - export MKFS_OPTIONS="" _scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1 _scratch_mount -- 2.21.0
1) Simple code and fix the wrong value of stripe_width by _scratch_mkfs_geom(). 2) Get hugepage size by _get_hugepagesize() and replace fixed 2M with hugepage size because hugepage size/PMD_SIZE is not 2M on some arches(e.g. hugepage size/PMD_SIZE is 512M on arm64). 3) For debugging, redirect the output of mkfs to $seqres.full. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- common/rc | 10 ++++++++++ tests/generic/413 | 10 ++-------- tests/xfs/260 | 4 ++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/common/rc b/common/rc index 567cf83b..b6a48241 100644 --- a/common/rc +++ b/common/rc @@ -170,6 +170,16 @@ _get_filesize() stat -c %s "$1" } +# Get hugepagesize in bytes +_get_hugepagesize() +{ + local hugepgsz=$(awk '/Hugepagesize/ {print $2}' /proc/meminfo) + # Call _notrun if $hugepgsz is not a number + echo "$hugepgsz" | egrep -q ^[0-9]+$ || \ + _notrun "Cannot get the value of Hugepagesize" + echo $((hugepgsz * 1024)) +} + _mount() { $MOUNT_PROG `_mount_ops_filter $*` diff --git a/tests/generic/413 b/tests/generic/413 index 19e1b926..dfe2912b 100755 --- a/tests/generic/413 +++ b/tests/generic/413 @@ -111,14 +111,8 @@ do_tests() t_mmap_dio_dax $((64 * 1024 * 1024)) } -# make fs 2Mb aligned for PMD fault testing -mkfs_opts="" -if [ "$FSTYP" == "ext4" ]; then - mkfs_opts="-E stride=512,stripe_width=1" -elif [ "$FSTYP" == "xfs" ]; then - mkfs_opts="-d su=2m,sw=1" -fi -_scratch_mkfs "$mkfs_opts" > /dev/null 2>&1 +# make fs aligned for PMD fault testing +_scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 # mount SCRATCH_DEV with dax option, TEST_DEV not export MOUNT_OPTIONS="" diff --git a/tests/xfs/260 b/tests/xfs/260 index bcdc6041..81b42f01 100755 --- a/tests/xfs/260 +++ b/tests/xfs/260 @@ -121,8 +121,8 @@ do_tests() t_dax_flag_mmap_dio $((64 * 1024 * 1024)) } -# make xfs 2Mb aligned for PMD fault testing -_scratch_mkfs "-d su=2m,sw=1" > /dev/null 2>&1 +# make xfs aligned for PMD fault testing +_scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 # mount with dax option _scratch_mount "-o dax" -- 2.21.0
1) Both ext4 and xfs have supported FS_XFLAG_DAX so move it to generic. 2) Modifying FS_XFLAG_DAX on flies does not take effect immediately so make files inherit the DAX state of parent directory. 3) Setting/clearing FS_XFLAG_DAX have no chance to change S_DAX flag if mount with dax option so remove the related subtest. 4) Setting/clearing FS_XFLAG_DAX doesn't change S_DAX flag on older xfs due to commit 742d84290739 ("xfs: disable per-inode DAX flag") so only do test when fs supports new dax=inode option. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> --- tests/{xfs/260 => generic/603} | 68 +++++++++++++++++----------------- tests/generic/603.out | 2 + tests/generic/group | 1 + tests/xfs/260.out | 2 - tests/xfs/group | 1 - 5 files changed, 36 insertions(+), 38 deletions(-) rename tests/{xfs/260 => generic/603} (57%) create mode 100644 tests/generic/603.out delete mode 100644 tests/xfs/260.out diff --git a/tests/xfs/260 b/tests/generic/603 similarity index 57% rename from tests/xfs/260 rename to tests/generic/603 index 81b42f01..5dabe447 100755 --- a/tests/xfs/260 +++ b/tests/generic/603 @@ -2,7 +2,7 @@ # SPDX-License-Identifier: GPL-2.0 # Copyright (c) 2017 Red Hat Inc. All Rights Reserved. # -# FS QA Test 260 +# FS QA Test 603 # # Test per-inode DAX flag by mmap direct/buffered IO. # @@ -28,76 +28,80 @@ _cleanup() # remove previous $seqres.full before test rm -f $seqres.full -_supported_fs xfs +_supported_fs generic _supported_os Linux -_require_scratch_dax_mountopt "dax" +_require_scratch_dax_mountopt "dax=always" _require_test_program "feature" _require_test_program "t_mmap_dio" _require_dax_iflag _require_xfs_io_command "falloc" -prep_files() +SRC_DIR=$SCRATCH_MNT/src +SRC_FILE=$SRC_DIR/tf_s +DST_DIR=$SCRATCH_MNT/dst +DST_FILE=$DST_DIR/tf_d + +prep_directories() { - rm -f $SCRATCH_MNT/tf_{s,d} + mkdir -p $SRC_DIR $DST_DIR +} +prep_files() +{ + rm -f $SRC_FILE $DST_FILE $XFS_IO_PROG -f -c "falloc 0 $tsize" \ - $SCRATCH_MNT/tf_{s,d} >> $seqres.full 2>&1 + $SRC_FILE $DST_FILE >> $seqres.full 2>&1 } t_both_dax() { + $XFS_IO_PROG -c "chattr +x" $SRC_DIR $DST_DIR prep_files - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_{s,d} # with O_DIRECT first - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} $1 "dio both dax" + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ + $1 "dio both dax" prep_files - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_{s,d} # again with buffered IO - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ $1 "buffered both dax" } t_nondax_to_dax() { + $XFS_IO_PROG -c "chattr -x" $SRC_DIR + $XFS_IO_PROG -c "chattr +x" $DST_DIR prep_files - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_s - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_d - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ $1 "dio nondax to dax" prep_files - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_s - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_d - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ $1 "buffered nondax to dax" } t_dax_to_nondax() { + $XFS_IO_PROG -c "chattr +x" $SRC_DIR + $XFS_IO_PROG -c "chattr -x" $DST_DIR prep_files - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_s - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_d - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ $1 "dio dax to nondax" prep_files - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_s - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_d - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ $1 "buffered dax to nondax" } t_both_nondax() { + $XFS_IO_PROG -c "chattr -x" $SRC_DIR $DST_DIR prep_files - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_{s,d} - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ $1 "dio both nondax" prep_files - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_{s,d} - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ $1 "buffered both nondax" } @@ -112,6 +116,7 @@ t_dax_flag_mmap_dio() do_tests() { + prep_directories # less than page size t_dax_flag_mmap_dio 1024 # page size @@ -124,17 +129,10 @@ do_tests() # make xfs aligned for PMD fault testing _scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 -# mount with dax option -_scratch_mount "-o dax" - tsize=$((128 * 1024 * 1024)) -do_tests -_scratch_unmount - -# mount again without dax option -export MOUNT_OPTIONS="" -_scratch_mount +# mount with dax=inode option +_scratch_mount "-o dax=inode" do_tests # success, all done diff --git a/tests/generic/603.out b/tests/generic/603.out new file mode 100644 index 00000000..6810da89 --- /dev/null +++ b/tests/generic/603.out @@ -0,0 +1,2 @@ +QA output created by 603 +Silence is golden diff --git a/tests/generic/group b/tests/generic/group index d9ab9a31..1d0d5606 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -605,3 +605,4 @@ 600 auto quick quota 601 auto quick quota 602 auto quick encrypt +603 auto attr quick dax diff --git a/tests/xfs/260.out b/tests/xfs/260.out deleted file mode 100644 index 18ca517c..00000000 --- a/tests/xfs/260.out +++ /dev/null @@ -1,2 +0,0 @@ -QA output created by 260 -Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index daf54add..71c30898 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -257,7 +257,6 @@ 257 auto quick clone 258 auto quick clone 259 auto quick -260 auto attr quick dax 261 auto quick quota 262 dangerous_fuzzers dangerous_scrub dangerous_online_repair 263 auto quick quota -- 2.21.0
With new kernel(e.g. v5.8-rc1), statx() can be used to qurey S_DAX flag on regular file, so add a test to verify the feature. Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=712b2698e4c024b561694cbcc1abba13eb0fd9ce Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --- tests/generic/604 | 100 ++++++++++++++++++++++++++++++++++++++++++ tests/generic/604.out | 2 + tests/generic/group | 1 + 3 files changed, 103 insertions(+) create mode 100644 tests/generic/604 create mode 100644 tests/generic/604.out diff --git a/tests/generic/604 b/tests/generic/604 new file mode 100644 index 00000000..59963dbc --- /dev/null +++ b/tests/generic/604 @@ -0,0 +1,100 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2020 Fujitsu. All Rights Reserved. +# +# FS QA Test 604 +# +# By the following cases, verify if statx() can query S_DAX flag +# on regular file correctly. +# 1) With dax=always option, FS_XFLAG_DAX is ignored and S_DAX flag +# always exists on regular file. +# 2) With dax=inode option, setting/clearing FS_XFLAG_DAX can change +# S_DAX flag on regular file. +# 3) With dax=never option, FS_XFLAG_DAX is ignored and S_DAX flag +# never exists on regular file. +# +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 + +_supported_fs generic +_supported_os Linux +_require_scratch_dax_mountopt "dax=always" +_require_dax_iflag +_require_xfs_io_command "statx" "-r" + +PARENT_DIR=$SCRATCH_MNT/testdir +TEST_FILE=$PARENT_DIR/testfile + +check_s_dax() +{ + local exp_s_dax=$1 + + local attributes=$($XFS_IO_PROG -c 'statx -r' $TEST_FILE | awk '/stat.attributes / { print $3 }') + if [ $exp_s_dax -eq 0 ]; then + (( attributes & 0x2000 )) && echo "$TEST_FILE has unexpected S_DAX flag" + else + (( attributes & 0x2000 )) || echo "$TEST_FILE doen't have expected S_DAX flag" + fi +} + +test_s_dax() +{ + local dax_option=$1 + local exp_s_dax1=$2 + local exp_s_dax2=$3 + + # Mount with specified dax option + _scratch_mount "-o $dax_option" + + # Prepare directory + mkdir -p $PARENT_DIR + + rm -f $TEST_FILE + $XFS_IO_PROG -c "chattr +x" $PARENT_DIR + touch $TEST_FILE + # Check if setting FS_XFLAG_DAX changes S_DAX flag + check_s_dax $exp_s_dax1 + + rm -f $TEST_FILE + $XFS_IO_PROG -c "chattr -x" $PARENT_DIR + touch $TEST_FILE + # Check if clearing FS_XFLAG_DAX changes S_DAX flag + check_s_dax $exp_s_dax2 + + _scratch_unmount +} + +do_tests() +{ + test_s_dax "dax=always" 1 1 + test_s_dax "dax=inode" 1 0 + test_s_dax "dax=never" 0 0 +} + +_scratch_mkfs >> $seqres.full 2>&1 + +do_tests + +# success, all done +echo "Silence is golden" +status=0 +exit diff --git a/tests/generic/604.out b/tests/generic/604.out new file mode 100644 index 00000000..c06a1c7f --- /dev/null +++ b/tests/generic/604.out @@ -0,0 +1,2 @@ +QA output created by 604 +Silence is golden diff --git a/tests/generic/group b/tests/generic/group index 1d0d5606..676e0d2e 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -606,3 +606,4 @@ 601 auto quick quota 602 auto quick encrypt 603 auto attr quick dax +604 auto attr quick dax -- 2.21.0
Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- tests/generic/605 | 199 ++++++++++++++++++++++++++++++++++++++++++ tests/generic/605.out | 2 + tests/generic/group | 1 + 3 files changed, 202 insertions(+) create mode 100644 tests/generic/605 create mode 100644 tests/generic/605.out diff --git a/tests/generic/605 b/tests/generic/605 new file mode 100644 index 00000000..6924223a --- /dev/null +++ b/tests/generic/605 @@ -0,0 +1,199 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2020 Fujitsu. All Rights Reserved. +# +# FS QA Test 605 +# +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various combinations. +# 1) New files and directories automatically inherit FS_XFLAG_DAX from their parent directory. +# 2) cp operation make files and directories inherit the FS_XFLAG_DAX from new parent directory. +# 3) mv operation make files and directories preserve the FS_XFLAG_DAX from old parent directory. +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted by dax mount options. + +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 + +_supported_fs generic +_supported_os Linux +_require_scratch +_require_dax_iflag +_require_xfs_io_command "lsattr" "-v" + +check_xflag() +{ + local target=$1 + local exp_xflag=$2 + + if [ $exp_xflag -eq 0 ]; then + _test_inode_flag dax $target && echo "$target has unexpected FS_XFLAG_DAX flag" + else + _test_inode_flag dax $target || echo "$target doen't have expected FS_XFLAG_DAX flag" + fi +} + +test_xflag_inheritance1() +{ + mkdir -p a + $XFS_IO_PROG -c "chattr +x" a + mkdir -p a/b/c + touch a/b/c/d + + check_xflag a 1 + check_xflag a/b 1 + check_xflag a/b/c 1 + check_xflag a/b/c/d 1 + + rm -rf a +} + +test_xflag_inheritance2() +{ + mkdir -p a/b + $XFS_IO_PROG -c "chattr +x" a + mkdir -p a/b/c a/d + touch a/b/c/e a/d/f + + check_xflag a 1 + check_xflag a/b 0 + check_xflag a/b/c 0 + check_xflag a/b/c/e 0 + check_xflag a/d 1 + check_xflag a/d/f 1 + + rm -rf a +} + +test_xflag_inheritance3() +{ + mkdir -p a/b + $XFS_IO_PROG -c "chattr +x" a/b + mkdir -p a/b/c a/d + touch a/b/c/e a/d/f + + check_xflag a 0 + check_xflag a/b 1 + check_xflag a/b/c 1 + check_xflag a/b/c/e 1 + check_xflag a/d 0 + check_xflag a/d/f 0 + + rm -rf a +} + +test_xflag_inheritance4() +{ + mkdir -p a + $XFS_IO_PROG -c "chattr +x" a + mkdir -p a/b/c + $XFS_IO_PROG -c "chattr -x" a/b + mkdir -p a/b/c/d a/b/e + touch a/b/c/d/f a/b/e/g + + check_xflag a 1 + check_xflag a/b 0 + check_xflag a/b/c 1 + check_xflag a/b/c/d 1 + check_xflag a/b/c/d/f 1 + check_xflag a/b/e 0 + check_xflag a/b/e/g 0 + + rm -rf a +} + +test_xflag_inheritance5() +{ + mkdir -p a b + $XFS_IO_PROG -c "chattr +x" a + mkdir -p a/c a/d b/e b/f + touch a/g b/h + + cp -r a/c b/ + cp -r b/e a/ + cp -r a/g b/ + mv a/d b/ + mv b/f a/ + mv b/h a/ + + check_xflag b/c 0 + check_xflag b/d 1 + check_xflag a/e 1 + check_xflag a/f 0 + check_xflag b/g 0 + check_xflag a/h 0 + + rm -rf a b +} + +do_xflag_tests() +{ + local option=$1 + + _scratch_mount "$option" + cd $SCRATCH_MNT + + for i in $(seq 1 5); do + test_xflag_inheritance${i} + done + + cd - > /dev/null + _scratch_unmount +} + +check_dax_mountopt() +{ + local option=$1 + local ret=0 + + _try_scratch_mount "-o $option" >> $seqres.full 2>&1 || return 1 + + # Match option name exactly + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 + + _scratch_unmount + + return $ret +} + +do_tests() +{ + # Mount without dax option + do_xflag_tests + + # Mount with old dax option if fs only supports it. + check_dax_mountopt "dax" && do_xflag_tests "-o dax" + + # Mount with new dax options if fs supports them. + if check_dax_mountopt "dax=always"; then + for dax_option in "dax=always" "dax=inode" "dax=never"; do + do_xflag_tests "-o $dax_option" + done + fi +} + +_scratch_mkfs >> $seqres.full 2>&1 + +do_tests + +# success, all done +echo "Silence is golden" +status=0 +exit diff --git a/tests/generic/605.out b/tests/generic/605.out new file mode 100644 index 00000000..1ae20049 --- /dev/null +++ b/tests/generic/605.out @@ -0,0 +1,2 @@ +QA output created by 605 +Silence is golden diff --git a/tests/generic/group b/tests/generic/group index 676e0d2e..a8451862 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -607,3 +607,4 @@ 602 auto quick encrypt 603 auto attr quick dax 604 auto attr quick dax +605 auto attr quick dax -- 2.21.0
On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote: > 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option > 2) _require_dax_iflag() checks FS_XFLAG_DAX > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > > V5->V6: > Simplify _require_scratch_dax_mountopt because it is enough to only check > dax/dax=always mount option. See the following reasons: > 1) we cannot detect if underlying device supports dax by mounting dax=inode > or dax=never. > 2) dax=always, dax=inode, dax=never are always introduced together. > > common/rc | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/common/rc b/common/rc > index f17b19f2..aeec1f11 100644 > --- a/common/rc > +++ b/common/rc > @@ -3188,6 +3188,28 @@ _require_scratch_dax() > _scratch_unmount > } > > +# Only accept dax/dax=always mount option becasue dax=always, dax=inode > +# and dax=never are always introduced together. > +_require_scratch_dax_mountopt() > +{ > + local mountopt=$1 > + > + _require_scratch > + _scratch_mkfs > /dev/null 2>&1 > + _try_scratch_mount "-o $mountopt" > /dev/null 2>&1 || \ > + _notrun "mount $SCRATCH_DEV with $mountopt failed" > + > + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ > + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" > + > + _scratch_unmount > +} > + > +_require_dax_iflag() > +{ > + _require_xfs_io_command "chattr" "x" > +} > + > # Does norecovery support by this fs? > _require_norecovery() > { > -- > 2.21.0 > > >
On Tue, Jul 14, 2020 at 05:40:05PM +0800, Xiao Yang wrote: > ext4 can accept the last one if the same mkfs options are passed but xfs cannot I'm having trouble parsing this commit message. What does 'last one' refer to? > accept the same mkfs options and reports "xxx option is respecified" error. Ok I think I understand now. Some FS's (XFS) do not accept an option more than once. So we can't just blindly add options to the end of the MKFS_OPTIONS. Is that correct? > I > prefer to override the same mkfs option which is defined in MKFS_OPTION so that > we can have a chance to pass other mkfs options to _scratch_mkfs_geom(). Instead the patch parses the current option string and replaces the value if the option is already there. This allows us to specify MKFS_OPTIONS to generic/223. I think the code is reasonable although my sed skills are not good enough to tell for sure... ;-) Ira > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > --- > common/rc | 14 +++++++++++++- > tests/generic/223 | 1 - > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/common/rc b/common/rc > index 6c908f2e..567cf83b 100644 > --- a/common/rc > +++ b/common/rc > @@ -1051,7 +1051,19 @@ _scratch_mkfs_geom() > > case $FSTYP in > xfs) > - MKFS_OPTIONS+=" -b size=$blocksize, -d su=$sunit_bytes,sw=$swidth_mult" > + if echo "$MKFS_OPTIONS" | egrep -q "b?size="; then > + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r "s/(b?size=)[0-9]+/\1$blocksize/") > + else > + MKFS_OPTIONS+=" -b size=$blocksize" > + fi > + > + if echo "$MKFS_OPTIONS" | egrep -q "(su|sunit|sw|swidth)="; then > + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r \ > + -e "s/(su|sunit)=[0-9kmg]+/su=$sunit_bytes/" \ > + -e "s/(sw|swidth)=[0-9kmg]+/sw=$swidth_mult/") > + else > + MKFS_OPTIONS+=" -d su=$sunit_bytes,sw=$swidth_mult" > + fi > ;; > ext4|ext4dev) > MKFS_OPTIONS+=" -b $blocksize -E stride=$sunit_blocks,stripe_width=$swidth_blocks" > diff --git a/tests/generic/223 b/tests/generic/223 > index 6cfd00dd..ba7c9a44 100755 > --- a/tests/generic/223 > +++ b/tests/generic/223 > @@ -41,7 +41,6 @@ for SUNIT_K in 8 16 32 64 128; do > let SUNIT_BLOCKS=$SUNIT_BYTES/$BLOCKSIZE > > echo "=== mkfs with su $SUNIT_BLOCKS blocks x 4 ===" > - export MKFS_OPTIONS="" > _scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE >> $seqres.full 2>&1 > _scratch_mount > > -- > 2.21.0 > > >
On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > --- > tests/generic/605 | 199 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/605.out | 2 + > tests/generic/group | 1 + > 3 files changed, 202 insertions(+) > create mode 100644 tests/generic/605 > create mode 100644 tests/generic/605.out > > diff --git a/tests/generic/605 b/tests/generic/605 > new file mode 100644 > index 00000000..6924223a > --- /dev/null > +++ b/tests/generic/605 > @@ -0,0 +1,199 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2020 Fujitsu. All Rights Reserved. > +# > +# FS QA Test 605 > +# > +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various combinations. > +# 1) New files and directories automatically inherit FS_XFLAG_DAX from their parent directory. > +# 2) cp operation make files and directories inherit the FS_XFLAG_DAX from new parent directory. > +# 3) mv operation make files and directories preserve the FS_XFLAG_DAX from old parent directory. > +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted by dax mount options. > + > +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 > + > +_supported_fs generic > +_supported_os Linux > +_require_scratch > +_require_dax_iflag > +_require_xfs_io_command "lsattr" "-v" > + > +check_xflag() > +{ > + local target=$1 > + local exp_xflag=$2 > + > + if [ $exp_xflag -eq 0 ]; then > + _test_inode_flag dax $target && echo "$target has unexpected FS_XFLAG_DAX flag" > + else > + _test_inode_flag dax $target || echo "$target doen't have expected FS_XFLAG_DAX flag" > + fi > +} > + > +test_xflag_inheritance1() > +{ > + mkdir -p a > + $XFS_IO_PROG -c "chattr +x" a > + mkdir -p a/b/c > + touch a/b/c/d > + > + check_xflag a 1 > + check_xflag a/b 1 > + check_xflag a/b/c 1 > + check_xflag a/b/c/d 1 > + > + rm -rf a > +} > + > +test_xflag_inheritance2() > +{ > + mkdir -p a/b > + $XFS_IO_PROG -c "chattr +x" a > + mkdir -p a/b/c a/d > + touch a/b/c/e a/d/f > + > + check_xflag a 1 > + check_xflag a/b 0 > + check_xflag a/b/c 0 > + check_xflag a/b/c/e 0 > + check_xflag a/d 1 > + check_xflag a/d/f 1 > + > + rm -rf a > +} > + > +test_xflag_inheritance3() > +{ > + mkdir -p a/b > + $XFS_IO_PROG -c "chattr +x" a/b > + mkdir -p a/b/c a/d > + touch a/b/c/e a/d/f > + > + check_xflag a 0 > + check_xflag a/b 1 > + check_xflag a/b/c 1 > + check_xflag a/b/c/e 1 > + check_xflag a/d 0 > + check_xflag a/d/f 0 > + > + rm -rf a > +} It really seems like 2 and 3 test the same thing? > + > +test_xflag_inheritance4() > +{ > + mkdir -p a > + $XFS_IO_PROG -c "chattr +x" a > + mkdir -p a/b/c > + $XFS_IO_PROG -c "chattr -x" a/b > + mkdir -p a/b/c/d a/b/e > + touch a/b/c/d/f a/b/e/g > + > + check_xflag a 1 > + check_xflag a/b 0 > + check_xflag a/b/c 1 > + check_xflag a/b/c/d 1 > + check_xflag a/b/c/d/f 1 > + check_xflag a/b/e 0 > + check_xflag a/b/e/g 0 > + > + rm -rf a > +} > + > +test_xflag_inheritance5() > +{ > + mkdir -p a b > + $XFS_IO_PROG -c "chattr +x" a > + mkdir -p a/c a/d b/e b/f > + touch a/g b/h > + > + cp -r a/c b/ > + cp -r b/e a/ > + cp -r a/g b/ > + mv a/d b/ > + mv b/f a/ > + mv b/h a/ > + > + check_xflag b/c 0 > + check_xflag b/d 1 > + check_xflag a/e 1 > + check_xflag a/f 0 > + check_xflag b/g 0 > + check_xflag a/h 0 > + > + rm -rf a b > +} > + > +do_xflag_tests() > +{ > + local option=$1 > + > + _scratch_mount "$option" > + cd $SCRATCH_MNT > + > + for i in $(seq 1 5); do > + test_xflag_inheritance${i} > + done > + > + cd - > /dev/null > + _scratch_unmount > +} > + > +check_dax_mountopt() > +{ > + local option=$1 > + local ret=0 > + > + _try_scratch_mount "-o $option" >> $seqres.full 2>&1 || return 1 > + > + # Match option name exactly > + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 > + > + _scratch_unmount > + > + return $ret > +} Should this be a common function? > + > +do_tests() > +{ > + # Mount without dax option > + do_xflag_tests > + > + # Mount with old dax option if fs only supports it. > + check_dax_mountopt "dax" && do_xflag_tests "-o dax" I don't understand the order here. If we are on an older kernel and the FS only supports '-o dax' the do_xflag_tests will fail won't it? So shouldn't we do this first and bail/'not run' this test if that is the case? I really don't think there is any point in testing the old XFS behavior because the FS_XFLAG_DAX had no effect. So even if it is broken it does not matter. Or perhaps I am missing something here? Ira > + > + # Mount with new dax options if fs supports them. > + if check_dax_mountopt "dax=always"; then > + for dax_option in "dax=always" "dax=inode" "dax=never"; do > + do_xflag_tests "-o $dax_option" > + done > + fi > +} > + > +_scratch_mkfs >> $seqres.full 2>&1 > + > +do_tests > + > +# success, all done > +echo "Silence is golden" > +status=0 > +exit > diff --git a/tests/generic/605.out b/tests/generic/605.out > new file mode 100644 > index 00000000..1ae20049 > --- /dev/null > +++ b/tests/generic/605.out > @@ -0,0 +1,2 @@ > +QA output created by 605 > +Silence is golden > diff --git a/tests/generic/group b/tests/generic/group > index 676e0d2e..a8451862 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -607,3 +607,4 @@ > 602 auto quick encrypt > 603 auto attr quick dax > 604 auto attr quick dax > +605 auto attr quick dax > -- > 2.21.0 > > >
On 2020/7/15 10:31, Ira Weiny wrote: > On Tue, Jul 14, 2020 at 05:40:05PM +0800, Xiao Yang wrote: >> ext4 can accept the last one if the same mkfs options are passed but xfs cannot > I'm having trouble parsing this commit message. What does 'last one' refer to? > >> accept the same mkfs options and reports "xxx option is respecified" error. > Ok I think I understand now. Some FS's (XFS) do not accept an option more than > once. So we can't just blindly add options to the end of the MKFS_OPTIONS. Is > that correct? Hi Ira, Correct. >> I >> prefer to override the same mkfs option which is defined in MKFS_OPTION so that >> we can have a chance to pass other mkfs options to _scratch_mkfs_geom(). > Instead the patch parses the current option string and replaces the value if > the option is already there. This allows us to specify MKFS_OPTIONS to > generic/223. Right. :-) XFS_MKFS_OPTIONS/MKFS_OPTIONS can be used to specify some custom options by user, so I don't want to clear it blindly. Thanks, Xiao Yang > I think the code is reasonable although my sed skills are not good enough to > tell for sure... ;-) > > Ira > >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >> --- >> common/rc | 14 +++++++++++++- >> tests/generic/223 | 1 - >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index 6c908f2e..567cf83b 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -1051,7 +1051,19 @@ _scratch_mkfs_geom() >> >> case $FSTYP in >> xfs) >> - MKFS_OPTIONS+=" -b size=$blocksize, -d su=$sunit_bytes,sw=$swidth_mult" >> + if echo "$MKFS_OPTIONS" | egrep -q "b?size="; then >> + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r "s/(b?size=)[0-9]+/\1$blocksize/") >> + else >> + MKFS_OPTIONS+=" -b size=$blocksize" >> + fi >> + >> + if echo "$MKFS_OPTIONS" | egrep -q "(su|sunit|sw|swidth)="; then >> + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r \ >> + -e "s/(su|sunit)=[0-9kmg]+/su=$sunit_bytes/" \ >> + -e "s/(sw|swidth)=[0-9kmg]+/sw=$swidth_mult/") >> + else >> + MKFS_OPTIONS+=" -d su=$sunit_bytes,sw=$swidth_mult" >> + fi >> ;; >> ext4|ext4dev) >> MKFS_OPTIONS+=" -b $blocksize -E stride=$sunit_blocks,stripe_width=$swidth_blocks" >> diff --git a/tests/generic/223 b/tests/generic/223 >> index 6cfd00dd..ba7c9a44 100755 >> --- a/tests/generic/223 >> +++ b/tests/generic/223 >> @@ -41,7 +41,6 @@ for SUNIT_K in 8 16 32 64 128; do >> let SUNIT_BLOCKS=$SUNIT_BYTES/$BLOCKSIZE >> >> echo "=== mkfs with su $SUNIT_BLOCKS blocks x 4 ===" >> - export MKFS_OPTIONS="" >> _scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE>> $seqres.full 2>&1 >> _scratch_mount >> >> -- >> 2.21.0 >> >> >> > > . >
On 2020/7/15 9:59, Ira Weiny wrote: > On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote: >> 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option >> 2) _require_dax_iflag() checks FS_XFLAG_DAX >> >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > Reviewed-by: Ira Weiny<ira.weiny@intel.com> Hi Ira, Do you want to check if invalid parameter is passed to _require_scratch_dax_mountopt? Thought I think it is not necessary(user should pass correct parament). Like this: [ "$mountopt" != "dax" ] && [ "$mountopt" != "dax=always" ] && _notrun "$mountopt is invalid" Best Regards, Xiao Yang >> --- >> >> V5->V6: >> Simplify _require_scratch_dax_mountopt because it is enough to only check >> dax/dax=always mount option. See the following reasons: >> 1) we cannot detect if underlying device supports dax by mounting dax=inode >> or dax=never. >> 2) dax=always, dax=inode, dax=never are always introduced together. >> >> common/rc | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/common/rc b/common/rc >> index f17b19f2..aeec1f11 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -3188,6 +3188,28 @@ _require_scratch_dax() >> _scratch_unmount >> } >> >> +# Only accept dax/dax=always mount option becasue dax=always, dax=inode >> +# and dax=never are always introduced together. >> +_require_scratch_dax_mountopt() >> +{ >> + local mountopt=$1 >> + >> + _require_scratch >> + _scratch_mkfs> /dev/null 2>&1 >> + _try_scratch_mount "-o $mountopt"> /dev/null 2>&1 || \ >> + _notrun "mount $SCRATCH_DEV with $mountopt failed" >> + >> + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ >> + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" >> + >> + _scratch_unmount >> +} >> + >> +_require_dax_iflag() >> +{ >> + _require_xfs_io_command "chattr" "x" >> +} >> + >> # Does norecovery support by this fs? >> _require_norecovery() >> { >> -- >> 2.21.0 >> >> >> > > . >
On Wed, Jul 15, 2020 at 11:19:58AM +0800, Xiao Yang wrote: > On 2020/7/15 9:59, Ira Weiny wrote: > > On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote: > > > 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option > > > 2) _require_dax_iflag() checks FS_XFLAG_DAX > > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > Reviewed-by: Ira Weiny<ira.weiny@intel.com> > Hi Ira, > > Do you want to check if invalid parameter is passed to > _require_scratch_dax_mountopt? Thought > I think it is not necessary(user should pass correct parament). > Like this: > [ "$mountopt" != "dax" ] && [ "$mountopt" != "dax=always" ] && _notrun > "$mountopt is invalid" I thought this patch was just checking if the filesystem/device supported DAX such that DAX tests could run using either old _or_ new dax support... But after thinking about it I think we need something more complicated. Something like: if ('-o dax=inode') // FS is DAX with per file support (ie ext4 || XFS kernel >= 5.8) else if ('-o dax') // FS is DAX (ie ext4 || XFS on kernel < 5.8) else // FS has no dax support. Do we do that with 2 functions? _require_scratch_dax_mountopt() _require_scratch_new_dax_mountopt() If both fail we are in the else clause above? We can use the dax=inode because it is a new option and is not necessarily the option used while running the tests. (I think. as I said my xfstests skills are not great so I'm not always sure of the correct patterns) Ira > > Best Regards, > Xiao Yang > > > --- > > > > > > V5->V6: > > > Simplify _require_scratch_dax_mountopt because it is enough to only check > > > dax/dax=always mount option. See the following reasons: > > > 1) we cannot detect if underlying device supports dax by mounting dax=inode > > > or dax=never. > > > 2) dax=always, dax=inode, dax=never are always introduced together. > > > > > > common/rc | 22 ++++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/common/rc b/common/rc > > > index f17b19f2..aeec1f11 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -3188,6 +3188,28 @@ _require_scratch_dax() > > > _scratch_unmount > > > } > > > > > > +# Only accept dax/dax=always mount option becasue dax=always, dax=inode > > > +# and dax=never are always introduced together. > > > +_require_scratch_dax_mountopt() > > > +{ > > > + local mountopt=$1 > > > + > > > + _require_scratch > > > + _scratch_mkfs> /dev/null 2>&1 > > > + _try_scratch_mount "-o $mountopt"> /dev/null 2>&1 || \ > > > + _notrun "mount $SCRATCH_DEV with $mountopt failed" > > > + > > > + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ > > > + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" > > > + > > > + _scratch_unmount > > > +} > > > + > > > +_require_dax_iflag() > > > +{ > > > + _require_xfs_io_command "chattr" "x" > > > +} > > > + > > > # Does norecovery support by this fs? > > > _require_norecovery() > > > { > > > -- > > > 2.21.0 > > > > > > > > > > > > > . > > > > >
On 2020/7/15 10:48, Ira Weiny wrote: > On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >> --- >> tests/generic/605 | 199 ++++++++++++++++++++++++++++++++++++++++++ >> tests/generic/605.out | 2 + >> tests/generic/group | 1 + >> 3 files changed, 202 insertions(+) >> create mode 100644 tests/generic/605 >> create mode 100644 tests/generic/605.out >> >> diff --git a/tests/generic/605 b/tests/generic/605 >> new file mode 100644 >> index 00000000..6924223a >> --- /dev/null >> +++ b/tests/generic/605 >> @@ -0,0 +1,199 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2020 Fujitsu. All Rights Reserved. >> +# >> +# FS QA Test 605 >> +# >> +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various combinations. >> +# 1) New files and directories automatically inherit FS_XFLAG_DAX from their parent directory. >> +# 2) cp operation make files and directories inherit the FS_XFLAG_DAX from new parent directory. >> +# 3) mv operation make files and directories preserve the FS_XFLAG_DAX from old parent directory. >> +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted by dax mount options. >> + >> +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 >> + >> +_supported_fs generic >> +_supported_os Linux >> +_require_scratch >> +_require_dax_iflag >> +_require_xfs_io_command "lsattr" "-v" >> + >> +check_xflag() >> +{ >> + local target=$1 >> + local exp_xflag=$2 >> + >> + if [ $exp_xflag -eq 0 ]; then >> + _test_inode_flag dax $target&& echo "$target has unexpected FS_XFLAG_DAX flag" >> + else >> + _test_inode_flag dax $target || echo "$target doen't have expected FS_XFLAG_DAX flag" >> + fi >> +} >> + >> +test_xflag_inheritance1() >> +{ >> + mkdir -p a >> + $XFS_IO_PROG -c "chattr +x" a >> + mkdir -p a/b/c >> + touch a/b/c/d >> + >> + check_xflag a 1 >> + check_xflag a/b 1 >> + check_xflag a/b/c 1 >> + check_xflag a/b/c/d 1 >> + >> + rm -rf a >> +} >> + >> +test_xflag_inheritance2() >> +{ >> + mkdir -p a/b >> + $XFS_IO_PROG -c "chattr +x" a >> + mkdir -p a/b/c a/d >> + touch a/b/c/e a/d/f >> + >> + check_xflag a 1 >> + check_xflag a/b 0 >> + check_xflag a/b/c 0 >> + check_xflag a/b/c/e 0 >> + check_xflag a/d 1 >> + check_xflag a/d/f 1 >> + >> + rm -rf a >> +} >> + >> +test_xflag_inheritance3() >> +{ >> + mkdir -p a/b >> + $XFS_IO_PROG -c "chattr +x" a/b >> + mkdir -p a/b/c a/d >> + touch a/b/c/e a/d/f >> + >> + check_xflag a 0 >> + check_xflag a/b 1 >> + check_xflag a/b/c 1 >> + check_xflag a/b/c/e 1 >> + check_xflag a/d 0 >> + check_xflag a/d/f 0 >> + >> + rm -rf a >> +} > It really seems like 2 and 3 test the same thing? Hi Ira, 2 constructs the following steps: 1) a is the parent directory of b 2) a doesn't have xflag and b has xflag 3) touch many directories/files in a and b 3 constructs the following steps: 1) a is the parent directory of b and b is the parent directory of c 2) a and c have xflag, and b doesn't have xflag 3) touch many directories/files in b and c Do you think they are same? I can remove one if you think so. >> + >> +test_xflag_inheritance4() >> +{ >> + mkdir -p a >> + $XFS_IO_PROG -c "chattr +x" a >> + mkdir -p a/b/c >> + $XFS_IO_PROG -c "chattr -x" a/b >> + mkdir -p a/b/c/d a/b/e >> + touch a/b/c/d/f a/b/e/g >> + >> + check_xflag a 1 >> + check_xflag a/b 0 >> + check_xflag a/b/c 1 >> + check_xflag a/b/c/d 1 >> + check_xflag a/b/c/d/f 1 >> + check_xflag a/b/e 0 >> + check_xflag a/b/e/g 0 >> + >> + rm -rf a >> +} >> + >> +test_xflag_inheritance5() >> +{ >> + mkdir -p a b >> + $XFS_IO_PROG -c "chattr +x" a >> + mkdir -p a/c a/d b/e b/f >> + touch a/g b/h >> + >> + cp -r a/c b/ >> + cp -r b/e a/ >> + cp -r a/g b/ >> + mv a/d b/ >> + mv b/f a/ >> + mv b/h a/ >> + >> + check_xflag b/c 0 >> + check_xflag b/d 1 >> + check_xflag a/e 1 >> + check_xflag a/f 0 >> + check_xflag b/g 0 >> + check_xflag a/h 0 >> + >> + rm -rf a b >> +} >> + >> +do_xflag_tests() >> +{ >> + local option=$1 >> + >> + _scratch_mount "$option" >> + cd $SCRATCH_MNT >> + >> + for i in $(seq 1 5); do >> + test_xflag_inheritance${i} >> + done >> + >> + cd -> /dev/null >> + _scratch_unmount >> +} >> + >> +check_dax_mountopt() >> +{ >> + local option=$1 >> + local ret=0 >> + >> + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 >> + >> + # Match option name exactly >> + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 >> + >> + _scratch_unmount >> + >> + return $ret >> +} > Should this be a common function? I am not sure if it should be a common function, because it may not be used by other tests in future. I also consider to merge the function into _require_scratch_dax_mountopt(). >> + >> +do_tests() >> +{ >> + # Mount without dax option >> + do_xflag_tests >> + >> + # Mount with old dax option if fs only supports it. >> + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" > I don't understand the order here. If we are on an older kernel and the FS > only supports '-o dax' the do_xflag_tests will fail won't it? With both old dax and new dax, the inheritance behavior of FS_XFLAG_DAX works well. > So shouldn't we do this first and bail/'not run' this test if that is the case? > > I really don't think there is any point in testing the old XFS behavior because > the FS_XFLAG_DAX had no effect. So even if it is broken it does not matter. > Or perhaps I am missing something here? This test is designed to verify the inheritance behavior of FS_XFLAG_DAX(not related to S_DAX) so I think it is fine for both old dax and new dax to run the test. Best Regards, Xiao Yang > Ira > >> + >> + # Mount with new dax options if fs supports them. >> + if check_dax_mountopt "dax=always"; then >> + for dax_option in "dax=always" "dax=inode" "dax=never"; do >> + do_xflag_tests "-o $dax_option" >> + done >> + fi >> +} >> + >> +_scratch_mkfs>> $seqres.full 2>&1 >> + >> +do_tests >> + >> +# success, all done >> +echo "Silence is golden" >> +status=0 >> +exit >> diff --git a/tests/generic/605.out b/tests/generic/605.out >> new file mode 100644 >> index 00000000..1ae20049 >> --- /dev/null >> +++ b/tests/generic/605.out >> @@ -0,0 +1,2 @@ >> +QA output created by 605 >> +Silence is golden >> diff --git a/tests/generic/group b/tests/generic/group >> index 676e0d2e..a8451862 100644 >> --- a/tests/generic/group >> +++ b/tests/generic/group >> @@ -607,3 +607,4 @@ >> 602 auto quick encrypt >> 603 auto attr quick dax >> 604 auto attr quick dax >> +605 auto attr quick dax >> -- >> 2.21.0 >> >> >> > > . >
On 2020/7/15 12:15, Ira Weiny wrote: > On Wed, Jul 15, 2020 at 11:19:58AM +0800, Xiao Yang wrote: >> On 2020/7/15 9:59, Ira Weiny wrote: >>> On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote: >>>> 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option >>>> 2) _require_dax_iflag() checks FS_XFLAG_DAX >>>> >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>> Reviewed-by: Ira Weiny<ira.weiny@intel.com> >> Hi Ira, >> >> Do you want to check if invalid parameter is passed to >> _require_scratch_dax_mountopt? Thought >> I think it is not necessary(user should pass correct parament). >> Like this: >> [ "$mountopt" != "dax" ]&& [ "$mountopt" != "dax=always" ]&& _notrun >> "$mountopt is invalid" > I thought this patch was just checking if the filesystem/device supported DAX > such that DAX tests could run using either old _or_ new dax support... > But after thinking about it I think we need something more complicated. > > Something like: > > if ('-o dax=inode') > // FS is DAX with per file support (ie ext4 || XFS kernel>= 5.8) Hi Ira, dax=inode/dax=never cannot check if underlying device supports dax, so I perfer to use dax=always For example: XFS: # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/ # mount | grep sda8 /dev/sda8 on /mnt/xfstests/test type xfs (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota) ... # mount -o dax=always /dev/sda8 /mnt/xfstests/test/ # mount | grep sda8 /dev/sda8 on /mnt/xfstests/test type xfs (rw,relatime,seclabel,attr2,dax=never,inode64,logbufs=8,logbsize=32k,noquota) EXT4: # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/ # mount | grep sda8 /dev/sda8 on /mnt/xfstests/test type ext4 (rw,relatime,seclabel,dax=inode) ... # mount -o dax=always /dev/sda8 /mnt/xfstests/test/ mount: /mnt/xfstests/test: wrong fs type, bad option, bad superblock on /dev/sda8, missing codepage or helper program, or other error. # dmesg | grep DAX [597988.165120] EXT4-fs (sda8): DAX unsupported by block device. If we want to test new dax(i.e. dax=inode, dax=always or dax=never), so pass "dax=always" to _require_scratch_dax_mountopt If we want to test old dax or new dax=always(either of them), so pass "dax" to _require_scratch_dax_mountopt Best Regards, Xiao Yang > else if ('-o dax') > // FS is DAX (ie ext4 || XFS on kernel< 5.8) > else > // FS has no dax support. > > Do we do that with 2 functions? > > _require_scratch_dax_mountopt() > _require_scratch_new_dax_mountopt() > > If both fail we are in the else clause above? > > We can use the dax=inode because it is a new option and is not necessarily the > option used while running the tests. (I think. as I said my xfstests skills > are not great so I'm not always sure of the correct patterns) > > Ira > >> Best Regards, >> Xiao Yang >>>> --- >>>> >>>> V5->V6: >>>> Simplify _require_scratch_dax_mountopt because it is enough to only check >>>> dax/dax=always mount option. See the following reasons: >>>> 1) we cannot detect if underlying device supports dax by mounting dax=inode >>>> or dax=never. >>>> 2) dax=always, dax=inode, dax=never are always introduced together. >>>> >>>> common/rc | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index f17b19f2..aeec1f11 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -3188,6 +3188,28 @@ _require_scratch_dax() >>>> _scratch_unmount >>>> } >>>> >>>> +# Only accept dax/dax=always mount option becasue dax=always, dax=inode >>>> +# and dax=never are always introduced together. >>>> +_require_scratch_dax_mountopt() >>>> +{ >>>> + local mountopt=$1 >>>> + >>>> + _require_scratch >>>> + _scratch_mkfs> /dev/null 2>&1 >>>> + _try_scratch_mount "-o $mountopt"> /dev/null 2>&1 || \ >>>> + _notrun "mount $SCRATCH_DEV with $mountopt failed" >>>> + >>>> + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ >>>> + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" >>>> + >>>> + _scratch_unmount >>>> +} >>>> + >>>> +_require_dax_iflag() >>>> +{ >>>> + _require_xfs_io_command "chattr" "x" >>>> +} >>>> + >>>> # Does norecovery support by this fs? >>>> _require_norecovery() >>>> { >>>> -- >>>> 2.21.0 >>>> >>>> >>>> >>> . >>> >> >> > > . >
On 2020/7/15 13:39, Xiao Yang wrote: > On 2020/7/15 10:48, Ira Weiny wrote: >> On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: >>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>> --- >>> tests/generic/605 | 199 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> tests/generic/605.out | 2 + >>> tests/generic/group | 1 + >>> 3 files changed, 202 insertions(+) >>> create mode 100644 tests/generic/605 >>> create mode 100644 tests/generic/605.out >>> >>> diff --git a/tests/generic/605 b/tests/generic/605 >>> new file mode 100644 >>> index 00000000..6924223a >>> --- /dev/null >>> +++ b/tests/generic/605 >>> @@ -0,0 +1,199 @@ >>> +#! /bin/bash >>> +# SPDX-License-Identifier: GPL-2.0 >>> +# Copyright (c) 2020 Fujitsu. All Rights Reserved. >>> +# >>> +# FS QA Test 605 >>> +# >>> +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various >>> combinations. >>> +# 1) New files and directories automatically inherit FS_XFLAG_DAX >>> from their parent directory. >>> +# 2) cp operation make files and directories inherit the >>> FS_XFLAG_DAX from new parent directory. >>> +# 3) mv operation make files and directories preserve the >>> FS_XFLAG_DAX from old parent directory. >>> +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted >>> by dax mount options. >>> + >>> +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 >>> + >>> +_supported_fs generic >>> +_supported_os Linux >>> +_require_scratch >>> +_require_dax_iflag >>> +_require_xfs_io_command "lsattr" "-v" >>> + >>> +check_xflag() >>> +{ >>> + local target=$1 >>> + local exp_xflag=$2 >>> + >>> + if [ $exp_xflag -eq 0 ]; then >>> + _test_inode_flag dax $target&& echo "$target has >>> unexpected FS_XFLAG_DAX flag" >>> + else >>> + _test_inode_flag dax $target || echo "$target doen't have >>> expected FS_XFLAG_DAX flag" >>> + fi >>> +} >>> + >>> +test_xflag_inheritance1() >>> +{ >>> + mkdir -p a >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c >>> + touch a/b/c/d >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 1 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/d 1 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance2() >>> +{ >>> + mkdir -p a/b >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c a/d >>> + touch a/b/c/e a/d/f >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 0 >>> + check_xflag a/b/c 0 >>> + check_xflag a/b/c/e 0 >>> + check_xflag a/d 1 >>> + check_xflag a/d/f 1 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance3() >>> +{ >>> + mkdir -p a/b >>> + $XFS_IO_PROG -c "chattr +x" a/b >>> + mkdir -p a/b/c a/d >>> + touch a/b/c/e a/d/f >>> + >>> + check_xflag a 0 >>> + check_xflag a/b 1 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/e 1 >>> + check_xflag a/d 0 >>> + check_xflag a/d/f 0 >>> + >>> + rm -rf a >>> +} >> It really seems like 2 and 3 test the same thing? > Hi Ira, > > 2 constructs the following steps: > 1) a is the parent directory of b > 2) a doesn't have xflag and b has xflag > 3) touch many directories/files in a and b > > 3 constructs the following steps: > 1) a is the parent directory of b and b is the parent directory of c > 2) a and c have xflag, and b doesn't have xflag > 3) touch many directories/files in b and c > > Do you think they are same? I can remove one if you think so. > >>> + >>> +test_xflag_inheritance4() >>> +{ >>> + mkdir -p a >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c >>> + $XFS_IO_PROG -c "chattr -x" a/b >>> + mkdir -p a/b/c/d a/b/e >>> + touch a/b/c/d/f a/b/e/g >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 0 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/d 1 >>> + check_xflag a/b/c/d/f 1 >>> + check_xflag a/b/e 0 >>> + check_xflag a/b/e/g 0 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance5() >>> +{ >>> + mkdir -p a b >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/c a/d b/e b/f >>> + touch a/g b/h >>> + >>> + cp -r a/c b/ >>> + cp -r b/e a/ >>> + cp -r a/g b/ >>> + mv a/d b/ >>> + mv b/f a/ >>> + mv b/h a/ >>> + >>> + check_xflag b/c 0 >>> + check_xflag b/d 1 >>> + check_xflag a/e 1 >>> + check_xflag a/f 0 >>> + check_xflag b/g 0 >>> + check_xflag a/h 0 >>> + >>> + rm -rf a b >>> +} >>> + >>> +do_xflag_tests() >>> +{ >>> + local option=$1 >>> + >>> + _scratch_mount "$option" >>> + cd $SCRATCH_MNT >>> + >>> + for i in $(seq 1 5); do >>> + test_xflag_inheritance${i} >>> + done >>> + >>> + cd -> /dev/null >>> + _scratch_unmount >>> +} >>> + >>> +check_dax_mountopt() >>> +{ >>> + local option=$1 >>> + local ret=0 >>> + >>> + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 >>> + >>> + # Match option name exactly >>> + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 >>> + >>> + _scratch_unmount >>> + >>> + return $ret >>> +} >> Should this be a common function? > > I am not sure if it should be a common function, because it may not be > used by other tests in future. > I also consider to merge the function into > _require_scratch_dax_mountopt(). For this comment, I try to merge the function into _require_scratch_dax_mountopt(), as below: ---------------------------------------------------------------------------------------------------------------- +# Only accept dax/dax=always mount option becasue dax=always, dax=inode +# and dax=never are always introduced together. +# Return 0 if filesystem/device supports the specified dax option. +# Return 1 if mount fails with the specified dax option. +# Return 2 if /proc/mounts shows wrong dax option. +# Check new dax=inode, dax=always or dax=never option by passing "dax=always". +# Check old dax or new dax=always by passing "dax". +_check_scratch_dax_mountopt() +{ + local option=$1 + + echo "$option" | egrep -q "dax(=always|$)" || \ + _notrun "invalid $option, only accept dax/dax=always" + + _require_scratch + _scratch_mkfs > /dev/null 2>&1 + + _try_scratch_mount "-o $option" > /dev/null 2>&1 || return 1 + + if _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)"; then + _scratch_unmount + return 0 + else + _scratch_unmount + return 2 + fi +} + +# Throw notrun if _check_scratch_dax_mountopt() returns a non-zero value. +_require_scratch_dax_mountopt() +{ + local mountopt=$1 + + _check_scratch_dax_mountopt "$mountopt" + local res=$? + + [ $res -eq 1 ] && _notrun "mount $SCRATCH_DEV with $mountopt failed" + [ $res -eq 2 ] && _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" +} ----------------------------------------------------------------------------------------------------------- > >>> + >>> +do_tests() >>> +{ >>> + # Mount without dax option >>> + do_xflag_tests >>> + >>> + # Mount with old dax option if fs only supports it. >>> + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" >> I don't understand the order here. If we are on an older kernel and >> the FS >> only supports '-o dax' the do_xflag_tests will fail won't it? > > With both old dax and new dax, the inheritance behavior of > FS_XFLAG_DAX works well. > >> So shouldn't we do this first and bail/'not run' this test if that is >> the case? >> >> I really don't think there is any point in testing the old XFS >> behavior because >> the FS_XFLAG_DAX had no effect. So even if it is broken it does not >> matter. >> Or perhaps I am missing something here? > > This test is designed to verify the inheritance behavior of > FS_XFLAG_DAX(not related to S_DAX) > so I think it is fine for both old dax and new dax to run the test. For this comment, I try to update it, as below: ------------------------------------------------------------------------- +do_tests() +{ + # Mount without dax option + do_xflag_tests + + # Mount with 'dax' or 'dax=always' option if fs supports it. + _check_scratch_dax_mountopt "dax" && do_xflag_tests "-o dax" + + # Mount with 'dax=inode' and 'dax=never' options if fs supports them. + if _check_scratch_dax_mountopt "dax=always"; then + for dax_option in "dax=inode" "dax=never"; do + do_xflag_tests "-o $dax_option" + done + fi +} ------------------------------------------------------------------------- > > Best Regards, > Xiao Yang >> Ira >> >>> + >>> + # Mount with new dax options if fs supports them. >>> + if check_dax_mountopt "dax=always"; then >>> + for dax_option in "dax=always" "dax=inode" "dax=never"; do >>> + do_xflag_tests "-o $dax_option" >>> + done >>> + fi >>> +} >>> + >>> +_scratch_mkfs>> $seqres.full 2>&1 >>> + >>> +do_tests >>> + >>> +# success, all done >>> +echo "Silence is golden" >>> +status=0 >>> +exit >>> diff --git a/tests/generic/605.out b/tests/generic/605.out >>> new file mode 100644 >>> index 00000000..1ae20049 >>> --- /dev/null >>> +++ b/tests/generic/605.out >>> @@ -0,0 +1,2 @@ >>> +QA output created by 605 >>> +Silence is golden >>> diff --git a/tests/generic/group b/tests/generic/group >>> index 676e0d2e..a8451862 100644 >>> --- a/tests/generic/group >>> +++ b/tests/generic/group >>> @@ -607,3 +607,4 @@ >>> 602 auto quick encrypt >>> 603 auto attr quick dax >>> 604 auto attr quick dax >>> +605 auto attr quick dax >>> -- >>> 2.21.0 >>> >>> >>> >> >> . >> > > > > . >
On 2020/7/15 13:39, Xiao Yang wrote: > On 2020/7/15 10:48, Ira Weiny wrote: >> On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: >>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>> --- >>> tests/generic/605 | 199 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> tests/generic/605.out | 2 + >>> tests/generic/group | 1 + >>> 3 files changed, 202 insertions(+) >>> create mode 100644 tests/generic/605 >>> create mode 100644 tests/generic/605.out >>> >>> diff --git a/tests/generic/605 b/tests/generic/605 >>> new file mode 100644 >>> index 00000000..6924223a >>> --- /dev/null >>> +++ b/tests/generic/605 >>> @@ -0,0 +1,199 @@ >>> +#! /bin/bash >>> +# SPDX-License-Identifier: GPL-2.0 >>> +# Copyright (c) 2020 Fujitsu. All Rights Reserved. >>> +# >>> +# FS QA Test 605 >>> +# >>> +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various >>> combinations. >>> +# 1) New files and directories automatically inherit FS_XFLAG_DAX >>> from their parent directory. >>> +# 2) cp operation make files and directories inherit the >>> FS_XFLAG_DAX from new parent directory. >>> +# 3) mv operation make files and directories preserve the >>> FS_XFLAG_DAX from old parent directory. >>> +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted >>> by dax mount options. >>> + >>> +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 >>> + >>> +_supported_fs generic >>> +_supported_os Linux >>> +_require_scratch >>> +_require_dax_iflag >>> +_require_xfs_io_command "lsattr" "-v" >>> + >>> +check_xflag() >>> +{ >>> + local target=$1 >>> + local exp_xflag=$2 >>> + >>> + if [ $exp_xflag -eq 0 ]; then >>> + _test_inode_flag dax $target&& echo "$target has >>> unexpected FS_XFLAG_DAX flag" >>> + else >>> + _test_inode_flag dax $target || echo "$target doen't have >>> expected FS_XFLAG_DAX flag" >>> + fi >>> +} >>> + >>> +test_xflag_inheritance1() >>> +{ >>> + mkdir -p a >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c >>> + touch a/b/c/d >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 1 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/d 1 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance2() >>> +{ >>> + mkdir -p a/b >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c a/d >>> + touch a/b/c/e a/d/f >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 0 >>> + check_xflag a/b/c 0 >>> + check_xflag a/b/c/e 0 >>> + check_xflag a/d 1 >>> + check_xflag a/d/f 1 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance3() >>> +{ >>> + mkdir -p a/b >>> + $XFS_IO_PROG -c "chattr +x" a/b >>> + mkdir -p a/b/c a/d >>> + touch a/b/c/e a/d/f >>> + >>> + check_xflag a 0 >>> + check_xflag a/b 1 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/e 1 >>> + check_xflag a/d 0 >>> + check_xflag a/d/f 0 >>> + >>> + rm -rf a >>> +} >> It really seems like 2 and 3 test the same thing? > Hi Ira, > > 2 constructs the following steps: > 1) a is the parent directory of b > 2) a doesn't have xflag and b has xflag > 3) touch many directories/files in a and b > > 3 constructs the following steps: > 1) a is the parent directory of b and b is the parent directory of c > 2) a and c have xflag, and b doesn't have xflag > 3) touch many directories/files in b and c Hi Ira, Sorry for misreading your comment, above is the difference between 3 and 4. The correct one is: 2 constructs the following steps: 1) a is the parent directory of b 2) a has xflag and b doesn't have xflag 3) touch many directories/files in a and b 3 constructs the following steps: 1) a is the parent directory of b 2) a doesn't have xflag and b has xflag 3) touch many directories/files in a and b Do you think they are same? I can remove one if you think so. Best Regards, Xiao Yang > > Do you think they are same? I can remove one if you think so. > >>> + >>> +test_xflag_inheritance4() >>> +{ >>> + mkdir -p a >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/b/c >>> + $XFS_IO_PROG -c "chattr -x" a/b >>> + mkdir -p a/b/c/d a/b/e >>> + touch a/b/c/d/f a/b/e/g >>> + >>> + check_xflag a 1 >>> + check_xflag a/b 0 >>> + check_xflag a/b/c 1 >>> + check_xflag a/b/c/d 1 >>> + check_xflag a/b/c/d/f 1 >>> + check_xflag a/b/e 0 >>> + check_xflag a/b/e/g 0 >>> + >>> + rm -rf a >>> +} >>> + >>> +test_xflag_inheritance5() >>> +{ >>> + mkdir -p a b >>> + $XFS_IO_PROG -c "chattr +x" a >>> + mkdir -p a/c a/d b/e b/f >>> + touch a/g b/h >>> + >>> + cp -r a/c b/ >>> + cp -r b/e a/ >>> + cp -r a/g b/ >>> + mv a/d b/ >>> + mv b/f a/ >>> + mv b/h a/ >>> + >>> + check_xflag b/c 0 >>> + check_xflag b/d 1 >>> + check_xflag a/e 1 >>> + check_xflag a/f 0 >>> + check_xflag b/g 0 >>> + check_xflag a/h 0 >>> + >>> + rm -rf a b >>> +} >>> + >>> +do_xflag_tests() >>> +{ >>> + local option=$1 >>> + >>> + _scratch_mount "$option" >>> + cd $SCRATCH_MNT >>> + >>> + for i in $(seq 1 5); do >>> + test_xflag_inheritance${i} >>> + done >>> + >>> + cd -> /dev/null >>> + _scratch_unmount >>> +} >>> + >>> +check_dax_mountopt() >>> +{ >>> + local option=$1 >>> + local ret=0 >>> + >>> + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 >>> + >>> + # Match option name exactly >>> + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 >>> + >>> + _scratch_unmount >>> + >>> + return $ret >>> +} >> Should this be a common function? > > I am not sure if it should be a common function, because it may not be > used by other tests in future. > I also consider to merge the function into > _require_scratch_dax_mountopt(). > >>> + >>> +do_tests() >>> +{ >>> + # Mount without dax option >>> + do_xflag_tests >>> + >>> + # Mount with old dax option if fs only supports it. >>> + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" >> I don't understand the order here. If we are on an older kernel and >> the FS >> only supports '-o dax' the do_xflag_tests will fail won't it? > > With both old dax and new dax, the inheritance behavior of > FS_XFLAG_DAX works well. > >> So shouldn't we do this first and bail/'not run' this test if that is >> the case? >> >> I really don't think there is any point in testing the old XFS >> behavior because >> the FS_XFLAG_DAX had no effect. So even if it is broken it does not >> matter. >> Or perhaps I am missing something here? > > This test is designed to verify the inheritance behavior of > FS_XFLAG_DAX(not related to S_DAX) > so I think it is fine for both old dax and new dax to run the test. > > Best Regards, > Xiao Yang >> Ira >> >>> + >>> + # Mount with new dax options if fs supports them. >>> + if check_dax_mountopt "dax=always"; then >>> + for dax_option in "dax=always" "dax=inode" "dax=never"; do >>> + do_xflag_tests "-o $dax_option" >>> + done >>> + fi >>> +} >>> + >>> +_scratch_mkfs>> $seqres.full 2>&1 >>> + >>> +do_tests >>> + >>> +# success, all done >>> +echo "Silence is golden" >>> +status=0 >>> +exit >>> diff --git a/tests/generic/605.out b/tests/generic/605.out >>> new file mode 100644 >>> index 00000000..1ae20049 >>> --- /dev/null >>> +++ b/tests/generic/605.out >>> @@ -0,0 +1,2 @@ >>> +QA output created by 605 >>> +Silence is golden >>> diff --git a/tests/generic/group b/tests/generic/group >>> index 676e0d2e..a8451862 100644 >>> --- a/tests/generic/group >>> +++ b/tests/generic/group >>> @@ -607,3 +607,4 @@ >>> 602 auto quick encrypt >>> 603 auto attr quick dax >>> 604 auto attr quick dax >>> +605 auto attr quick dax >>> -- >>> 2.21.0 >>> >>> >>> >> >> . >> > > > > . >
On Wed, Jul 15, 2020 at 01:55:14PM +0800, Xiao Yang wrote: > On 2020/7/15 12:15, Ira Weiny wrote: > > On Wed, Jul 15, 2020 at 11:19:58AM +0800, Xiao Yang wrote: > > > On 2020/7/15 9:59, Ira Weiny wrote: > > > > On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote: > > > > > 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option > > > > > 2) _require_dax_iflag() checks FS_XFLAG_DAX > > > > > > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > > > Reviewed-by: Ira Weiny<ira.weiny@intel.com> > > > Hi Ira, > > > > > > Do you want to check if invalid parameter is passed to > > > _require_scratch_dax_mountopt? Thought > > > I think it is not necessary(user should pass correct parament). > > > Like this: > > > [ "$mountopt" != "dax" ]&& [ "$mountopt" != "dax=always" ]&& _notrun > > > "$mountopt is invalid" > > I thought this patch was just checking if the filesystem/device supported DAX > > such that DAX tests could run using either old _or_ new dax support... > > But after thinking about it I think we need something more complicated. > > > > Something like: > > > > if ('-o dax=inode') > > // FS is DAX with per file support (ie ext4 || XFS kernel>= 5.8) > Hi Ira, > > dax=inode/dax=never cannot check if underlying device supports dax, so I > perfer to use dax=always > For example: > XFS: > # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/ > # mount | grep sda8 > /dev/sda8 on /mnt/xfstests/test type xfs > (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota) > ... > # mount -o dax=always /dev/sda8 /mnt/xfstests/test/ > # mount | grep sda8 > /dev/sda8 on /mnt/xfstests/test type xfs > (rw,relatime,seclabel,attr2,dax=never,inode64,logbufs=8,logbsize=32k,noquota) > > EXT4: > # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/ > # mount | grep sda8 > /dev/sda8 on /mnt/xfstests/test type ext4 (rw,relatime,seclabel,dax=inode) > ... > # mount -o dax=always /dev/sda8 /mnt/xfstests/test/ > mount: /mnt/xfstests/test: wrong fs type, bad option, bad superblock on > /dev/sda8, missing codepage or helper program, or other error. > # dmesg | grep DAX > [597988.165120] EXT4-fs (sda8): DAX unsupported by block device. > > If we want to test new dax(i.e. dax=inode, dax=always or dax=never), so pass > "dax=always" to _require_scratch_dax_mountopt > If we want to test old dax or new dax=always(either of them), so pass "dax" > to _require_scratch_dax_mountopt That does seem like the only way to figure out if a fs/device combination supports dax. The valid arguments to the _require_scratch_dax* helpers are worth mentioning in a comment above the function, but I suspect that adding validation code is probably overkill. --D > Best Regards, > Xiao Yang > > else if ('-o dax') > > // FS is DAX (ie ext4 || XFS on kernel< 5.8) > > else > > // FS has no dax support. > > > > Do we do that with 2 functions? > > > > _require_scratch_dax_mountopt() > > _require_scratch_new_dax_mountopt() > > > > If both fail we are in the else clause above? > > > > We can use the dax=inode because it is a new option and is not necessarily the > > option used while running the tests. (I think. as I said my xfstests skills > > are not great so I'm not always sure of the correct patterns) > > > > Ira > > > > > Best Regards, > > > Xiao Yang > > > > > --- > > > > > > > > > > V5->V6: > > > > > Simplify _require_scratch_dax_mountopt because it is enough to only check > > > > > dax/dax=always mount option. See the following reasons: > > > > > 1) we cannot detect if underlying device supports dax by mounting dax=inode > > > > > or dax=never. > > > > > 2) dax=always, dax=inode, dax=never are always introduced together. > > > > > > > > > > common/rc | 22 ++++++++++++++++++++++ > > > > > 1 file changed, 22 insertions(+) > > > > > > > > > > diff --git a/common/rc b/common/rc > > > > > index f17b19f2..aeec1f11 100644 > > > > > --- a/common/rc > > > > > +++ b/common/rc > > > > > @@ -3188,6 +3188,28 @@ _require_scratch_dax() > > > > > _scratch_unmount > > > > > } > > > > > > > > > > +# Only accept dax/dax=always mount option becasue dax=always, dax=inode > > > > > +# and dax=never are always introduced together. > > > > > +_require_scratch_dax_mountopt() > > > > > +{ > > > > > + local mountopt=$1 > > > > > + > > > > > + _require_scratch > > > > > + _scratch_mkfs> /dev/null 2>&1 > > > > > + _try_scratch_mount "-o $mountopt"> /dev/null 2>&1 || \ > > > > > + _notrun "mount $SCRATCH_DEV with $mountopt failed" > > > > > + > > > > > + _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)" || \ > > > > > + _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt" > > > > > + > > > > > + _scratch_unmount > > > > > +} > > > > > + > > > > > +_require_dax_iflag() > > > > > +{ > > > > > + _require_xfs_io_command "chattr" "x" > > > > > +} > > > > > + > > > > > # Does norecovery support by this fs? > > > > > _require_norecovery() > > > > > { > > > > > -- > > > > > 2.21.0 > > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > >
On Wed, Jul 15, 2020 at 11:12:36AM +0800, Xiao Yang wrote: > On 2020/7/15 10:31, Ira Weiny wrote: > > On Tue, Jul 14, 2020 at 05:40:05PM +0800, Xiao Yang wrote: > > > ext4 can accept the last one if the same mkfs options are passed but xfs cannot > > I'm having trouble parsing this commit message. What does 'last one' refer to? > > > > > accept the same mkfs options and reports "xxx option is respecified" error. > > Ok I think I understand now. Some FS's (XFS) do not accept an option more than > > once. So we can't just blindly add options to the end of the MKFS_OPTIONS. Is > > that correct? > Hi Ira, > > Correct. > > > I > > > prefer to override the same mkfs option which is defined in MKFS_OPTION so that > > > we can have a chance to pass other mkfs options to _scratch_mkfs_geom(). > > Instead the patch parses the current option string and replaces the value if > > the option is already there. This allows us to specify MKFS_OPTIONS to > > generic/223. > Right. :-) > > XFS_MKFS_OPTIONS/MKFS_OPTIONS can be used to specify some custom options by user, > so I don't want to clear it blindly. > > Thanks, > Xiao Yang > > I think the code is reasonable although my sed skills are not good enough to > > tell for sure... ;-) > > > > Ira > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > > --- > > > common/rc | 14 +++++++++++++- > > > tests/generic/223 | 1 - > > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/common/rc b/common/rc > > > index 6c908f2e..567cf83b 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -1051,7 +1051,19 @@ _scratch_mkfs_geom() > > > > > > case $FSTYP in > > > xfs) > > > - MKFS_OPTIONS+=" -b size=$blocksize, -d su=$sunit_bytes,sw=$swidth_mult" > > > + if echo "$MKFS_OPTIONS" | egrep -q "b?size="; then > > > + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r "s/(b?size=)[0-9]+/\1$blocksize/") > > > + else > > > + MKFS_OPTIONS+=" -b size=$blocksize" > > > + fi > > > + > > > + if echo "$MKFS_OPTIONS" | egrep -q "(su|sunit|sw|swidth)="; then > > > + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r \ > > > + -e "s/(su|sunit)=[0-9kmg]+/su=$sunit_bytes/" \ > > > + -e "s/(sw|swidth)=[0-9kmg]+/sw=$swidth_mult/") > > > + else > > > + MKFS_OPTIONS+=" -d su=$sunit_bytes,sw=$swidth_mult" > > > + fi ...ok, I see, this makes the function smart enough to substitute geometry parameters instead of dumping them on the end and letting that blow up. Heh, ok, that's definitely a weird quirk I've noticed. > > > ;; > > > ext4|ext4dev) > > > MKFS_OPTIONS+=" -b $blocksize -E stride=$sunit_blocks,stripe_width=$swidth_blocks" > > > diff --git a/tests/generic/223 b/tests/generic/223 > > > index 6cfd00dd..ba7c9a44 100755 > > > --- a/tests/generic/223 > > > +++ b/tests/generic/223 > > > @@ -41,7 +41,6 @@ for SUNIT_K in 8 16 32 64 128; do > > > let SUNIT_BLOCKS=$SUNIT_BYTES/$BLOCKSIZE > > > > > > echo "=== mkfs with su $SUNIT_BLOCKS blocks x 4 ===" > > > - export MKFS_OPTIONS="" So I guess you're deleting this so that the test runs with whatever MKFS_OPTIONS the test runner specified, while letting the test edit blocksize and stripe parameters? Proving I'm still bad at remembering to read commit messages, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > > _scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE>> $seqres.full 2>&1 > > > _scratch_mount > > > > > > -- > > > 2.21.0 > > > > > > > > > > > > > . > > > > >
On Tue, Jul 14, 2020 at 05:40:04PM +0800, Xiao Yang wrote: > 1) Make related tests use _require_scratch_dax_mountopt() and _require_dax_iflag() > 2) Remove unused _require_scratch_dax() > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> Looks ok to me, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > V5->V6: > Merge the third patch into the second patch. > > common/rc | 13 ------------- > tests/ext4/030 | 2 +- > tests/ext4/031 | 4 ++-- > tests/generic/413 | 2 +- > tests/generic/462 | 2 +- > tests/xfs/260 | 4 ++-- > 6 files changed, 7 insertions(+), 20 deletions(-) > > diff --git a/common/rc b/common/rc > index aeec1f11..6c908f2e 100644 > --- a/common/rc > +++ b/common/rc > @@ -3175,19 +3175,6 @@ _require_scratch_shutdown() > } > > # Does dax mount option work on this dev/fs? > -_require_scratch_dax() > -{ > - _require_scratch > - _scratch_mkfs > /dev/null 2>&1 > - _try_scratch_mount -o dax || \ > - _notrun "mount $SCRATCH_DEV with dax failed" > - # Check options to be sure. XFS ignores dax option > - # and goes on if dev underneath does not support dax. > - _fs_options $SCRATCH_DEV | grep -qw "dax" || \ > - _notrun "$SCRATCH_DEV $FSTYP does not support -o dax" > - _scratch_unmount > -} > - > # Only accept dax/dax=always mount option becasue dax=always, dax=inode > # and dax=never are always introduced together. > _require_scratch_dax_mountopt() > diff --git a/tests/ext4/030 b/tests/ext4/030 > index 93bbca86..fb5cf451 100755 > --- a/tests/ext4/030 > +++ b/tests/ext4/030 > @@ -33,7 +33,7 @@ rm -f $seqres.full > # Modify as appropriate. > _supported_os Linux > _supported_fs ext4 > -_require_scratch_dax > +_require_scratch_dax_mountopt "dax" > _require_test_program "t_ext4_dax_journal_corruption" > _require_command "$CHATTR_PROG" chattr > > diff --git a/tests/ext4/031 b/tests/ext4/031 > index dc58214e..20e2fab7 100755 > --- a/tests/ext4/031 > +++ b/tests/ext4/031 > @@ -37,7 +37,7 @@ MOUNT_OPTIONS="" > # Modify as appropriate. > _supported_os Linux > _supported_fs ext4 > -_require_scratch_dax > +_require_scratch_dax_mountopt "dax" > _require_test_program "t_ext4_dax_inline_corruption" > _require_scratch_ext4_feature "inline_data" > > @@ -56,7 +56,7 @@ _scratch_unmount >> $seqres.full 2>&1 > _try_scratch_mount "-o dax" >> $seqres.full 2>&1 > > if [[ $? != 0 ]]; then > - # _require_scratch_dax already verified that we could mount with DAX. > + # _require_scratch_dax_mountopt already verified that we could mount with DAX. > # Failure here is expected because we have inline data. > echo "Silence is golden" > status=0 > diff --git a/tests/generic/413 b/tests/generic/413 > index 1ce89aff..19e1b926 100755 > --- a/tests/generic/413 > +++ b/tests/generic/413 > @@ -31,7 +31,7 @@ rm -f $seqres.full > _supported_fs generic > _supported_os Linux > _require_test > -_require_scratch_dax > +_require_scratch_dax_mountopt "dax" > _require_test_program "feature" > _require_test_program "t_mmap_dio" > _require_xfs_io_command "falloc" > diff --git a/tests/generic/462 b/tests/generic/462 > index 1ab6cadc..4a940239 100755 > --- a/tests/generic/462 > +++ b/tests/generic/462 > @@ -37,7 +37,7 @@ rm -f $seqres.full > _supported_fs generic > _supported_os Linux > _require_test > -_require_scratch_dax > +_require_scratch_dax_mountopt "dax" > _require_test_program "t_mmap_write_ro" > # running by unpriviliged user is not necessary to reproduce > # this bug, just trying to test more. > diff --git a/tests/xfs/260 b/tests/xfs/260 > index 3464ffef..bcdc6041 100755 > --- a/tests/xfs/260 > +++ b/tests/xfs/260 > @@ -30,10 +30,10 @@ rm -f $seqres.full > > _supported_fs xfs > _supported_os Linux > -_require_scratch_dax > +_require_scratch_dax_mountopt "dax" > _require_test_program "feature" > _require_test_program "t_mmap_dio" > -_require_xfs_io_command "chattr" "x" > +_require_dax_iflag > _require_xfs_io_command "falloc" > > prep_files() > -- > 2.21.0 > > >
On Tue, Jul 14, 2020 at 05:40:06PM +0800, Xiao Yang wrote: > 1) Simple code and fix the wrong value of stripe_width by _scratch_mkfs_geom(). > 2) Get hugepage size by _get_hugepagesize() and replace fixed 2M with > hugepage size because hugepage size/PMD_SIZE is not 2M on some > arches(e.g. hugepage size/PMD_SIZE is 512M on arm64). > 3) For debugging, redirect the output of mkfs to $seqres.full. > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> Thanks for making this change... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > common/rc | 10 ++++++++++ > tests/generic/413 | 10 ++-------- > tests/xfs/260 | 4 ++-- > 3 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/common/rc b/common/rc > index 567cf83b..b6a48241 100644 > --- a/common/rc > +++ b/common/rc > @@ -170,6 +170,16 @@ _get_filesize() > stat -c %s "$1" > } > > +# Get hugepagesize in bytes > +_get_hugepagesize() > +{ > + local hugepgsz=$(awk '/Hugepagesize/ {print $2}' /proc/meminfo) > + # Call _notrun if $hugepgsz is not a number > + echo "$hugepgsz" | egrep -q ^[0-9]+$ || \ > + _notrun "Cannot get the value of Hugepagesize" > + echo $((hugepgsz * 1024)) > +} > + > _mount() > { > $MOUNT_PROG `_mount_ops_filter $*` > diff --git a/tests/generic/413 b/tests/generic/413 > index 19e1b926..dfe2912b 100755 > --- a/tests/generic/413 > +++ b/tests/generic/413 > @@ -111,14 +111,8 @@ do_tests() > t_mmap_dio_dax $((64 * 1024 * 1024)) > } > > -# make fs 2Mb aligned for PMD fault testing > -mkfs_opts="" > -if [ "$FSTYP" == "ext4" ]; then > - mkfs_opts="-E stride=512,stripe_width=1" > -elif [ "$FSTYP" == "xfs" ]; then > - mkfs_opts="-d su=2m,sw=1" > -fi > -_scratch_mkfs "$mkfs_opts" > /dev/null 2>&1 > +# make fs aligned for PMD fault testing > +_scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 > > # mount SCRATCH_DEV with dax option, TEST_DEV not > export MOUNT_OPTIONS="" > diff --git a/tests/xfs/260 b/tests/xfs/260 > index bcdc6041..81b42f01 100755 > --- a/tests/xfs/260 > +++ b/tests/xfs/260 > @@ -121,8 +121,8 @@ do_tests() > t_dax_flag_mmap_dio $((64 * 1024 * 1024)) > } > > -# make xfs 2Mb aligned for PMD fault testing > -_scratch_mkfs "-d su=2m,sw=1" > /dev/null 2>&1 > +# make xfs aligned for PMD fault testing > +_scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 > > # mount with dax option > _scratch_mount "-o dax" > -- > 2.21.0 > > >
On Tue, Jul 14, 2020 at 05:40:07PM +0800, Xiao Yang wrote: > 1) Both ext4 and xfs have supported FS_XFLAG_DAX so move it to generic. > 2) Modifying FS_XFLAG_DAX on flies does not take effect immediately so > make files inherit the DAX state of parent directory. > 3) Setting/clearing FS_XFLAG_DAX have no chance to change S_DAX flag if > mount with dax option so remove the related subtest. > 4) Setting/clearing FS_XFLAG_DAX doesn't change S_DAX flag on older xfs > due to commit 742d84290739 ("xfs: disable per-inode DAX flag") so > only do test when fs supports new dax=inode option. > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > tests/{xfs/260 => generic/603} | 68 +++++++++++++++++----------------- Ooh renaming detection, nice... Looks fine to me, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > tests/generic/603.out | 2 + > tests/generic/group | 1 + > tests/xfs/260.out | 2 - > tests/xfs/group | 1 - > 5 files changed, 36 insertions(+), 38 deletions(-) > rename tests/{xfs/260 => generic/603} (57%) > create mode 100644 tests/generic/603.out > delete mode 100644 tests/xfs/260.out > > diff --git a/tests/xfs/260 b/tests/generic/603 > similarity index 57% > rename from tests/xfs/260 > rename to tests/generic/603 > index 81b42f01..5dabe447 100755 > --- a/tests/xfs/260 > +++ b/tests/generic/603 > @@ -2,7 +2,7 @@ > # SPDX-License-Identifier: GPL-2.0 > # Copyright (c) 2017 Red Hat Inc. All Rights Reserved. > # > -# FS QA Test 260 > +# FS QA Test 603 > # > # Test per-inode DAX flag by mmap direct/buffered IO. > # > @@ -28,76 +28,80 @@ _cleanup() > # remove previous $seqres.full before test > rm -f $seqres.full > > -_supported_fs xfs > +_supported_fs generic > _supported_os Linux > -_require_scratch_dax_mountopt "dax" > +_require_scratch_dax_mountopt "dax=always" > _require_test_program "feature" > _require_test_program "t_mmap_dio" > _require_dax_iflag > _require_xfs_io_command "falloc" > > -prep_files() > +SRC_DIR=$SCRATCH_MNT/src > +SRC_FILE=$SRC_DIR/tf_s > +DST_DIR=$SCRATCH_MNT/dst > +DST_FILE=$DST_DIR/tf_d > + > +prep_directories() > { > - rm -f $SCRATCH_MNT/tf_{s,d} > + mkdir -p $SRC_DIR $DST_DIR > +} > > +prep_files() > +{ > + rm -f $SRC_FILE $DST_FILE > $XFS_IO_PROG -f -c "falloc 0 $tsize" \ > - $SCRATCH_MNT/tf_{s,d} >> $seqres.full 2>&1 > + $SRC_FILE $DST_FILE >> $seqres.full 2>&1 > } > > t_both_dax() > { > + $XFS_IO_PROG -c "chattr +x" $SRC_DIR $DST_DIR > prep_files > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_{s,d} > # with O_DIRECT first > - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} $1 "dio both dax" > + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ > + $1 "dio both dax" > > prep_files > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_{s,d} > # again with buffered IO > - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ > $1 "buffered both dax" > } > > t_nondax_to_dax() > { > + $XFS_IO_PROG -c "chattr -x" $SRC_DIR > + $XFS_IO_PROG -c "chattr +x" $DST_DIR > prep_files > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_s > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_d > - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ > $1 "dio nondax to dax" > > prep_files > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_s > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_d > - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ > $1 "buffered nondax to dax" > } > > t_dax_to_nondax() > { > + $XFS_IO_PROG -c "chattr +x" $SRC_DIR > + $XFS_IO_PROG -c "chattr -x" $DST_DIR > prep_files > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_s > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_d > - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ > $1 "dio dax to nondax" > > prep_files > - $XFS_IO_PROG -c "chattr +x" $SCRATCH_MNT/tf_s > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_d > - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ > $1 "buffered dax to nondax" > } > > t_both_nondax() > { > + $XFS_IO_PROG -c "chattr -x" $SRC_DIR $DST_DIR > prep_files > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_{s,d} > - $here/src/t_mmap_dio $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio $SRC_FILE $DST_FILE \ > $1 "dio both nondax" > > prep_files > - $XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT/tf_{s,d} > - $here/src/t_mmap_dio -b $SCRATCH_MNT/tf_{s,d} \ > + $here/src/t_mmap_dio -b $SRC_FILE $DST_FILE \ > $1 "buffered both nondax" > } > > @@ -112,6 +116,7 @@ t_dax_flag_mmap_dio() > > do_tests() > { > + prep_directories > # less than page size > t_dax_flag_mmap_dio 1024 > # page size > @@ -124,17 +129,10 @@ do_tests() > # make xfs aligned for PMD fault testing > _scratch_mkfs_geom $(_get_hugepagesize) 1 >> $seqres.full 2>&1 > > -# mount with dax option > -_scratch_mount "-o dax" > - > tsize=$((128 * 1024 * 1024)) > > -do_tests > -_scratch_unmount > - > -# mount again without dax option > -export MOUNT_OPTIONS="" > -_scratch_mount > +# mount with dax=inode option > +_scratch_mount "-o dax=inode" > do_tests > > # success, all done > diff --git a/tests/generic/603.out b/tests/generic/603.out > new file mode 100644 > index 00000000..6810da89 > --- /dev/null > +++ b/tests/generic/603.out > @@ -0,0 +1,2 @@ > +QA output created by 603 > +Silence is golden > diff --git a/tests/generic/group b/tests/generic/group > index d9ab9a31..1d0d5606 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -605,3 +605,4 @@ > 600 auto quick quota > 601 auto quick quota > 602 auto quick encrypt > +603 auto attr quick dax > diff --git a/tests/xfs/260.out b/tests/xfs/260.out > deleted file mode 100644 > index 18ca517c..00000000 > --- a/tests/xfs/260.out > +++ /dev/null > @@ -1,2 +0,0 @@ > -QA output created by 260 > -Silence is golden > diff --git a/tests/xfs/group b/tests/xfs/group > index daf54add..71c30898 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -257,7 +257,6 @@ > 257 auto quick clone > 258 auto quick clone > 259 auto quick > -260 auto attr quick dax > 261 auto quick quota > 262 dangerous_fuzzers dangerous_scrub dangerous_online_repair > 263 auto quick quota > -- > 2.21.0 > > >
On Wed, Jul 15, 2020 at 05:44:53PM +0800, Xiao Yang wrote: > On 2020/7/15 13:39, Xiao Yang wrote: > > On 2020/7/15 10:48, Ira Weiny wrote: > > > On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > > > --- > > > > tests/generic/605 | 199 > > > > ++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/605.out | 2 + > > > > tests/generic/group | 1 + > > > > 3 files changed, 202 insertions(+) > > > > create mode 100644 tests/generic/605 > > > > create mode 100644 tests/generic/605.out > > > > > > > > diff --git a/tests/generic/605 b/tests/generic/605 > > > > new file mode 100644 > > > > index 00000000..6924223a > > > > --- /dev/null > > > > +++ b/tests/generic/605 > > > > @@ -0,0 +1,199 @@ > > > > +#! /bin/bash > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# Copyright (c) 2020 Fujitsu. All Rights Reserved. > > > > +# > > > > +# FS QA Test 605 > > > > +# > > > > +# Verify the inheritance behavior of FS_XFLAG_DAX flag in > > > > various combinations. > > > > +# 1) New files and directories automatically inherit > > > > FS_XFLAG_DAX from their parent directory. > > > > +# 2) cp operation make files and directories inherit the > > > > FS_XFLAG_DAX from new parent directory. > > > > +# 3) mv operation make files and directories preserve the > > > > FS_XFLAG_DAX from old parent directory. > > > > +# In addition, setting/clearing FS_XFLAG_DAX flag is not > > > > impacted by dax mount options. > > > > + > > > > +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 > > > > + > > > > +_supported_fs generic > > > > +_supported_os Linux > > > > +_require_scratch > > > > +_require_dax_iflag > > > > +_require_xfs_io_command "lsattr" "-v" > > > > + > > > > +check_xflag() > > > > +{ > > > > + local target=$1 > > > > + local exp_xflag=$2 > > > > + > > > > + if [ $exp_xflag -eq 0 ]; then > > > > + _test_inode_flag dax $target&& echo "$target has > > > > unexpected FS_XFLAG_DAX flag" > > > > + else > > > > + _test_inode_flag dax $target || echo "$target doen't > > > > have expected FS_XFLAG_DAX flag" > > > > + fi > > > > +} > > > > + > > > > +test_xflag_inheritance1() > > > > +{ > > > > + mkdir -p a > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > + mkdir -p a/b/c > > > > + touch a/b/c/d > > > > + > > > > + check_xflag a 1 > > > > + check_xflag a/b 1 > > > > + check_xflag a/b/c 1 > > > > + check_xflag a/b/c/d 1 > > > > + > > > > + rm -rf a > > > > +} > > > > + > > > > +test_xflag_inheritance2() > > > > +{ > > > > + mkdir -p a/b > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > + mkdir -p a/b/c a/d > > > > + touch a/b/c/e a/d/f > > > > + > > > > + check_xflag a 1 > > > > + check_xflag a/b 0 > > > > + check_xflag a/b/c 0 > > > > + check_xflag a/b/c/e 0 > > > > + check_xflag a/d 1 > > > > + check_xflag a/d/f 1 > > > > + > > > > + rm -rf a > > > > +} > > > > + > > > > +test_xflag_inheritance3() > > > > +{ > > > > + mkdir -p a/b > > > > + $XFS_IO_PROG -c "chattr +x" a/b > > > > + mkdir -p a/b/c a/d > > > > + touch a/b/c/e a/d/f > > > > + > > > > + check_xflag a 0 > > > > + check_xflag a/b 1 > > > > + check_xflag a/b/c 1 > > > > + check_xflag a/b/c/e 1 > > > > + check_xflag a/d 0 > > > > + check_xflag a/d/f 0 > > > > + > > > > + rm -rf a > > > > +} > > > It really seems like 2 and 3 test the same thing? > > Hi Ira, > > > > 2 constructs the following steps: > > 1) a is the parent directory of b > > 2) a doesn't have xflag and b has xflag > > 3) touch many directories/files in a and b > > > > 3 constructs the following steps: > > 1) a is the parent directory of b and b is the parent directory of c > > 2) a and c have xflag, and b doesn't have xflag > > 3) touch many directories/files in b and c > Hi Ira, > > Sorry for misreading your comment, above is the difference between 3 and 4. > The correct one is: > 2 constructs the following steps: > 1) a is the parent directory of b > 2) a has xflag and b doesn't have xflag > 3) touch many directories/files in a and b > > 3 constructs the following steps: > 1) a is the parent directory of b > 2) a doesn't have xflag and b has xflag > 3) touch many directories/files in a and b > > Do you think they are same? I can remove one if you think so. For an earlier version of this series I thought about recommending that each of these functions describe what they aim to test. Then I realized that such descriptions would probably be nearly as long as the function body, and said nothing. But now that Ira's confused, I think that's a stronger argument for each of the test functions having a short description. # If a/ is +x and b/ is -x, check that b's new children don't # inherit +x from a/. test_xflag_inheritance2() {...} Put another way, this adds enough redundancy between the comment and the code that someone else can feel confident that the code still captures the intent of the author. FWIW I think 2 and 3 test opposite variations of the same thing (a's state doesn't somehow override b's), so they're fine. The xfs implementation uses the same inheritance control code for FS_XFLAG_DAX, but doesn't mean everyone else will necessarily do that. --D > Best Regards, > Xiao Yang > > > > Do you think they are same? I can remove one if you think so. > > > > > > + > > > > +test_xflag_inheritance4() > > > > +{ > > > > + mkdir -p a > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > + mkdir -p a/b/c > > > > + $XFS_IO_PROG -c "chattr -x" a/b > > > > + mkdir -p a/b/c/d a/b/e > > > > + touch a/b/c/d/f a/b/e/g > > > > + > > > > + check_xflag a 1 > > > > + check_xflag a/b 0 > > > > + check_xflag a/b/c 1 > > > > + check_xflag a/b/c/d 1 > > > > + check_xflag a/b/c/d/f 1 > > > > + check_xflag a/b/e 0 > > > > + check_xflag a/b/e/g 0 > > > > + > > > > + rm -rf a > > > > +} > > > > + > > > > +test_xflag_inheritance5() > > > > +{ > > > > + mkdir -p a b > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > + mkdir -p a/c a/d b/e b/f > > > > + touch a/g b/h > > > > + > > > > + cp -r a/c b/ > > > > + cp -r b/e a/ > > > > + cp -r a/g b/ > > > > + mv a/d b/ > > > > + mv b/f a/ > > > > + mv b/h a/ > > > > + > > > > + check_xflag b/c 0 > > > > + check_xflag b/d 1 > > > > + check_xflag a/e 1 > > > > + check_xflag a/f 0 > > > > + check_xflag b/g 0 > > > > + check_xflag a/h 0 > > > > + > > > > + rm -rf a b > > > > +} > > > > + > > > > +do_xflag_tests() > > > > +{ > > > > + local option=$1 > > > > + > > > > + _scratch_mount "$option" > > > > + cd $SCRATCH_MNT > > > > + > > > > + for i in $(seq 1 5); do > > > > + test_xflag_inheritance${i} > > > > + done > > > > + > > > > + cd -> /dev/null > > > > + _scratch_unmount > > > > +} > > > > + > > > > +check_dax_mountopt() > > > > +{ > > > > + local option=$1 > > > > + local ret=0 > > > > + > > > > + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 > > > > + > > > > + # Match option name exactly > > > > + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 > > > > + > > > > + _scratch_unmount > > > > + > > > > + return $ret > > > > +} > > > Should this be a common function? > > > > I am not sure if it should be a common function, because it may not be > > used by other tests in future. > > I also consider to merge the function into > > _require_scratch_dax_mountopt(). > > > > > > + > > > > +do_tests() > > > > +{ > > > > + # Mount without dax option > > > > + do_xflag_tests > > > > + > > > > + # Mount with old dax option if fs only supports it. > > > > + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" > > > I don't understand the order here. If we are on an older kernel and > > > the FS > > > only supports '-o dax' the do_xflag_tests will fail won't it? > > > > With both old dax and new dax, the inheritance behavior of FS_XFLAG_DAX > > works well. > > > > > So shouldn't we do this first and bail/'not run' this test if that > > > is the case? > > > > > > I really don't think there is any point in testing the old XFS > > > behavior because > > > the FS_XFLAG_DAX had no effect. So even if it is broken it does not > > > matter. > > > Or perhaps I am missing something here? > > > > This test is designed to verify the inheritance behavior of > > FS_XFLAG_DAX(not related to S_DAX) > > so I think it is fine for both old dax and new dax to run the test. > > > > Best Regards, > > Xiao Yang > > > Ira > > > > > > > + > > > > + # Mount with new dax options if fs supports them. > > > > + if check_dax_mountopt "dax=always"; then > > > > + for dax_option in "dax=always" "dax=inode" "dax=never"; do > > > > + do_xflag_tests "-o $dax_option" > > > > + done > > > > + fi > > > > +} > > > > + > > > > +_scratch_mkfs>> $seqres.full 2>&1 > > > > + > > > > +do_tests > > > > + > > > > +# success, all done > > > > +echo "Silence is golden" > > > > +status=0 > > > > +exit > > > > diff --git a/tests/generic/605.out b/tests/generic/605.out > > > > new file mode 100644 > > > > index 00000000..1ae20049 > > > > --- /dev/null > > > > +++ b/tests/generic/605.out > > > > @@ -0,0 +1,2 @@ > > > > +QA output created by 605 > > > > +Silence is golden > > > > diff --git a/tests/generic/group b/tests/generic/group > > > > index 676e0d2e..a8451862 100644 > > > > --- a/tests/generic/group > > > > +++ b/tests/generic/group > > > > @@ -607,3 +607,4 @@ > > > > 602 auto quick encrypt > > > > 603 auto attr quick dax > > > > 604 auto attr quick dax > > > > +605 auto attr quick dax > > > > -- > > > > 2.21.0 > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > . > > > > >
On 2020/7/16 0:19, Darrick J. Wong wrote: > On Wed, Jul 15, 2020 at 05:44:53PM +0800, Xiao Yang wrote: >> On 2020/7/15 13:39, Xiao Yang wrote: >>> On 2020/7/15 10:48, Ira Weiny wrote: >>>> On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: >>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>>> --- >>>>> tests/generic/605 | 199 >>>>> ++++++++++++++++++++++++++++++++++++++++++ >>>>> tests/generic/605.out | 2 + >>>>> tests/generic/group | 1 + >>>>> 3 files changed, 202 insertions(+) >>>>> create mode 100644 tests/generic/605 >>>>> create mode 100644 tests/generic/605.out >>>>> >>>>> diff --git a/tests/generic/605 b/tests/generic/605 >>>>> new file mode 100644 >>>>> index 00000000..6924223a >>>>> --- /dev/null >>>>> +++ b/tests/generic/605 >>>>> @@ -0,0 +1,199 @@ >>>>> +#! /bin/bash >>>>> +# SPDX-License-Identifier: GPL-2.0 >>>>> +# Copyright (c) 2020 Fujitsu. All Rights Reserved. >>>>> +# >>>>> +# FS QA Test 605 >>>>> +# >>>>> +# Verify the inheritance behavior of FS_XFLAG_DAX flag in >>>>> various combinations. >>>>> +# 1) New files and directories automatically inherit >>>>> FS_XFLAG_DAX from their parent directory. >>>>> +# 2) cp operation make files and directories inherit the >>>>> FS_XFLAG_DAX from new parent directory. >>>>> +# 3) mv operation make files and directories preserve the >>>>> FS_XFLAG_DAX from old parent directory. >>>>> +# In addition, setting/clearing FS_XFLAG_DAX flag is not >>>>> impacted by dax mount options. >>>>> + >>>>> +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 >>>>> + >>>>> +_supported_fs generic >>>>> +_supported_os Linux >>>>> +_require_scratch >>>>> +_require_dax_iflag >>>>> +_require_xfs_io_command "lsattr" "-v" >>>>> + >>>>> +check_xflag() >>>>> +{ >>>>> + local target=$1 >>>>> + local exp_xflag=$2 >>>>> + >>>>> + if [ $exp_xflag -eq 0 ]; then >>>>> + _test_inode_flag dax $target&& echo "$target has >>>>> unexpected FS_XFLAG_DAX flag" >>>>> + else >>>>> + _test_inode_flag dax $target || echo "$target doen't >>>>> have expected FS_XFLAG_DAX flag" >>>>> + fi >>>>> +} >>>>> + >>>>> +test_xflag_inheritance1() >>>>> +{ >>>>> + mkdir -p a >>>>> + $XFS_IO_PROG -c "chattr +x" a >>>>> + mkdir -p a/b/c >>>>> + touch a/b/c/d >>>>> + >>>>> + check_xflag a 1 >>>>> + check_xflag a/b 1 >>>>> + check_xflag a/b/c 1 >>>>> + check_xflag a/b/c/d 1 >>>>> + >>>>> + rm -rf a >>>>> +} >>>>> + >>>>> +test_xflag_inheritance2() >>>>> +{ >>>>> + mkdir -p a/b >>>>> + $XFS_IO_PROG -c "chattr +x" a >>>>> + mkdir -p a/b/c a/d >>>>> + touch a/b/c/e a/d/f >>>>> + >>>>> + check_xflag a 1 >>>>> + check_xflag a/b 0 >>>>> + check_xflag a/b/c 0 >>>>> + check_xflag a/b/c/e 0 >>>>> + check_xflag a/d 1 >>>>> + check_xflag a/d/f 1 >>>>> + >>>>> + rm -rf a >>>>> +} >>>>> + >>>>> +test_xflag_inheritance3() >>>>> +{ >>>>> + mkdir -p a/b >>>>> + $XFS_IO_PROG -c "chattr +x" a/b >>>>> + mkdir -p a/b/c a/d >>>>> + touch a/b/c/e a/d/f >>>>> + >>>>> + check_xflag a 0 >>>>> + check_xflag a/b 1 >>>>> + check_xflag a/b/c 1 >>>>> + check_xflag a/b/c/e 1 >>>>> + check_xflag a/d 0 >>>>> + check_xflag a/d/f 0 >>>>> + >>>>> + rm -rf a >>>>> +} >>>> It really seems like 2 and 3 test the same thing? >>> Hi Ira, >>> >>> 2 constructs the following steps: >>> 1) a is the parent directory of b >>> 2) a doesn't have xflag and b has xflag >>> 3) touch many directories/files in a and b >>> >>> 3 constructs the following steps: >>> 1) a is the parent directory of b and b is the parent directory of c >>> 2) a and c have xflag, and b doesn't have xflag >>> 3) touch many directories/files in b and c >> Hi Ira, >> >> Sorry for misreading your comment, above is the difference between 3 and 4. >> The correct one is: >> 2 constructs the following steps: >> 1) a is the parent directory of b >> 2) a has xflag and b doesn't have xflag >> 3) touch many directories/files in a and b >> >> 3 constructs the following steps: >> 1) a is the parent directory of b >> 2) a doesn't have xflag and b has xflag >> 3) touch many directories/files in a and b >> >> Do you think they are same? I can remove one if you think so. > For an earlier version of this series I thought about recommending that > each of these functions describe what they aim to test. Then I realized > that such descriptions would probably be nearly as long as the function > body, and said nothing. > > But now that Ira's confused, I think that's a stronger argument for each > of the test functions having a short description. > > # If a/ is +x and b/ is -x, check that b's new children don't > # inherit +x from a/. > test_xflag_inheritance2() {...} > > Put another way, this adds enough redundancy between the comment and the > code that someone else can feel confident that the code still captures > the intent of the author. > > FWIW I think 2 and 3 test opposite variations of the same thing (a's > state doesn't somehow override b's), so they're fine. The xfs > implementation uses the same inheritance control code for FS_XFLAG_DAX, > but doesn't mean everyone else will necessarily do that. Hi Darrck, Do you prefer to keep both 2 and 3? right? :-) Thanks, Xiao Yang > --D > >> Best Regards, >> Xiao Yang >>> Do you think they are same? I can remove one if you think so. >>> >>>>> + >>>>> +test_xflag_inheritance4() >>>>> +{ >>>>> + mkdir -p a >>>>> + $XFS_IO_PROG -c "chattr +x" a >>>>> + mkdir -p a/b/c >>>>> + $XFS_IO_PROG -c "chattr -x" a/b >>>>> + mkdir -p a/b/c/d a/b/e >>>>> + touch a/b/c/d/f a/b/e/g >>>>> + >>>>> + check_xflag a 1 >>>>> + check_xflag a/b 0 >>>>> + check_xflag a/b/c 1 >>>>> + check_xflag a/b/c/d 1 >>>>> + check_xflag a/b/c/d/f 1 >>>>> + check_xflag a/b/e 0 >>>>> + check_xflag a/b/e/g 0 >>>>> + >>>>> + rm -rf a >>>>> +} >>>>> + >>>>> +test_xflag_inheritance5() >>>>> +{ >>>>> + mkdir -p a b >>>>> + $XFS_IO_PROG -c "chattr +x" a >>>>> + mkdir -p a/c a/d b/e b/f >>>>> + touch a/g b/h >>>>> + >>>>> + cp -r a/c b/ >>>>> + cp -r b/e a/ >>>>> + cp -r a/g b/ >>>>> + mv a/d b/ >>>>> + mv b/f a/ >>>>> + mv b/h a/ >>>>> + >>>>> + check_xflag b/c 0 >>>>> + check_xflag b/d 1 >>>>> + check_xflag a/e 1 >>>>> + check_xflag a/f 0 >>>>> + check_xflag b/g 0 >>>>> + check_xflag a/h 0 >>>>> + >>>>> + rm -rf a b >>>>> +} >>>>> + >>>>> +do_xflag_tests() >>>>> +{ >>>>> + local option=$1 >>>>> + >>>>> + _scratch_mount "$option" >>>>> + cd $SCRATCH_MNT >>>>> + >>>>> + for i in $(seq 1 5); do >>>>> + test_xflag_inheritance${i} >>>>> + done >>>>> + >>>>> + cd -> /dev/null >>>>> + _scratch_unmount >>>>> +} >>>>> + >>>>> +check_dax_mountopt() >>>>> +{ >>>>> + local option=$1 >>>>> + local ret=0 >>>>> + >>>>> + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 >>>>> + >>>>> + # Match option name exactly >>>>> + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 >>>>> + >>>>> + _scratch_unmount >>>>> + >>>>> + return $ret >>>>> +} >>>> Should this be a common function? >>> I am not sure if it should be a common function, because it may not be >>> used by other tests in future. >>> I also consider to merge the function into >>> _require_scratch_dax_mountopt(). >>> >>>>> + >>>>> +do_tests() >>>>> +{ >>>>> + # Mount without dax option >>>>> + do_xflag_tests >>>>> + >>>>> + # Mount with old dax option if fs only supports it. >>>>> + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" >>>> I don't understand the order here. If we are on an older kernel and >>>> the FS >>>> only supports '-o dax' the do_xflag_tests will fail won't it? >>> With both old dax and new dax, the inheritance behavior of FS_XFLAG_DAX >>> works well. >>> >>>> So shouldn't we do this first and bail/'not run' this test if that >>>> is the case? >>>> >>>> I really don't think there is any point in testing the old XFS >>>> behavior because >>>> the FS_XFLAG_DAX had no effect. So even if it is broken it does not >>>> matter. >>>> Or perhaps I am missing something here? >>> This test is designed to verify the inheritance behavior of >>> FS_XFLAG_DAX(not related to S_DAX) >>> so I think it is fine for both old dax and new dax to run the test. >>> >>> Best Regards, >>> Xiao Yang >>>> Ira >>>> >>>>> + >>>>> + # Mount with new dax options if fs supports them. >>>>> + if check_dax_mountopt "dax=always"; then >>>>> + for dax_option in "dax=always" "dax=inode" "dax=never"; do >>>>> + do_xflag_tests "-o $dax_option" >>>>> + done >>>>> + fi >>>>> +} >>>>> + >>>>> +_scratch_mkfs>> $seqres.full 2>&1 >>>>> + >>>>> +do_tests >>>>> + >>>>> +# success, all done >>>>> +echo "Silence is golden" >>>>> +status=0 >>>>> +exit >>>>> diff --git a/tests/generic/605.out b/tests/generic/605.out >>>>> new file mode 100644 >>>>> index 00000000..1ae20049 >>>>> --- /dev/null >>>>> +++ b/tests/generic/605.out >>>>> @@ -0,0 +1,2 @@ >>>>> +QA output created by 605 >>>>> +Silence is golden >>>>> diff --git a/tests/generic/group b/tests/generic/group >>>>> index 676e0d2e..a8451862 100644 >>>>> --- a/tests/generic/group >>>>> +++ b/tests/generic/group >>>>> @@ -607,3 +607,4 @@ >>>>> 602 auto quick encrypt >>>>> 603 auto attr quick dax >>>>> 604 auto attr quick dax >>>>> +605 auto attr quick dax >>>>> -- >>>>> 2.21.0 >>>>> >>>>> >>>>> >>>> . >>>> >>> >>> >>> . >>> >> >> > > . >
于 2020/7/15 16:10, Xiao Yang 写道: > On 2020/7/15 13:39, Xiao Yang wrote: >> On 2020/7/15 10:48, Ira Weiny wrote: >>> On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>> --- >>>> tests/generic/605 | 199 ++++++++++++++++++++++++++++++++++++++++++ >>>> tests/generic/605.out | 2 + >>>> tests/generic/group | 1 + >>>> 3 files changed, 202 insertions(+) >>>> create mode 100644 tests/generic/605 >>>> create mode 100644 tests/generic/605.out >>>> >>>> diff --git a/tests/generic/605 b/tests/generic/605 >>>> new file mode 100644 >>>> index 00000000..6924223a >>>> --- /dev/null >>>> +++ b/tests/generic/605 >>>> @@ -0,0 +1,199 @@ >>>> +#! /bin/bash >>>> +# SPDX-License-Identifier: GPL-2.0 >>>> +# Copyright (c) 2020 Fujitsu. All Rights Reserved. >>>> +# >>>> +# FS QA Test 605 >>>> +# >>>> +# Verify the inheritance behavior of FS_XFLAG_DAX flag in various >>>> combinations. >>>> +# 1) New files and directories automatically inherit FS_XFLAG_DAX >>>> from their parent directory. >>>> +# 2) cp operation make files and directories inherit the >>>> FS_XFLAG_DAX from new parent directory. >>>> +# 3) mv operation make files and directories preserve the >>>> FS_XFLAG_DAX from old parent directory. >>>> +# In addition, setting/clearing FS_XFLAG_DAX flag is not impacted >>>> by dax mount options. >>>> + >>>> +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 >>>> + >>>> +_supported_fs generic >>>> +_supported_os Linux >>>> +_require_scratch >>>> +_require_dax_iflag >>>> +_require_xfs_io_command "lsattr" "-v" >>>> + >>>> +check_xflag() >>>> +{ >>>> + local target=$1 >>>> + local exp_xflag=$2 >>>> + >>>> + if [ $exp_xflag -eq 0 ]; then >>>> + _test_inode_flag dax $target&& echo "$target has unexpected >>>> FS_XFLAG_DAX flag" >>>> + else >>>> + _test_inode_flag dax $target || echo "$target doen't have >>>> expected FS_XFLAG_DAX flag" >>>> + fi >>>> +} >>>> + >>>> +test_xflag_inheritance1() >>>> +{ >>>> + mkdir -p a >>>> + $XFS_IO_PROG -c "chattr +x" a >>>> + mkdir -p a/b/c >>>> + touch a/b/c/d >>>> + >>>> + check_xflag a 1 >>>> + check_xflag a/b 1 >>>> + check_xflag a/b/c 1 >>>> + check_xflag a/b/c/d 1 >>>> + >>>> + rm -rf a >>>> +} >>>> + >>>> +test_xflag_inheritance2() >>>> +{ >>>> + mkdir -p a/b >>>> + $XFS_IO_PROG -c "chattr +x" a >>>> + mkdir -p a/b/c a/d >>>> + touch a/b/c/e a/d/f >>>> + >>>> + check_xflag a 1 >>>> + check_xflag a/b 0 >>>> + check_xflag a/b/c 0 >>>> + check_xflag a/b/c/e 0 >>>> + check_xflag a/d 1 >>>> + check_xflag a/d/f 1 >>>> + >>>> + rm -rf a >>>> +} >>>> + >>>> +test_xflag_inheritance3() >>>> +{ >>>> + mkdir -p a/b >>>> + $XFS_IO_PROG -c "chattr +x" a/b >>>> + mkdir -p a/b/c a/d >>>> + touch a/b/c/e a/d/f >>>> + >>>> + check_xflag a 0 >>>> + check_xflag a/b 1 >>>> + check_xflag a/b/c 1 >>>> + check_xflag a/b/c/e 1 >>>> + check_xflag a/d 0 >>>> + check_xflag a/d/f 0 >>>> + >>>> + rm -rf a >>>> +} >>> It really seems like 2 and 3 test the same thing? >> Hi Ira, >> >> 2 constructs the following steps: >> 1) a is the parent directory of b >> 2) a doesn't have xflag and b has xflag >> 3) touch many directories/files in a and b >> >> 3 constructs the following steps: >> 1) a is the parent directory of b and b is the parent directory of c >> 2) a and c have xflag, and b doesn't have xflag >> 3) touch many directories/files in b and c >> >> Do you think they are same? I can remove one if you think so. >> >>>> + >>>> +test_xflag_inheritance4() >>>> +{ >>>> + mkdir -p a >>>> + $XFS_IO_PROG -c "chattr +x" a >>>> + mkdir -p a/b/c >>>> + $XFS_IO_PROG -c "chattr -x" a/b >>>> + mkdir -p a/b/c/d a/b/e >>>> + touch a/b/c/d/f a/b/e/g >>>> + >>>> + check_xflag a 1 >>>> + check_xflag a/b 0 >>>> + check_xflag a/b/c 1 >>>> + check_xflag a/b/c/d 1 >>>> + check_xflag a/b/c/d/f 1 >>>> + check_xflag a/b/e 0 >>>> + check_xflag a/b/e/g 0 >>>> + >>>> + rm -rf a >>>> +} >>>> + >>>> +test_xflag_inheritance5() >>>> +{ >>>> + mkdir -p a b >>>> + $XFS_IO_PROG -c "chattr +x" a >>>> + mkdir -p a/c a/d b/e b/f >>>> + touch a/g b/h >>>> + >>>> + cp -r a/c b/ >>>> + cp -r b/e a/ >>>> + cp -r a/g b/ >>>> + mv a/d b/ >>>> + mv b/f a/ >>>> + mv b/h a/ >>>> + >>>> + check_xflag b/c 0 >>>> + check_xflag b/d 1 >>>> + check_xflag a/e 1 >>>> + check_xflag a/f 0 >>>> + check_xflag b/g 0 >>>> + check_xflag a/h 0 >>>> + >>>> + rm -rf a b >>>> +} >>>> + >>>> +do_xflag_tests() >>>> +{ >>>> + local option=$1 >>>> + >>>> + _scratch_mount "$option" >>>> + cd $SCRATCH_MNT >>>> + >>>> + for i in $(seq 1 5); do >>>> + test_xflag_inheritance${i} >>>> + done >>>> + >>>> + cd -> /dev/null >>>> + _scratch_unmount >>>> +} >>>> + >>>> +check_dax_mountopt() >>>> +{ >>>> + local option=$1 >>>> + local ret=0 >>>> + >>>> + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 >>>> + >>>> + # Match option name exactly >>>> + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 >>>> + >>>> + _scratch_unmount >>>> + >>>> + return $ret >>>> +} >>> Should this be a common function? >> >> I am not sure if it should be a common function, because it may not >> be used by other tests in future. >> I also consider to merge the function into >> _require_scratch_dax_mountopt(). > For this comment, I try to merge the function into > _require_scratch_dax_mountopt(), as below: > ---------------------------------------------------------------------------------------------------------------- > > +# Only accept dax/dax=always mount option becasue dax=always, dax=inode > +# and dax=never are always introduced together. > +# Return 0 if filesystem/device supports the specified dax option. > +# Return 1 if mount fails with the specified dax option. > +# Return 2 if /proc/mounts shows wrong dax option. > +# Check new dax=inode, dax=always or dax=never option by passing > "dax=always". > +# Check old dax or new dax=always by passing "dax". > +_check_scratch_dax_mountopt() > +{ > + local option=$1 > + > + echo "$option" | egrep -q "dax(=always|$)" || \ > + _notrun "invalid $option, only accept dax/dax=always" > + > + _require_scratch > + _scratch_mkfs > /dev/null 2>&1 > + > + _try_scratch_mount "-o $option" > /dev/null 2>&1 || return 1 > + > + if _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)"; then > + _scratch_unmount > + return 0 > + else > + _scratch_unmount > + return 2 > + fi > +} > + > +# Throw notrun if _check_scratch_dax_mountopt() returns a non-zero > value. > +_require_scratch_dax_mountopt() > +{ > + local mountopt=$1 > + > + _check_scratch_dax_mountopt "$mountopt" > + local res=$? > + > + [ $res -eq 1 ] && _notrun "mount $SCRATCH_DEV with $mountopt failed" > + [ $res -eq 2 ] && _notrun "$SCRATCH_DEV $FSTYP does not support -o > $mountopt" > +} > ----------------------------------------------------------------------------------------------------------- > Hi Darrick, How about this change? 1) Introduce _check_scratch_dax_mountopt() to check dax option. (return 0 if dax option is supported, return 1 if mount fail or return 2 if /proc/mounts show wrong info) 2) _require_scratch_dax_mountopt() calls Introduce _check_scratch_dax_mountopt() and throws notrun if check_scratch_dax_mountopt() returns a non-zero value. >> >>>> + >>>> +do_tests() >>>> +{ >>>> + # Mount without dax option >>>> + do_xflag_tests >>>> + >>>> + # Mount with old dax option if fs only supports it. >>>> + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" >>> I don't understand the order here. If we are on an older kernel and >>> the FS >>> only supports '-o dax' the do_xflag_tests will fail won't it? >> >> With both old dax and new dax, the inheritance behavior of >> FS_XFLAG_DAX works well. >> >>> So shouldn't we do this first and bail/'not run' this test if that >>> is the case? >>> >>> I really don't think there is any point in testing the old XFS >>> behavior because >>> the FS_XFLAG_DAX had no effect. So even if it is broken it does not >>> matter. >>> Or perhaps I am missing something here? >> >> This test is designed to verify the inheritance behavior of >> FS_XFLAG_DAX(not related to S_DAX) >> so I think it is fine for both old dax and new dax to run the test. > For this comment, I try to update it, as below: > ------------------------------------------------------------------------- > +do_tests() > +{ > + # Mount without dax option > + do_xflag_tests > + > + # Mount with 'dax' or 'dax=always' option if fs supports it. > + _check_scratch_dax_mountopt "dax" && do_xflag_tests "-o dax" > + > + # Mount with 'dax=inode' and 'dax=never' options if fs supports them. > + if _check_scratch_dax_mountopt "dax=always"; then > + for dax_option in "dax=inode" "dax=never"; do > + do_xflag_tests "-o $dax_option" > + done > + fi > +} > ------------------------------------------------------------------------- After the implemention of _check_scratch_dax_mountopt(), we can use it here. Thanks, Xiao Yang >> >> Best Regards, >> Xiao Yang >>> Ira >>> >>>> + >>>> + # Mount with new dax options if fs supports them. >>>> + if check_dax_mountopt "dax=always"; then >>>> + for dax_option in "dax=always" "dax=inode" "dax=never"; do >>>> + do_xflag_tests "-o $dax_option" >>>> + done >>>> + fi >>>> +} >>>> + >>>> +_scratch_mkfs>> $seqres.full 2>&1 >>>> + >>>> +do_tests >>>> + >>>> +# success, all done >>>> +echo "Silence is golden" >>>> +status=0 >>>> +exit >>>> diff --git a/tests/generic/605.out b/tests/generic/605.out >>>> new file mode 100644 >>>> index 00000000..1ae20049 >>>> --- /dev/null >>>> +++ b/tests/generic/605.out >>>> @@ -0,0 +1,2 @@ >>>> +QA output created by 605 >>>> +Silence is golden >>>> diff --git a/tests/generic/group b/tests/generic/group >>>> index 676e0d2e..a8451862 100644 >>>> --- a/tests/generic/group >>>> +++ b/tests/generic/group >>>> @@ -607,3 +607,4 @@ >>>> 602 auto quick encrypt >>>> 603 auto attr quick dax >>>> 604 auto attr quick dax >>>> +605 auto attr quick dax >>>> -- >>>> 2.21.0 >>>> >>>> >>>> >>> >>> . >>> >> >> >> >> . >> > > > > . >
On Wed, Jul 15, 2020 at 08:56:31AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 15, 2020 at 01:55:14PM +0800, Xiao Yang wrote:
> > On 2020/7/15 12:15, Ira Weiny wrote:
> > > On Wed, Jul 15, 2020 at 11:19:58AM +0800, Xiao Yang wrote:
> > > > On 2020/7/15 9:59, Ira Weiny wrote:
> > > > > On Tue, Jul 14, 2020 at 05:40:03PM +0800, Xiao Yang wrote:
> > > > > > 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option
> > > > > > 2) _require_dax_iflag() checks FS_XFLAG_DAX
> > > > > >
> > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > > > Reviewed-by: Ira Weiny<ira.weiny@intel.com>
> > > > Hi Ira,
> > > >
> > > > Do you want to check if invalid parameter is passed to
> > > > _require_scratch_dax_mountopt? Thought
> > > > I think it is not necessary(user should pass correct parament).
> > > > Like this:
> > > > [ "$mountopt" != "dax" ]&& [ "$mountopt" != "dax=always" ]&& _notrun
> > > > "$mountopt is invalid"
> > > I thought this patch was just checking if the filesystem/device supported DAX
> > > such that DAX tests could run using either old _or_ new dax support...
> > > But after thinking about it I think we need something more complicated.
> > >
> > > Something like:
> > >
> > > if ('-o dax=inode')
> > > // FS is DAX with per file support (ie ext4 || XFS kernel>= 5.8)
> > Hi Ira,
> >
> > dax=inode/dax=never cannot check if underlying device supports dax, so I
> > perfer to use dax=always
> > For example:
> > XFS:
> > # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/
> > # mount | grep sda8
> > /dev/sda8 on /mnt/xfstests/test type xfs
> > (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> > ...
> > # mount -o dax=always /dev/sda8 /mnt/xfstests/test/
> > # mount | grep sda8
> > /dev/sda8 on /mnt/xfstests/test type xfs
> > (rw,relatime,seclabel,attr2,dax=never,inode64,logbufs=8,logbsize=32k,noquota)
> >
> > EXT4:
> > # mount -o dax=inode /dev/sda8 /mnt/xfstests/test/
> > # mount | grep sda8
> > /dev/sda8 on /mnt/xfstests/test type ext4 (rw,relatime,seclabel,dax=inode)
> > ...
> > # mount -o dax=always /dev/sda8 /mnt/xfstests/test/
> > mount: /mnt/xfstests/test: wrong fs type, bad option, bad superblock on
> > /dev/sda8, missing codepage or helper program, or other error.
> > # dmesg | grep DAX
> > [597988.165120] EXT4-fs (sda8): DAX unsupported by block device.
> >
> > If we want to test new dax(i.e. dax=inode, dax=always or dax=never), so pass
> > "dax=always" to _require_scratch_dax_mountopt
> > If we want to test old dax or new dax=always(either of them), so pass "dax"
> > to _require_scratch_dax_mountopt
>
> That does seem like the only way to figure out if a fs/device
> combination supports dax.
>
> The valid arguments to the _require_scratch_dax* helpers are worth
> mentioning in a comment above the function, but I suspect that adding
> validation code is probably overkill.
Ok yes I see it now.
You are both correct. I think a comment which indicates that one should pads
'-o dax' to check for 'old support' vs '-o dax=always' for 'new support' and
the test chooses which option to pass (based on the support it needs) to
_require_scratch_dax_mountopt()...
Sorry I was not completely following before!
Ira
On Thu, Jul 16, 2020 at 12:33:31AM +0800, Xiao Yang wrote: > On 2020/7/16 0:19, Darrick J. Wong wrote: > > On Wed, Jul 15, 2020 at 05:44:53PM +0800, Xiao Yang wrote: > > > On 2020/7/15 13:39, Xiao Yang wrote: > > > > On 2020/7/15 10:48, Ira Weiny wrote: > > > > > On Tue, Jul 14, 2020 at 05:40:09PM +0800, Xiao Yang wrote: > > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > > > > > --- > > > > > > tests/generic/605 | 199 > > > > > > ++++++++++++++++++++++++++++++++++++++++++ > > > > > > tests/generic/605.out | 2 + > > > > > > tests/generic/group | 1 + > > > > > > 3 files changed, 202 insertions(+) > > > > > > create mode 100644 tests/generic/605 > > > > > > create mode 100644 tests/generic/605.out > > > > > > > > > > > > diff --git a/tests/generic/605 b/tests/generic/605 > > > > > > new file mode 100644 > > > > > > index 00000000..6924223a > > > > > > --- /dev/null > > > > > > +++ b/tests/generic/605 > > > > > > @@ -0,0 +1,199 @@ > > > > > > +#! /bin/bash > > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > > +# Copyright (c) 2020 Fujitsu. All Rights Reserved. > > > > > > +# > > > > > > +# FS QA Test 605 > > > > > > +# > > > > > > +# Verify the inheritance behavior of FS_XFLAG_DAX flag in > > > > > > various combinations. > > > > > > +# 1) New files and directories automatically inherit > > > > > > FS_XFLAG_DAX from their parent directory. > > > > > > +# 2) cp operation make files and directories inherit the > > > > > > FS_XFLAG_DAX from new parent directory. > > > > > > +# 3) mv operation make files and directories preserve the > > > > > > FS_XFLAG_DAX from old parent directory. > > > > > > +# In addition, setting/clearing FS_XFLAG_DAX flag is not > > > > > > impacted by dax mount options. > > > > > > + > > > > > > +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 > > > > > > + > > > > > > +_supported_fs generic > > > > > > +_supported_os Linux > > > > > > +_require_scratch > > > > > > +_require_dax_iflag > > > > > > +_require_xfs_io_command "lsattr" "-v" > > > > > > + > > > > > > +check_xflag() > > > > > > +{ > > > > > > + local target=$1 > > > > > > + local exp_xflag=$2 > > > > > > + > > > > > > + if [ $exp_xflag -eq 0 ]; then > > > > > > + _test_inode_flag dax $target&& echo "$target has > > > > > > unexpected FS_XFLAG_DAX flag" > > > > > > + else > > > > > > + _test_inode_flag dax $target || echo "$target doen't > > > > > > have expected FS_XFLAG_DAX flag" > > > > > > + fi > > > > > > +} > > > > > > + > > > > > > +test_xflag_inheritance1() > > > > > > +{ > > > > > > + mkdir -p a > > > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > > > + mkdir -p a/b/c > > > > > > + touch a/b/c/d > > > > > > + > > > > > > + check_xflag a 1 > > > > > > + check_xflag a/b 1 > > > > > > + check_xflag a/b/c 1 > > > > > > + check_xflag a/b/c/d 1 > > > > > > + > > > > > > + rm -rf a > > > > > > +} > > > > > > + > > > > > > +test_xflag_inheritance2() > > > > > > +{ > > > > > > + mkdir -p a/b > > > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > > > + mkdir -p a/b/c a/d > > > > > > + touch a/b/c/e a/d/f > > > > > > + > > > > > > + check_xflag a 1 > > > > > > + check_xflag a/b 0 > > > > > > + check_xflag a/b/c 0 > > > > > > + check_xflag a/b/c/e 0 > > > > > > + check_xflag a/d 1 > > > > > > + check_xflag a/d/f 1 > > > > > > + > > > > > > + rm -rf a > > > > > > +} > > > > > > + > > > > > > +test_xflag_inheritance3() > > > > > > +{ > > > > > > + mkdir -p a/b > > > > > > + $XFS_IO_PROG -c "chattr +x" a/b > > > > > > + mkdir -p a/b/c a/d > > > > > > + touch a/b/c/e a/d/f > > > > > > + > > > > > > + check_xflag a 0 > > > > > > + check_xflag a/b 1 > > > > > > + check_xflag a/b/c 1 > > > > > > + check_xflag a/b/c/e 1 > > > > > > + check_xflag a/d 0 > > > > > > + check_xflag a/d/f 0 > > > > > > + > > > > > > + rm -rf a > > > > > > +} > > > > > It really seems like 2 and 3 test the same thing? > > > > Hi Ira, > > > > > > > > 2 constructs the following steps: > > > > 1) a is the parent directory of b > > > > 2) a doesn't have xflag and b has xflag > > > > 3) touch many directories/files in a and b > > > > > > > > 3 constructs the following steps: > > > > 1) a is the parent directory of b and b is the parent directory of c > > > > 2) a and c have xflag, and b doesn't have xflag > > > > 3) touch many directories/files in b and c > > > Hi Ira, > > > > > > Sorry for misreading your comment, above is the difference between 3 and 4. > > > The correct one is: > > > 2 constructs the following steps: > > > 1) a is the parent directory of b > > > 2) a has xflag and b doesn't have xflag > > > 3) touch many directories/files in a and b > > > > > > 3 constructs the following steps: > > > 1) a is the parent directory of b > > > 2) a doesn't have xflag and b has xflag > > > 3) touch many directories/files in a and b > > > > > > Do you think they are same? I can remove one if you think so. > > For an earlier version of this series I thought about recommending that > > each of these functions describe what they aim to test. Then I realized > > that such descriptions would probably be nearly as long as the function > > body, and said nothing. > > > > But now that Ira's confused, I think that's a stronger argument for each > > of the test functions having a short description. > > > > # If a/ is +x and b/ is -x, check that b's new children don't > > # inherit +x from a/. > > test_xflag_inheritance2() {...} > > > > Put another way, this adds enough redundancy between the comment and the > > code that someone else can feel confident that the code still captures > > the intent of the author. > > > > FWIW I think 2 and 3 test opposite variations of the same thing (a's > > state doesn't somehow override b's), so they're fine. The xfs > > implementation uses the same inheritance control code for FS_XFLAG_DAX, > > but doesn't mean everyone else will necessarily do that. > Hi Darrck, > > Do you prefer to keep both 2 and 3? right? :-) My point was more about the fact that I don't think 2 and 3 actually exercising any additional code paths within the kernel. But looking at it this morning (rather than late last night) I could see where changes to the kernel logic may introduce some issue in the future so we have the test and we should leave it! :-D Ira > > Thanks, > Xiao Yang > > --D > > > > > Best Regards, > > > Xiao Yang > > > > Do you think they are same? I can remove one if you think so. > > > > > > > > > > + > > > > > > +test_xflag_inheritance4() > > > > > > +{ > > > > > > + mkdir -p a > > > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > > > + mkdir -p a/b/c > > > > > > + $XFS_IO_PROG -c "chattr -x" a/b > > > > > > + mkdir -p a/b/c/d a/b/e > > > > > > + touch a/b/c/d/f a/b/e/g > > > > > > + > > > > > > + check_xflag a 1 > > > > > > + check_xflag a/b 0 > > > > > > + check_xflag a/b/c 1 > > > > > > + check_xflag a/b/c/d 1 > > > > > > + check_xflag a/b/c/d/f 1 > > > > > > + check_xflag a/b/e 0 > > > > > > + check_xflag a/b/e/g 0 > > > > > > + > > > > > > + rm -rf a > > > > > > +} > > > > > > + > > > > > > +test_xflag_inheritance5() > > > > > > +{ > > > > > > + mkdir -p a b > > > > > > + $XFS_IO_PROG -c "chattr +x" a > > > > > > + mkdir -p a/c a/d b/e b/f > > > > > > + touch a/g b/h > > > > > > + > > > > > > + cp -r a/c b/ > > > > > > + cp -r b/e a/ > > > > > > + cp -r a/g b/ > > > > > > + mv a/d b/ > > > > > > + mv b/f a/ > > > > > > + mv b/h a/ > > > > > > + > > > > > > + check_xflag b/c 0 > > > > > > + check_xflag b/d 1 > > > > > > + check_xflag a/e 1 > > > > > > + check_xflag a/f 0 > > > > > > + check_xflag b/g 0 > > > > > > + check_xflag a/h 0 > > > > > > + > > > > > > + rm -rf a b > > > > > > +} > > > > > > + > > > > > > +do_xflag_tests() > > > > > > +{ > > > > > > + local option=$1 > > > > > > + > > > > > > + _scratch_mount "$option" > > > > > > + cd $SCRATCH_MNT > > > > > > + > > > > > > + for i in $(seq 1 5); do > > > > > > + test_xflag_inheritance${i} > > > > > > + done > > > > > > + > > > > > > + cd -> /dev/null > > > > > > + _scratch_unmount > > > > > > +} > > > > > > + > > > > > > +check_dax_mountopt() > > > > > > +{ > > > > > > + local option=$1 > > > > > > + local ret=0 > > > > > > + > > > > > > + _try_scratch_mount "-o $option">> $seqres.full 2>&1 || return 1 > > > > > > + > > > > > > + # Match option name exactly > > > > > > + _fs_options $SCRATCH_DEV | egrep -q "$option(,|$)" || ret=1 > > > > > > + > > > > > > + _scratch_unmount > > > > > > + > > > > > > + return $ret > > > > > > +} > > > > > Should this be a common function? > > > > I am not sure if it should be a common function, because it may not be > > > > used by other tests in future. > > > > I also consider to merge the function into > > > > _require_scratch_dax_mountopt(). > > > > > > > > > > + > > > > > > +do_tests() > > > > > > +{ > > > > > > + # Mount without dax option > > > > > > + do_xflag_tests > > > > > > + > > > > > > + # Mount with old dax option if fs only supports it. > > > > > > + check_dax_mountopt "dax"&& do_xflag_tests "-o dax" > > > > > I don't understand the order here. If we are on an older kernel and > > > > > the FS > > > > > only supports '-o dax' the do_xflag_tests will fail won't it? > > > > With both old dax and new dax, the inheritance behavior of FS_XFLAG_DAX > > > > works well. > > > > > > > > > So shouldn't we do this first and bail/'not run' this test if that > > > > > is the case? > > > > > > > > > > I really don't think there is any point in testing the old XFS > > > > > behavior because > > > > > the FS_XFLAG_DAX had no effect. So even if it is broken it does not > > > > > matter. > > > > > Or perhaps I am missing something here? > > > > This test is designed to verify the inheritance behavior of > > > > FS_XFLAG_DAX(not related to S_DAX) > > > > so I think it is fine for both old dax and new dax to run the test. > > > > > > > > Best Regards, > > > > Xiao Yang > > > > > Ira > > > > > > > > > > > + > > > > > > + # Mount with new dax options if fs supports them. > > > > > > + if check_dax_mountopt "dax=always"; then > > > > > > + for dax_option in "dax=always" "dax=inode" "dax=never"; do > > > > > > + do_xflag_tests "-o $dax_option" > > > > > > + done > > > > > > + fi > > > > > > +} > > > > > > + > > > > > > +_scratch_mkfs>> $seqres.full 2>&1 > > > > > > + > > > > > > +do_tests > > > > > > + > > > > > > +# success, all done > > > > > > +echo "Silence is golden" > > > > > > +status=0 > > > > > > +exit > > > > > > diff --git a/tests/generic/605.out b/tests/generic/605.out > > > > > > new file mode 100644 > > > > > > index 00000000..1ae20049 > > > > > > --- /dev/null > > > > > > +++ b/tests/generic/605.out > > > > > > @@ -0,0 +1,2 @@ > > > > > > +QA output created by 605 > > > > > > +Silence is golden > > > > > > diff --git a/tests/generic/group b/tests/generic/group > > > > > > index 676e0d2e..a8451862 100644 > > > > > > --- a/tests/generic/group > > > > > > +++ b/tests/generic/group > > > > > > @@ -607,3 +607,4 @@ > > > > > > 602 auto quick encrypt > > > > > > 603 auto attr quick dax > > > > > > 604 auto attr quick dax > > > > > > +605 auto attr quick dax > > > > > > -- > > > > > > 2.21.0 > > > > > > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > >
于 2020/7/16 0:07, Darrick J. Wong 写道: > On Wed, Jul 15, 2020 at 11:12:36AM +0800, Xiao Yang wrote: >> On 2020/7/15 10:31, Ira Weiny wrote: >>> On Tue, Jul 14, 2020 at 05:40:05PM +0800, Xiao Yang wrote: >>>> ext4 can accept the last one if the same mkfs options are passed but xfs cannot >>> I'm having trouble parsing this commit message. What does 'last one' refer to? >>> >>>> accept the same mkfs options and reports "xxx option is respecified" error. >>> Ok I think I understand now. Some FS's (XFS) do not accept an option more than >>> once. So we can't just blindly add options to the end of the MKFS_OPTIONS. Is >>> that correct? >> Hi Ira, >> >> Correct. >>>> I >>>> prefer to override the same mkfs option which is defined in MKFS_OPTION so that >>>> we can have a chance to pass other mkfs options to _scratch_mkfs_geom(). >>> Instead the patch parses the current option string and replaces the value if >>> the option is already there. This allows us to specify MKFS_OPTIONS to >>> generic/223. >> Right. :-) >> >> XFS_MKFS_OPTIONS/MKFS_OPTIONS can be used to specify some custom options by user, >> so I don't want to clear it blindly. >> >> Thanks, >> Xiao Yang >>> I think the code is reasonable although my sed skills are not good enough to >>> tell for sure... ;-) >>> >>> Ira >>> >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>> --- >>>> common/rc | 14 +++++++++++++- >>>> tests/generic/223 | 1 - >>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index 6c908f2e..567cf83b 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -1051,7 +1051,19 @@ _scratch_mkfs_geom() >>>> >>>> case $FSTYP in >>>> xfs) >>>> - MKFS_OPTIONS+=" -b size=$blocksize, -d su=$sunit_bytes,sw=$swidth_mult" >>>> + if echo "$MKFS_OPTIONS" | egrep -q "b?size="; then >>>> + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r "s/(b?size=)[0-9]+/\1$blocksize/") >>>> + else >>>> + MKFS_OPTIONS+=" -b size=$blocksize" >>>> + fi >>>> + >>>> + if echo "$MKFS_OPTIONS" | egrep -q "(su|sunit|sw|swidth)="; then >>>> + MKFS_OPTIONS=$(echo "$MKFS_OPTIONS" | sed -r \ >>>> + -e "s/(su|sunit)=[0-9kmg]+/su=$sunit_bytes/" \ >>>> + -e "s/(sw|swidth)=[0-9kmg]+/sw=$swidth_mult/") >>>> + else >>>> + MKFS_OPTIONS+=" -d su=$sunit_bytes,sw=$swidth_mult" >>>> + fi > ...ok, I see, this makes the function smart enough to substitute > geometry parameters instead of dumping them on the end and letting that > blow up. Heh, ok, that's definitely a weird quirk I've noticed. > >>>> ;; >>>> ext4|ext4dev) >>>> MKFS_OPTIONS+=" -b $blocksize -E stride=$sunit_blocks,stripe_width=$swidth_blocks" >>>> diff --git a/tests/generic/223 b/tests/generic/223 >>>> index 6cfd00dd..ba7c9a44 100755 >>>> --- a/tests/generic/223 >>>> +++ b/tests/generic/223 >>>> @@ -41,7 +41,6 @@ for SUNIT_K in 8 16 32 64 128; do >>>> let SUNIT_BLOCKS=$SUNIT_BYTES/$BLOCKSIZE >>>> >>>> echo "=== mkfs with su $SUNIT_BLOCKS blocks x 4 ===" >>>> - export MKFS_OPTIONS="" > So I guess you're deleting this so that the test runs with whatever > MKFS_OPTIONS the test runner specified, while letting the test edit > blocksize and stripe parameters? Hi Darrick, Right :-) . My change wants to achieve it. Thanks, Xiao Yang > Proving I'm still bad at remembering to read commit messages, > Reviewed-by: Darrick J. Wong<darrick.wong@oracle.com> > > --D > >>>> _scratch_mkfs_geom $SUNIT_BYTES 4 $BLOCKSIZE>> $seqres.full 2>&1 >>>> _scratch_mount >>>> >>>> -- >>>> 2.21.0 >>>> >>>> >>>> >>> . >>> >> >> > > . >