| Message ID | 20200413054419.1560503-1-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs/XXX: Add xfs/XXX | expand |
On Mon, Apr 13, 2020 at 9:06 AM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > Add XXX to test per file DAX operations. Please change commit title to "xfs: Add test for per file DAX operations" The title Add xfs/XXX is not useful even if XXX where a number. But the kernel patch suggests that there is an intention to make this behavior also applicable to ext4?? If that is the case I would recommend making this a generic tests which requires filesystem support for -o dax=XXX > > The following is tested[*] > > 1. There exists an in-kernel access mode flag S_DAX that is set when > file accesses go directly to persistent memory, bypassing the page > cache. Applications must call statx to discover the current S_DAX > state (STATX_ATTR_DAX). > > 2. There exists an advisory file inode flag FS_XFLAG_DAX that is > inherited from the parent directory FS_XFLAG_DAX inode flag at file > creation time. This advisory flag can be set or cleared at any > time, but doing so does not immediately affect the S_DAX state. > > Unless overridden by mount options (see (3)), if FS_XFLAG_DAX is set > and the fs is on pmem then it will enable S_DAX at inode load time; > if FS_XFLAG_DAX is not set, it will not enable S_DAX. > > 3. There exists a dax= mount option. > > "-o dax=never" means "never set S_DAX, ignore FS_XFLAG_DAX." > > "-o dax=always" means "always set S_DAX (at least on pmem), > and ignore FS_XFLAG_DAX." > > "-o dax" is an alias for "dax=always". > > "-o dax=inode" means "follow FS_XFLAG_DAX" and is the default. > > 4. There exists an advisory directory inode flag FS_XFLAG_DAX that can > be set or cleared at any time. The flag state is copied into any > files or subdirectories when they are created within that directory. > > 5. Programs that require a specific file access mode (DAX or not DAX) > can do one of the following: > > (a) Create files in directories that the FS_XFLAG_DAX flag set as > needed; or > > (b) Have the administrator set an override via mount option; or > > (c) Set or clear the file's FS_XFLAG_DAX flag as needed. Programs > must then cause the kernel to evict the inode from memory. This > can be done by: > > i> Closing the file and re-opening the file and using statx to > see if the fs has changed the S_DAX flag; and > > ii> If the file still does not have the desired S_DAX access > mode, either unmount and remount the filesystem, or close > the file and use drop_caches. > > 6. It's not unreasonable that users who want to squeeze every last bit > of performance out of the particular rough and tumble bits of their > storage also be exposed to the difficulties of what happens when the > operating system can't totally virtualize those hardware > capabilities. Your high performance sports car is not a Toyota > minivan, as it were. > > [*] https://lore.kernel.org/lkml/20200409165927.GD6741@magnolia/ > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes from v6 (kernel patch set): > Start versions tracking the kernel patch set. > Update for new requirements > > Changes from V1 (xfstests patch): > Add test to ensure moved files preserve their flag > Check chattr of non-dax flags (check bug found by Darrick) > --- ... > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -511,3 +511,4 @@ > 511 auto quick quota > 512 auto quick acl attr > 513 auto mount > +999 auto The test looks also 'quick' Thanks, Amir.
On Mon, Apr 13, 2020 at 10:54:36AM +0300, Amir Goldstein wrote: > On Mon, Apr 13, 2020 at 9:06 AM <ira.weiny@intel.com> wrote: > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > Add XXX to test per file DAX operations. > > Please change commit title to "xfs: Add test for per file DAX operations" > The title Add xfs/XXX is not useful even if XXX where a number. Thanks and... yes this really should have been marked RFC as the requirements have been in flux and I never meant for this to be merged until the kernel patches are merged. Hence keeping the number XXX. RFC would have been better. I've update v8. thanks! > > But the kernel patch suggests that there is an intention to make > this behavior also applicable to ext4?? > If that is the case I would recommend making this a generic tests > which requires filesystem support for -o dax=XXX I have a patch set for ext4 which is not quite passing this. I'm not sure what is going on yet. Once that is working I was going to move this to generic. (The documentation in the kernel patch set also reflects ext4 being different from xfs for the time being.) This is mainly because I'm not sure if ext4 will make 5.8 or not. Would you prefer making this generic now? I assume there is some way to mark generic tests for a subset of FS's? I have not figured that out yet. > > > > > The following is tested[*] > > > > 1. There exists an in-kernel access mode flag S_DAX that is set when > > file accesses go directly to persistent memory, bypassing the page > > cache. Applications must call statx to discover the current S_DAX > > state (STATX_ATTR_DAX). > > > > 2. There exists an advisory file inode flag FS_XFLAG_DAX that is > > inherited from the parent directory FS_XFLAG_DAX inode flag at file > > creation time. This advisory flag can be set or cleared at any > > time, but doing so does not immediately affect the S_DAX state. > > > > Unless overridden by mount options (see (3)), if FS_XFLAG_DAX is set > > and the fs is on pmem then it will enable S_DAX at inode load time; > > if FS_XFLAG_DAX is not set, it will not enable S_DAX. > > > > 3. There exists a dax= mount option. > > > > "-o dax=never" means "never set S_DAX, ignore FS_XFLAG_DAX." > > > > "-o dax=always" means "always set S_DAX (at least on pmem), > > and ignore FS_XFLAG_DAX." > > > > "-o dax" is an alias for "dax=always". > > > > "-o dax=inode" means "follow FS_XFLAG_DAX" and is the default. > > > > 4. There exists an advisory directory inode flag FS_XFLAG_DAX that can > > be set or cleared at any time. The flag state is copied into any > > files or subdirectories when they are created within that directory. > > > > 5. Programs that require a specific file access mode (DAX or not DAX) > > can do one of the following: > > > > (a) Create files in directories that the FS_XFLAG_DAX flag set as > > needed; or > > > > (b) Have the administrator set an override via mount option; or > > > > (c) Set or clear the file's FS_XFLAG_DAX flag as needed. Programs > > must then cause the kernel to evict the inode from memory. This > > can be done by: > > > > i> Closing the file and re-opening the file and using statx to > > see if the fs has changed the S_DAX flag; and > > > > ii> If the file still does not have the desired S_DAX access > > mode, either unmount and remount the filesystem, or close > > the file and use drop_caches. > > > > 6. It's not unreasonable that users who want to squeeze every last bit > > of performance out of the particular rough and tumble bits of their > > storage also be exposed to the difficulties of what happens when the > > operating system can't totally virtualize those hardware > > capabilities. Your high performance sports car is not a Toyota > > minivan, as it were. > > > > [*] https://lore.kernel.org/lkml/20200409165927.GD6741@magnolia/ > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > --- > > Changes from v6 (kernel patch set): > > Start versions tracking the kernel patch set. > > Update for new requirements > > > > Changes from V1 (xfstests patch): > > Add test to ensure moved files preserve their flag > > Check chattr of non-dax flags (check bug found by Darrick) > > --- > ... > > --- a/tests/xfs/group > > +++ b/tests/xfs/group > > @@ -511,3 +511,4 @@ > > 511 auto quick quota > > 512 auto quick acl attr > > 513 auto mount > > +999 auto > > The test looks also 'quick' Sure! Updated in v8 Thanks for the review! Ira > > Thanks, > Amir.
> > > > But the kernel patch suggests that there is an intention to make > > this behavior also applicable to ext4?? > > If that is the case I would recommend making this a generic tests > > which requires filesystem support for -o dax=XXX > > I have a patch set for ext4 which is not quite passing this. I'm not sure what > is going on yet. > > Once that is working I was going to move this to generic. (The documentation > in the kernel patch set also reflects ext4 being different from xfs for the > time being.) IMO, if ext4 maintainer is on board with the plan to make this behavior of ext4 then it is best to add this test as generic from the start. Any other filesystems that may tag along later? > > This is mainly because I'm not sure if ext4 will make 5.8 or not. Would you > prefer making this generic now? I assume there is some way to mark generic > tests for a subset of FS's? I have not figured that out yet. > There is a way, _supported_fs, see the tests/shared/*, but the idea it to get rid of those in favor of feature tests such as _require_scratch_dax I believe it should be trivial to implement _require_scratch_dax_never Thanks, Amir.
On Mon, Apr 13, 2020 at 07:11:46PM +0300, Amir Goldstein wrote: > > > > > > But the kernel patch suggests that there is an intention to make > > > this behavior also applicable to ext4?? > > > If that is the case I would recommend making this a generic tests > > > which requires filesystem support for -o dax=XXX > > > > I have a patch set for ext4 which is not quite passing this. I'm not sure what > > is going on yet. > > > > Once that is working I was going to move this to generic. (The documentation > > in the kernel patch set also reflects ext4 being different from xfs for the > > time being.) > > IMO, if ext4 maintainer is on board with the plan to make this behavior of > ext4 then it is best to add this test as generic from the start. > Any other filesystems that may tag along later? I was under the impression that any test can go in generic/ so long as it isn't using fs-specific interfaces (e.g. xfs error injection), even if not all filesystems actually support the functionality being examined by the test. > > > > This is mainly because I'm not sure if ext4 will make 5.8 or not. Would you > > prefer making this generic now? I assume there is some way to mark generic > > tests for a subset of FS's? I have not figured that out yet. > > > > There is a way, _supported_fs, see the tests/shared/*, > but the idea it to get rid of those in favor of feature tests such as > _require_scratch_dax > > I believe it should be trivial to implement > _require_scratch_dax_never Agreed, though I would name the helper to make it clear that it's checking the dax mount options (e.g. "_require_scratch_dax_mountopt") because "never" is a little subtle here. > Thanks, > Amir.
On Sun, Apr 12, 2020 at 10:44:19PM -0700, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Add XXX to test per file DAX operations. > > The following is tested[*] > > 1. There exists an in-kernel access mode flag S_DAX that is set when > file accesses go directly to persistent memory, bypassing the page > cache. Applications must call statx to discover the current S_DAX > state (STATX_ATTR_DAX). > > 2. There exists an advisory file inode flag FS_XFLAG_DAX that is > inherited from the parent directory FS_XFLAG_DAX inode flag at file > creation time. This advisory flag can be set or cleared at any > time, but doing so does not immediately affect the S_DAX state. > > Unless overridden by mount options (see (3)), if FS_XFLAG_DAX is set > and the fs is on pmem then it will enable S_DAX at inode load time; > if FS_XFLAG_DAX is not set, it will not enable S_DAX. > > 3. There exists a dax= mount option. > > "-o dax=never" means "never set S_DAX, ignore FS_XFLAG_DAX." > > "-o dax=always" means "always set S_DAX (at least on pmem), > and ignore FS_XFLAG_DAX." > > "-o dax" is an alias for "dax=always". > > "-o dax=inode" means "follow FS_XFLAG_DAX" and is the default. > > 4. There exists an advisory directory inode flag FS_XFLAG_DAX that can > be set or cleared at any time. The flag state is copied into any > files or subdirectories when they are created within that directory. > > 5. Programs that require a specific file access mode (DAX or not DAX) > can do one of the following: > > (a) Create files in directories that the FS_XFLAG_DAX flag set as > needed; or > > (b) Have the administrator set an override via mount option; or > > (c) Set or clear the file's FS_XFLAG_DAX flag as needed. Programs > must then cause the kernel to evict the inode from memory. This > can be done by: > > i> Closing the file and re-opening the file and using statx to > see if the fs has changed the S_DAX flag; and > > ii> If the file still does not have the desired S_DAX access > mode, either unmount and remount the filesystem, or close > the file and use drop_caches. > > 6. It's not unreasonable that users who want to squeeze every last bit > of performance out of the particular rough and tumble bits of their > storage also be exposed to the difficulties of what happens when the > operating system can't totally virtualize those hardware > capabilities. Your high performance sports car is not a Toyota > minivan, as it were. > > [*] https://lore.kernel.org/lkml/20200409165927.GD6741@magnolia/ > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes from v6 (kernel patch set): > Start versions tracking the kernel patch set. > Update for new requirements > > Changes from V1 (xfstests patch): > Add test to ensure moved files preserve their flag > Check chattr of non-dax flags (check bug found by Darrick) > --- > tests/xfs/999 | 272 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/999.out | 22 ++++ > tests/xfs/group | 1 + > 3 files changed, 295 insertions(+) > create mode 100755 tests/xfs/999 > create mode 100644 tests/xfs/999.out > > diff --git a/tests/xfs/999 b/tests/xfs/999 > new file mode 100755 > index 000000000000..3ca04b98c517 > --- /dev/null > +++ b/tests/xfs/999 > @@ -0,0 +1,272 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2019 Intel, Corp. All Rights Reserved. > +# > +# FSQA Test No. 999 (temporary) > +# > +# Test setting of DAX flag > +# > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > +echo "QA output created by $seq" > + > +here=`pwd` > +status=1 # failure is the default! > + > +dax_dir=$TEST_DIR/dax-dir > +dax_sub_dir=$TEST_DIR/dax-dir/dax-sub-dir > +dax_inh_file=$dax_dir/dax-inh-file > +dax_non_inh_file=$dax_dir/dax-non-inh-file > +non_dax=$TEST_DIR/non-dax > +dax_file=$TEST_DIR/dax-dir/dax-file > +dax_file_copy=$TEST_DIR/dax-file-copy > +dax_file_move=$TEST_DIR/dax-file-move > +data_file=$TEST_DIR/non-dax-dir/data-file > + > +_cleanup() { > + rm -rf $TEST_DIR/* > +} > + > +trap "_cleanup ; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common/rc > + > +# real QA test starts here > +_supported_os Linux > +_require_xfs_io_command "lsattr" > +_require_xfs_io_command "statx" > +_require_test This might be a good time to introduce a few new helpers: _require_scratch_dax ("Does $SCRATCH_DEV support DAX?") _require_scratch_dax_mountopt ("Does the fs support the DAX mount options?") _require_scratch_daX_iflag ("Does the fs support FS_XFLAG_DAX?") Also, I think this should be split into two tests. One test to check the correct operation of FS_XFLAG_DAX, because the flag has behaviors that always apply even if the test/scratch devices are not pmem; and a second test to check the S_DAX operation if the system under test actually has pmem. > + > +# > +# mnt_opt's we expect > +# '' > +# '-o dax=off' > +# '-o dax=inode' > +# '-o dax' > +# '-o dax=always' > +function remount_w_option { > + mnt_opt=$1 > + export MOUNT_OPTIONS="" > + export TEST_FS_MOUNT_OPTS="" > + _test_unmount &> /dev/null > + _test_mount $mnt_opt &> /dev/null > +} > + > +function check_dax_mount_option { > + mnt_opt=$1 > + _fs_options $TEST_DEV | grep -qw '$mnt_opt' > + if [ "$?" == "0" ]; then > + echo "FAILED: to mount FS with option '$mnt_opt'" > + fi > +} > + > +function check_xflag_dax { > + xfs_io -c 'lsattr' $1 | awk -e '{ print $1 }' | grep 'x' &> /dev/null s/xfs_io/$XFS_IO_PROG/g > + if [ "$?" != "0" ]; then > + echo "FAILED: Did NOT find FS_XFLAG_DAX on $1" > + fi > +} > + > +function check_s_dax { > + attr=`xfs_io -c 'statx -r' $1 | grep 'stat.attributes' | awk -e '{ print $3 }'` Also, the statx bits mean this test needs: _require_xfs_io_command statx -r So that we skip the test on old userspace. --D > + masked=$(( $attr & 0x2000 )) > + if [ "$masked" != "8192" ]; then > + echo "FAILED: Did NOT find S_DAX flag on $1" > + fi > +} > + > +function check_no_xflag_dax { > + xfs_io -c 'lsattr' $1 | awk -e '{ print $1 }' | grep 'x' &> /dev/null > + if [ "$?" == "0" ]; then > + echo "FAILED: Found FS_XFLAG_DAX on $1" > + fi > +} > + > +function check_no_s_dax { > + attr=`xfs_io -c 'statx -r' $1 | grep 'stat.attributes' | awk -e '{ print $3 }'` > + masked=$(( $attr & 0x2000 )) > + if [ "$masked" == "8192" ]; then > + echo "FAILED: Found S_DAX flag on $1" > + fi > +} > + > +echo "running tests..." > + > +remount_w_option "" > +check_dax_mount_option "dax=inode" > + > +echo " *** mark dax-dir as dax enabled" > +mkdir $dax_dir > +xfs_io -c 'chattr +x' $dax_dir > +check_xflag_dax $dax_dir > + > +echo " *** check file inheritance" > +touch $dax_inh_file > +check_xflag_dax $dax_inh_file > +check_s_dax $dax_inh_file > + > +echo " *** check directory inheritance" > +mkdir $dax_sub_dir > +check_xflag_dax $dax_sub_dir > + > +echo " *** check changing directory" > +xfs_io -c 'chattr -x' $dax_dir > +check_no_xflag_dax $dax_dir > +check_no_s_dax $dax_dir > + > +echo " *** check non file inheritance" > +touch $dax_non_inh_file > +check_no_xflag_dax $dax_non_inh_file > +check_no_s_dax $dax_non_inh_file > + > +echo " *** check that previous file stays enabled" > +check_xflag_dax $dax_inh_file > +check_s_dax $dax_inh_file > + > +echo " *** Reset the directory" > +xfs_io -c 'chattr +x' $dax_dir > +check_xflag_dax $dax_dir > + > +# Set up for next test > +touch $dax_file > +touch $non_dax > + > +# > +# inode flag > +# ./ > +# + dax-dir/ X > +# + dax-sub-dir/ X > +# + dax-inh-file X > +# + dax-non-inh-file > +# + dax-file X > +# + non-dax > +# > + > +# check mount overrides > +# ===================== > + > +echo " *** Check '-o dax'" > +remount_w_option "-o dax" > +check_dax_mount_option "dax=always" > + > +echo " *** non-dax inode but overrides to be effective" > +check_no_xflag_dax $non_dax > +check_s_dax $non_dax > + > +echo " *** Check for non-dax inode to be dax with mount option" > +check_no_xflag_dax $dax_non_inh_file > +check_s_dax $dax_non_inh_file > + > + > +echo " *** Check '-o dax=never'" > +remount_w_option "-o dax=never" > +check_dax_mount_option "dax=never" > + > +check_xflag_dax $dax_dir > +check_xflag_dax $dax_sub_dir > +check_xflag_dax $dax_inh_file > +check_no_s_dax $dax_inh_file > +check_no_xflag_dax $dax_non_inh_file > +check_no_s_dax $dax_non_inh_file > +check_no_xflag_dax $non_dax > +check_no_s_dax $non_dax > +check_xflag_dax $dax_file > +check_no_s_dax $dax_file > + > + > +echo " *** Check '-o dax=inode'" > +remount_w_option "-o dax=inode" > +check_dax_mount_option "dax=inode" > + > +check_xflag_dax $dax_dir > +check_xflag_dax $dax_sub_dir > +check_xflag_dax $dax_inh_file > +check_s_dax $dax_inh_file > +check_no_xflag_dax $dax_non_inh_file > +check_no_s_dax $dax_non_inh_file > +check_no_xflag_dax $non_dax > +check_no_s_dax $non_dax > +check_xflag_dax $dax_file > +check_s_dax $dax_file > + > + > +# Check non-zero file operations > +# ============================== > + > +echo " *** Verify changing FS_XFLAG_DAX flag" > + > +mkdir $TEST_DIR/non-dax-dir > +$here/ltp/fsx $options -N 20000 $data_file > $tmp.log 2>&1 & > +pid=$! > +check_no_xflag_dax $data_file > +check_no_s_dax $data_file > + > +if [ ! -n "$pid" ]; then > + echo "FAILED to start fsx" > +else > + # toggle inode flag back and forth. > + # s_dax should not change while fsx is using file. > + xfs_io -c 'chattr +x' $data_file &> /dev/null > + check_xflag_dax $data_file > + check_no_s_dax $data_file > + xfs_io -c 'chattr -x' $data_file &> /dev/null > + check_no_xflag_dax $data_file > + check_no_s_dax $data_file > + xfs_io -c 'chattr +x' $data_file &> /dev/null > + check_xflag_dax $data_file > + check_no_s_dax $data_file > +fi > + > +wait $pid > +status=$? > +if [ "$status" != "0" ]; then > + echo "FAILED: fsx exited with status : $status" > + head $dax_file.fsxlog > +fi > +pid="" > + > +echo 2 > /proc/sys/vm/drop_caches > + > +echo " *** Verify S_DAX change on drop_caches" > +# s_dax should change after drop_caches > +check_xflag_dax $data_file > +check_s_dax $data_file > + > + > +# Check inheritance on cp, mv > +# =========================== > + > +echo " *** check making 'data' dax with cp" > +cp $non_dax $dax_dir/conv-dax > +check_xflag_dax $dax_dir/conv-dax > +check_s_dax $dax_dir/conv-dax > + > +echo " *** check making 'data' non-dax with cp" > +rm -f $data_file > +cp $dax_dir/conv-dax $data_file > +check_no_xflag_dax $data_file > +check_no_s_dax $data_file > + > +echo " *** Moved files 'don't inherit'" > +mv $non_dax $dax_dir/move-dax > +check_no_xflag_dax $dax_dir/move-dax > +check_no_s_dax $dax_dir/move-dax > + > +echo " *** Moved files preserve their flag" > +mv $dax_file $TEST_DIR/move-dax > +check_xflag_dax $TEST_DIR/move-dax > +check_s_dax $TEST_DIR/move-dax > + > +echo " *** Check file chattr of non-dax flags" > +xfs_io -c 'chattr +s' $dax_inh_file > +xfs_io -c 'chattr -s' $dax_inh_file > + > +echo " *** Check '-o dax=garbage'" > +remount_w_option "-o dax=garbage" > +_check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR > +if [ "$?" == "0" ]; then > + echo "ERROR: fs mounted with '-o dax=garbage'" > +fi > + > +status=0 ; exit > diff --git a/tests/xfs/999.out b/tests/xfs/999.out > new file mode 100644 > index 000000000000..3a4f970a5073 > --- /dev/null > +++ b/tests/xfs/999.out > @@ -0,0 +1,22 @@ > +QA output created by 999 > +running tests... > + *** mark dax-dir as dax enabled > + *** check file inheritance > + *** check directory inheritance > + *** check changing directory > + *** check non file inheritance > + *** check that previous file stays enabled > + *** Reset the directory > + *** Check '-o dax' > + *** non-dax inode but overrides to be effective > + *** Check for non-dax inode to be dax with mount option > + *** Check '-o dax=never' > + *** Check '-o dax=inode' > + *** Verify changing FS_XFLAG_DAX flag > + *** Verify S_DAX change on drop_caches > + *** check making 'data' dax with cp > + *** check making 'data' non-dax with cp > + *** Moved files 'don't inherit' > + *** Moved files preserve their flag > + *** Check file chattr of non-dax flags > + *** Check '-o dax=garbage' > diff --git a/tests/xfs/group b/tests/xfs/group > index 522d4bc44d1f..816883a268bf 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -511,3 +511,4 @@ > 511 auto quick quota > 512 auto quick acl attr > 513 auto mount > +999 auto > -- > 2.25.1 >
On 2020/4/14 0:30, Darrick J. Wong wrote: > This might be a good time to introduce a few new helpers: > > _require_scratch_dax ("Does $SCRATCH_DEV support DAX?") > _require_scratch_dax_mountopt ("Does the fs support the DAX mount options?") > _require_scratch_daX_iflag ("Does the fs support FS_XFLAG_DAX?") Hi Darrick, Now, I am trying to introduce these new helpers and have some questions: 1) There are five testcases related to old dax implementation, should we only convert them to new dax implementation or make them compatible with old and new dax implementation? 2) I think _require_xfs_io_command "chattr" "x" is enough to check if fs supports FS_XFLAG_DAX. Is it necessary to add _require_scratch_dax_iflag()? like this: _require_scratch_dax_iflag() { _require_xfs_io_command "chattr" "x" } Best Regards, Xiao Yang
On Tue, Jun 02, 2020 at 04:51:48PM +0800, Xiao Yang wrote: > On 2020/4/14 0:30, Darrick J. Wong wrote: > > This might be a good time to introduce a few new helpers: > > > > _require_scratch_dax ("Does $SCRATCH_DEV support DAX?") > > _require_scratch_dax_mountopt ("Does the fs support the DAX mount options?") > > _require_scratch_daX_iflag ("Does the fs support FS_XFLAG_DAX?") > Hi Darrick, > > Now, I am trying to introduce these new helpers and have some questions: > 1) There are five testcases related to old dax implementation, should we > only convert them to new dax implementation or make them compatible with old > and new dax implementation? What is the 'old' DAX implementation? ext2 XIP? > 2) I think _require_xfs_io_command "chattr" "x" is enough to check if fs > supports FS_XFLAG_DAX. Is it necessary to add _require_scratch_dax_iflag()? > like this: > _require_scratch_dax_iflag() > { > _require_xfs_io_command "chattr" "x" > } I suggested that list based on the major control knobs that will be visible to userspace programs. Even if this is just a one-line helper, its name is useful for recognizing which of those knobs we're looking for. Yes, you could probably save a trivial amount of time by skipping one iteration of bash function calling, but now everyone has to remember that the xfs_io chattr "x" flag means the dax inode flag, and not confuse it for chmod +x or something else. --D > Best Regards, > Xiao Yang > >
On 2020/6/3 2:14, Darrick J. Wong wrote: > On Tue, Jun 02, 2020 at 04:51:48PM +0800, Xiao Yang wrote: >> On 2020/4/14 0:30, Darrick J. Wong wrote: >>> This might be a good time to introduce a few new helpers: >>> >>> _require_scratch_dax ("Does $SCRATCH_DEV support DAX?") >>> _require_scratch_dax_mountopt ("Does the fs support the DAX mount options?") >>> _require_scratch_daX_iflag ("Does the fs support FS_XFLAG_DAX?") >> Hi Darrick, >> >> Now, I am trying to introduce these new helpers and have some questions: >> 1) There are five testcases related to old dax implementation, should we >> only convert them to new dax implementation or make them compatible with old >> and new dax implementation? > > What is the 'old' DAX implementation? ext2 XIP? Hi Darrick, Thanks for your quick feedback. Right, the 'old' DAX implementation means old dax mount option(i.e. -o dax) Compare new and old dax mount option on ext4 and xfs, is the following logic right? -o dax=always == -o dax -o dax=never == without dax -o dax=inode == nothing Of course, we should uses new option if ext4/xfs supports new dax mount option on distros. But should we fallback to use old option if ext4/xfs doesn't support new dax mount option on some old distros? btw: it seems hard for testcases to use two different sets of mount options(i.e. old and new) so do you have any suggestion? > >> 2) I think _require_xfs_io_command "chattr" "x" is enough to check if fs >> supports FS_XFLAG_DAX. Is it necessary to add _require_scratch_dax_iflag()? >> like this: >> _require_scratch_dax_iflag() >> { >> _require_xfs_io_command "chattr" "x" >> } > > I suggested that list based on the major control knobs that will be > visible to userspace programs. Even if this is just a one-line helper, > its name is useful for recognizing which of those knobs we're looking > for. > > Yes, you could probably save a trivial amount of time by skipping one > iteration of bash function calling, but now everyone has to remember > that the xfs_io chattr "x" flag means the dax inode flag, and not > confuse it for chmod +x or something else. Got it, thanks for your detailed explanation. Best Regards, Xiao Yang > > --D > >> Best Regards, >> Xiao Yang >> >> > > > . >
On Wed, Jun 03, 2020 at 09:56:13AM +0800, Xiao Yang wrote: > On 2020/6/3 2:14, Darrick J. Wong wrote: > > On Tue, Jun 02, 2020 at 04:51:48PM +0800, Xiao Yang wrote: > > > On 2020/4/14 0:30, Darrick J. Wong wrote: > > > > This might be a good time to introduce a few new helpers: > > > > > > > > _require_scratch_dax ("Does $SCRATCH_DEV support DAX?") > > > > _require_scratch_dax_mountopt ("Does the fs support the DAX mount options?") > > > > _require_scratch_daX_iflag ("Does the fs support FS_XFLAG_DAX?") > > > Hi Darrick, > > > > > > Now, I am trying to introduce these new helpers and have some questions: > > > 1) There are five testcases related to old dax implementation, should we > > > only convert them to new dax implementation or make them compatible with old > > > and new dax implementation? > > > > What is the 'old' DAX implementation? ext2 XIP? > Hi Darrick, > > Thanks for your quick feedback. > > Right, the 'old' DAX implementation means old dax mount option(i.e. -o dax) > > Compare new and old dax mount option on ext4 and xfs, is the following logic > right? > -o dax=always == -o dax > -o dax=never == without dax > -o dax=inode == nothing No. -o dax=always is the same as -o dax. dax=inode was and still is the behavior you got with no option at all. -o dax=never is a new option. > Of course, we should uses new option if ext4/xfs supports new dax mount > option on distros. But should we fallback to use old option if ext4/xfs > doesn't support new dax mount option on some old distros? > btw: > it seems hard for testcases to use two different sets of mount options(i.e. > old and new) so do you have any suggestion? Try dax=never, it should work on any type of storage device if the kernel implements the "new" mount options at all. --D > > > > > 2) I think _require_xfs_io_command "chattr" "x" is enough to check if fs > > > supports FS_XFLAG_DAX. Is it necessary to add _require_scratch_dax_iflag()? > > > like this: > > > _require_scratch_dax_iflag() > > > { > > > _require_xfs_io_command "chattr" "x" > > > } > > > > I suggested that list based on the major control knobs that will be > > visible to userspace programs. Even if this is just a one-line helper, > > its name is useful for recognizing which of those knobs we're looking > > for. > > > > Yes, you could probably save a trivial amount of time by skipping one > > iteration of bash function calling, but now everyone has to remember > > that the xfs_io chattr "x" flag means the dax inode flag, and not > > confuse it for chmod +x or something else. > > Got it, thanks for your detailed explanation. > > Best Regards, > Xiao Yang > > > > --D > > > > > Best Regards, > > > Xiao Yang > > > > > > > > > > > > . > > > > >
diff --git a/tests/xfs/999 b/tests/xfs/999 new file mode 100755 index 000000000000..3ca04b98c517 --- /dev/null +++ b/tests/xfs/999 @@ -0,0 +1,272 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2019 Intel, Corp. All Rights Reserved. +# +# FSQA Test No. 999 (temporary) +# +# Test setting of DAX flag +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +dax_dir=$TEST_DIR/dax-dir +dax_sub_dir=$TEST_DIR/dax-dir/dax-sub-dir +dax_inh_file=$dax_dir/dax-inh-file +dax_non_inh_file=$dax_dir/dax-non-inh-file +non_dax=$TEST_DIR/non-dax +dax_file=$TEST_DIR/dax-dir/dax-file +dax_file_copy=$TEST_DIR/dax-file-copy +dax_file_move=$TEST_DIR/dax-file-move +data_file=$TEST_DIR/non-dax-dir/data-file + +_cleanup() { + rm -rf $TEST_DIR/* +} + +trap "_cleanup ; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common/rc + +# real QA test starts here +_supported_os Linux +_require_xfs_io_command "lsattr" +_require_xfs_io_command "statx" +_require_test + +# +# mnt_opt's we expect +# '' +# '-o dax=off' +# '-o dax=inode' +# '-o dax' +# '-o dax=always' +function remount_w_option { + mnt_opt=$1 + export MOUNT_OPTIONS="" + export TEST_FS_MOUNT_OPTS="" + _test_unmount &> /dev/null + _test_mount $mnt_opt &> /dev/null +} + +function check_dax_mount_option { + mnt_opt=$1 + _fs_options $TEST_DEV | grep -qw '$mnt_opt' + if [ "$?" == "0" ]; then + echo "FAILED: to mount FS with option '$mnt_opt'" + fi +} + +function check_xflag_dax { + xfs_io -c 'lsattr' $1 | awk -e '{ print $1 }' | grep 'x' &> /dev/null + if [ "$?" != "0" ]; then + echo "FAILED: Did NOT find FS_XFLAG_DAX on $1" + fi +} + +function check_s_dax { + attr=`xfs_io -c 'statx -r' $1 | grep 'stat.attributes' | awk -e '{ print $3 }'` + masked=$(( $attr & 0x2000 )) + if [ "$masked" != "8192" ]; then + echo "FAILED: Did NOT find S_DAX flag on $1" + fi +} + +function check_no_xflag_dax { + xfs_io -c 'lsattr' $1 | awk -e '{ print $1 }' | grep 'x' &> /dev/null + if [ "$?" == "0" ]; then + echo "FAILED: Found FS_XFLAG_DAX on $1" + fi +} + +function check_no_s_dax { + attr=`xfs_io -c 'statx -r' $1 | grep 'stat.attributes' | awk -e '{ print $3 }'` + masked=$(( $attr & 0x2000 )) + if [ "$masked" == "8192" ]; then + echo "FAILED: Found S_DAX flag on $1" + fi +} + +echo "running tests..." + +remount_w_option "" +check_dax_mount_option "dax=inode" + +echo " *** mark dax-dir as dax enabled" +mkdir $dax_dir +xfs_io -c 'chattr +x' $dax_dir +check_xflag_dax $dax_dir + +echo " *** check file inheritance" +touch $dax_inh_file +check_xflag_dax $dax_inh_file +check_s_dax $dax_inh_file + +echo " *** check directory inheritance" +mkdir $dax_sub_dir +check_xflag_dax $dax_sub_dir + +echo " *** check changing directory" +xfs_io -c 'chattr -x' $dax_dir +check_no_xflag_dax $dax_dir +check_no_s_dax $dax_dir + +echo " *** check non file inheritance" +touch $dax_non_inh_file +check_no_xflag_dax $dax_non_inh_file +check_no_s_dax $dax_non_inh_file + +echo " *** check that previous file stays enabled" +check_xflag_dax $dax_inh_file +check_s_dax $dax_inh_file + +echo " *** Reset the directory" +xfs_io -c 'chattr +x' $dax_dir +check_xflag_dax $dax_dir + +# Set up for next test +touch $dax_file +touch $non_dax + +# +# inode flag +# ./ +# + dax-dir/ X +# + dax-sub-dir/ X +# + dax-inh-file X +# + dax-non-inh-file +# + dax-file X +# + non-dax +# + +# check mount overrides +# ===================== + +echo " *** Check '-o dax'" +remount_w_option "-o dax" +check_dax_mount_option "dax=always" + +echo " *** non-dax inode but overrides to be effective" +check_no_xflag_dax $non_dax +check_s_dax $non_dax + +echo " *** Check for non-dax inode to be dax with mount option" +check_no_xflag_dax $dax_non_inh_file +check_s_dax $dax_non_inh_file + + +echo " *** Check '-o dax=never'" +remount_w_option "-o dax=never" +check_dax_mount_option "dax=never" + +check_xflag_dax $dax_dir +check_xflag_dax $dax_sub_dir +check_xflag_dax $dax_inh_file +check_no_s_dax $dax_inh_file +check_no_xflag_dax $dax_non_inh_file +check_no_s_dax $dax_non_inh_file +check_no_xflag_dax $non_dax +check_no_s_dax $non_dax +check_xflag_dax $dax_file +check_no_s_dax $dax_file + + +echo " *** Check '-o dax=inode'" +remount_w_option "-o dax=inode" +check_dax_mount_option "dax=inode" + +check_xflag_dax $dax_dir +check_xflag_dax $dax_sub_dir +check_xflag_dax $dax_inh_file +check_s_dax $dax_inh_file +check_no_xflag_dax $dax_non_inh_file +check_no_s_dax $dax_non_inh_file +check_no_xflag_dax $non_dax +check_no_s_dax $non_dax +check_xflag_dax $dax_file +check_s_dax $dax_file + + +# Check non-zero file operations +# ============================== + +echo " *** Verify changing FS_XFLAG_DAX flag" + +mkdir $TEST_DIR/non-dax-dir +$here/ltp/fsx $options -N 20000 $data_file > $tmp.log 2>&1 & +pid=$! +check_no_xflag_dax $data_file +check_no_s_dax $data_file + +if [ ! -n "$pid" ]; then + echo "FAILED to start fsx" +else + # toggle inode flag back and forth. + # s_dax should not change while fsx is using file. + xfs_io -c 'chattr +x' $data_file &> /dev/null + check_xflag_dax $data_file + check_no_s_dax $data_file + xfs_io -c 'chattr -x' $data_file &> /dev/null + check_no_xflag_dax $data_file + check_no_s_dax $data_file + xfs_io -c 'chattr +x' $data_file &> /dev/null + check_xflag_dax $data_file + check_no_s_dax $data_file +fi + +wait $pid +status=$? +if [ "$status" != "0" ]; then + echo "FAILED: fsx exited with status : $status" + head $dax_file.fsxlog +fi +pid="" + +echo 2 > /proc/sys/vm/drop_caches + +echo " *** Verify S_DAX change on drop_caches" +# s_dax should change after drop_caches +check_xflag_dax $data_file +check_s_dax $data_file + + +# Check inheritance on cp, mv +# =========================== + +echo " *** check making 'data' dax with cp" +cp $non_dax $dax_dir/conv-dax +check_xflag_dax $dax_dir/conv-dax +check_s_dax $dax_dir/conv-dax + +echo " *** check making 'data' non-dax with cp" +rm -f $data_file +cp $dax_dir/conv-dax $data_file +check_no_xflag_dax $data_file +check_no_s_dax $data_file + +echo " *** Moved files 'don't inherit'" +mv $non_dax $dax_dir/move-dax +check_no_xflag_dax $dax_dir/move-dax +check_no_s_dax $dax_dir/move-dax + +echo " *** Moved files preserve their flag" +mv $dax_file $TEST_DIR/move-dax +check_xflag_dax $TEST_DIR/move-dax +check_s_dax $TEST_DIR/move-dax + +echo " *** Check file chattr of non-dax flags" +xfs_io -c 'chattr +s' $dax_inh_file +xfs_io -c 'chattr -s' $dax_inh_file + +echo " *** Check '-o dax=garbage'" +remount_w_option "-o dax=garbage" +_check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR +if [ "$?" == "0" ]; then + echo "ERROR: fs mounted with '-o dax=garbage'" +fi + +status=0 ; exit diff --git a/tests/xfs/999.out b/tests/xfs/999.out new file mode 100644 index 000000000000..3a4f970a5073 --- /dev/null +++ b/tests/xfs/999.out @@ -0,0 +1,22 @@ +QA output created by 999 +running tests... + *** mark dax-dir as dax enabled + *** check file inheritance + *** check directory inheritance + *** check changing directory + *** check non file inheritance + *** check that previous file stays enabled + *** Reset the directory + *** Check '-o dax' + *** non-dax inode but overrides to be effective + *** Check for non-dax inode to be dax with mount option + *** Check '-o dax=never' + *** Check '-o dax=inode' + *** Verify changing FS_XFLAG_DAX flag + *** Verify S_DAX change on drop_caches + *** check making 'data' dax with cp + *** check making 'data' non-dax with cp + *** Moved files 'don't inherit' + *** Moved files preserve their flag + *** Check file chattr of non-dax flags + *** Check '-o dax=garbage' diff --git a/tests/xfs/group b/tests/xfs/group index 522d4bc44d1f..816883a268bf 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -511,3 +511,4 @@ 511 auto quick quota 512 auto quick acl attr 513 auto mount +999 auto