xfs: some DAX fixes
Subsystems (3)
cover xfs: some DAX fixes
These two fixes are to the now-disabled per-inode DAX support in XFS. I started working on these in the v4.13 timeframe before the per-inode DAX feature was turned off.
I realize that they may not be critical right now since the per-inode DAX feature is turned off, but I think that if we ever do re-enable that feature we will want these two issues to have been fixed.
I also do think that they are both vaulable for stable kernels.
These used to be part of my ext4 per-inode enabling:
https://lkml.org/lkml/2017/9/5/748
But I've since split that into a series of XFS bug fixes (these two) and a series of ext4 bug fixes.
Ross Zwisler (2):
xfs: always use DAX if mount option is used
xfs: validate bdev support for DAX inode flag
fs/xfs/xfs_ioctl.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
Signature · 6 lines
2.9.5 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 1/2 xfs: always use DAX if mount option is used
Before support for the per-inode DAX flag was disabled the XFS the code had an issue where the user couldn't reliably tell whether or not DAX was being used to service page faults and I/O when the DAX mount option was used. In this case each inode within the mounted filesystem started with S_DAX set due to the mount option, but it could be cleared if someone touched the individual inode flag.
For example (v4.13 and before):
# mount | grep dax /dev/pmem0 on /mnt type xfs (rw,relatime,seclabel,attr2,dax,inode64,sunit=4096,swidth=4096,noquota)
# touch /mnt/a /mnt/b # both files currently use DAX
# xfs_io -c "lsattr" /mnt/* # neither has the DAX inode option set ----------e----- /mnt/a ----------e----- /mnt/b
# xfs_io -c "chattr -x" /mnt/a # this clears S_DAX for /mnt/a
# xfs_io -c "lsattr" /mnt/* ----------e----- /mnt/a ----------e----- /mnt/b
We end up with both /mnt/a and /mnt/b looking identical from the point of view of the mount option and from lsattr, but one is using DAX and the other is not.
Fix this by always doing DAX I/O when either the mount option is set or when the DAX inode flag is set. This means that DAX will always be used for all inodes on a filesystem mounted with -o dax, making the usage reliable and detectable.
This does not fix the race issues that caused the XFS DAX inode option to be disabled, so that option will still be disabled. If/when we re-enable it, though, I think we will want this issue to have been fixed. I also do think that we want to fix this in stable kernels.
--- fs/xfs/xfs_ioctl.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
fs/xfs/xfs_ioctl.c +9 -2
@@ -1013,7 +1013,7 @@ xfs_diflags_to_linux(else inode->i_flags &= ~S_NOATIME ;#if 0 /* disabled until the flag switching races are sorted out */ -if (xflags &FS_XFLAG_DAX ) +if ((xflags &FS_XFLAG_DAX ) || (ip->i_mount->m_flags &XFS_MOUNT_DAX )) inode->i_flags |=S_DAX ;else inode->i_flags &= ~S_DAX ;
@@ -1104,7 +1104,14 @@ xfs_ioctl_setattr_dax_invalidate(return -EINVAL ; } -/* If the DAX state is not changing, we have nothing to do here. */ +/* +* If the DAX state is not changing, we have nothing to do here. If +* the DAX mount option was used we will update the DAX inode flag as +* the user requested but we will continue to use DAX for I/O and page +* faults regardless of how the inode flag is set. +*/ +if (ip->i_mount->m_flags &XFS_MOUNT_DAX ) +return 0 ;if ((fa->fsx_xflags &FS_XFLAG_DAX ) &&IS_DAX (inode))return 0 ;if (!(fa->fsx_xflags &FS_XFLAG_DAX ) && !IS_DAX (inode))
Signature · 6 lines
2.9.5 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 1/2] xfs: always use DAX if mount option is used
Quoted 6 lines · Ross Zwisler
Before support for the per-inode DAX flag was disabled the XFS the code had an issue where the user couldn't reliably tell whether or not DAX was being used to service page faults and I/O when the DAX mount option was used. In this case each inode within the mounted filesystem started with
S_DAXset due to the mount option, but it could be cleared if someone touched the individual inode flag.
Looks good, but can you please add a testcase to xfstests for this?
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 1/2] xfs: always use DAX if mount option is used
Quoted 9 lines · Christoph Hellwig
Quoted 6 lines · Ross Zwisler
Before support for the per-inode DAX flag was disabled the XFS the code had an issue where the user couldn't reliably tell whether or not DAX was being used to service page faults and I/O when the DAX mount option was used. In this case each inode within the mounted filesystem started with
S_DAXset due to the mount option, but it could be cleared if someone touched the individual inode flag.Looks good, but can you please add a testcase to xfstests for this?
Hmm...aside from looking at tracepoints, I'm not sure how to detect whether or not S_DAX is actually being used for an inode. I don't see any other xfstests that try and look at the trace buffer, but I guess that could work.
Do you know of a better way to detect this test failure/success?
Quoted 1 line
Reviewed-by Christoph Hellwig <[email protected]>
Thanks for the review!
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 1/1 xfs: add regression test for DAX mount option usage
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
--- tests/xfs/431 | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/431.out | 3 +++ tests/xfs/group | 1 + 3 files changed, 85 insertions(+) create mode 100755 tests/xfs/431 create mode 100644 tests/xfs/431.out
tests/xfs/431 new file +81 -0
@@ -0,0 +1,81 @@ +#! /bin/bash +# FS QA Test xfs/431 +# +# This is a regression test for kernel patch: +# xfs: always use DAX if mount option is used +# created by Ross Zwisler <[email protected]> +# +#----------------------------------------------------------------------- +# Copyright (c) 2017 Intel Corporation. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc + +# remove previous $seqres.full before test +rm -f $seqres.full + +# Modify as appropriate. +_supported_os Linux +_supported_fs xfs +_require_scratch_dax + +# real QA test starts here +# format and mount +_scratch_mkfs > $seqres.full 2>&1 +_scratch_mount "-o dax" >> $seqres.full 2>&1 + +pgsize=`$here/src/feature -s` + +# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_on +echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enable + +# enable tracing for our one mmap I/O, then see if dax was used +echo 1 > /sys/kernel/debug/tracing/tracing_on +xfs_io -t -c "truncate $pgsize" \ + -c "chattr -x" \ + -c "mmap -r 0 $pgsize" -c "mread 0 $pgsize" -c "munmap" \ + -f $SCRATCH_MNT/testfile >> $seqres.full +echo 0 > /sys/kernel/debug/tracing/tracing_on + +grep -q 'xfs_io.*dax_load_hole' /sys/kernel/debug/tracing/trace +if [[ $? == 0 ]]; then + echo "DAX was used" +else + echo "DAX was NOT used" +fi + +# success, all done +echo "Silence is golden" +status=0 +exit
tests/xfs/431.out new file +3 -0
@@ -0,0 +1,3 @@ +QA output created by 431 +DAX was used +Silence is golden
tests/xfs/group +1 -0
@@ -427,3 +427,4 @@ 428 dangerous_fuzzers dangerous_scrub dangerous_online_repair 429 dangerous_fuzzers dangerous_scrub dangerous_repair 430 dangerous_fuzzers dangerous_scrub dangerous_online_repair +431 auto quick
Signature · 6 lines
2.9.5 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 10 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>
<>
tests/xfs/431 line 61
Quoted 4 lines
tests/xfs/431 new file +4
-0+# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_on +echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enable
I think I can do a better job of:
a) detecting whether debugfs is present, and if not just bail the test as not run
b) recording the current state of the debugfs (tracing_on, fs_dax/enable), doing what I need to do, then restoring the previous state.
Before I work on a v2, though, I'd love to get feedback on whether or not using tracepoints is okay in fstests.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 21 lines · Ross Zwisler
Quoted 10 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]><>
tests/xfs/431 line 61
Quoted 4 lines
tests/xfs/431 new file +4
-0+# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_on +echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enableI think I can do a better job of:
Quoted 8 lines
a) detecting whether debugfs is present, and if not just bail the test as not run
b) recording the current state of the debugfs (
tracing_on, fs_dax/enable), doing what I need to do, then restoring the previous state.Before I work on a v2, though, I'd love to get feedback on whether or not using tracepoints is okay in fstests.
I think it is more straightforward to use "perf record -e fs_dax $test". Then you know the events are filtered to your test pid and you don't need to worry about managing the trace interface.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH v2 1/1 xfs: add regression test for DAX mount option usage
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
---
Changes since v1:
- Use perf instead of tracepoints to detect whether DAX is used. (Dan)
--- tests/xfs/431 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/431.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 80 insertions(+) create mode 100755 tests/xfs/431 create mode 100644 tests/xfs/431.out
tests/xfs/431 new file +77 -0
@@ -0,0 +1,77 @@ +#! /bin/bash +# FS QA Test xfs/431 +# +# This is a regression test for kernel patch: +# xfs: always use DAX if mount option is used +# created by Ross Zwisler <[email protected]> +# +#----------------------------------------------------------------------- +# Copyright (c) 2017 Intel Corporation. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc + +# remove previous $seqres.full before test +rm -f $seqres.full + +# Modify as appropriate. +_supported_os Linux +_supported_fs xfs +_require_scratch_dax + +# real QA test starts here +export PERF_PROG="`set_prog_path perf`" +[ "$PERF_PROG" = "" ] && _notrun "perf not found" + +_scratch_mkfs > $seqres.full 2>&1 +_scratch_mount "-o dax" >> $seqres.full 2>&1 + +pgsize=`$here/src/feature -s` + +PERF_OUTPUT=$tmp.perf + +$PERF_PROG record -o $PERF_OUTPUT -e 'fs_dax:dax_load_hole' \ + $XFS_IO_PROG -t -c "truncate $pgsize" \ + -c "chattr -x" \ + -c "mmap -r 0 $pgsize" -c "mread 0 $pgsize" -c "munmap" \ + -f $SCRATCH_MNT/testfile > /dev/null 2>&1 + +$PERF_PROG script -i $PERF_OUTPUT | grep -q 'fs_dax:dax_load_hole' +if [[ $? == 0 ]]; then + echo "DAX was used" +else + echo "DAX was NOT used" +fi + +# success, all done +status=0 +exit
tests/xfs/431.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 431 +DAX was used
tests/xfs/group +1 -0
@@ -427,3 +427,4 @@ 428 dangerous_fuzzers dangerous_scrub dangerous_online_repair 429 dangerous_fuzzers dangerous_scrub dangerous_repair 430 dangerous_fuzzers dangerous_scrub dangerous_online_repair +431 auto quick
Signature · 6 lines
2.9.5 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage
Hi Ross,
Quoted 8 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Quoted 6 lines
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>---
Changes since v1:
- Use perf instead of tracepoints to detect whether DAX is used. (Dan)
Thanks for the test! But I agreed with Dave here, it doesn't seem like a good idea to depend on the kernel tracepoints in a test, but I can't think of a better solution either, so I didn't get to this patch earlier..
Before XFS disabled the ability to switch on & off per-inode DAX flag, the x flag was only shown after an explicit 'chattr +x', even if XFS was mounted with dax option, e.g.
# mkfs -t xfs -f /dev/ram0 # mount -o dax /dev/ram0 /mnt/xfs # echo "test" > /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------- /mnt/xfs/testfile # xfs_io -c "chattr +x" /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------x /mnt/xfs/testfile
I'm wondering if it makes sense to make lsattr print the x flag by default when XFS is mounted with dax option, that way, we have a method to know whether dax is used or not on a particular file too.
Thanks,
Eryu
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage
Quoted 27 lines · Eryu Guan
Hi Ross,
Quoted 8 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Quoted 6 lines
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>---
Changes since v1:
- Use perf instead of tracepoints to detect whether DAX is used. (Dan)Thanks for the test! But I agreed with Dave here, it doesn't seem like a good idea to depend on the kernel tracepoints in a test, but I can't think of a better solution either, so I didn't get to this patch earlier..
Before XFS disabled the ability to switch on & off per-inode DAX flag, the x flag was only shown after an explicit 'chattr +x', even if XFS was mounted with dax option, e.g.
Quoted 8 lines
# mkfs -t xfs -f /dev/ram0 # mount -o dax /dev/ram0 /mnt/xfs # echo "test" > /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------- /mnt/xfs/testfile # xfs_io -c "chattr +x" /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------x /mnt/xfs/testfile
XFS actually still works this way, you just don't get dax now when you chattr +x. :-/ But the inode flag is actually still there, gets updated by chattr and can be listed with lsattr.
Actually, that feels like a really bad situation to be in - Christoph & Dave, should we do more to remove the flag as long as it's not working? i.e. remove it from the lsattr output and make "chattr +x" fail with -EINVAL or similar?
Quoted 3 lines
I'm wondering if it makes sense to make lsattr print the x flag by default when XFS is mounted with dax option, that way, we have a method to know whether dax is used or not on a particular file too.
Well, the per-inode flag that gets set in the filesystem metadata and the actual S_DAX runtime flag which controls whether or not we do DAX I/O and page faults are different things, and as we've seen they aren't always synchronized.
I think making the 'x' flag in lsattr reflect the current state of S_DAX is interesting, but it would suffer from the same TOCTOU races that Dave was concerned about for the proposed VM_DAX flag:
https://lkml.org/lkml/2016/9/16/13
It could also be surprising to users - if they had mounted with -o dax, lsattr on each of their files would show the 'x' flag, but if they remount without that option those 'x' flags would go away. I think this is surprising because normally it takes a chattr to modify the flags.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage
Quoted 19 lines · Ross Zwisler
Quoted 18 lines · Eryu Guan
Hi Ross,
Quoted 14 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>---
Changes since v1:
- Use perf instead of tracepoints to detect whether DAX is used. (Dan)
Quoted 8 lines
Quoted 8 lines
Thanks for the test! But I agreed with Dave here, it doesn't seem like a good idea to depend on the kernel tracepoints in a test, but I can't think of a better solution either, so I didn't get to this patch earlier..
Before XFS disabled the ability to switch on & off per-inode DAX flag, the x flag was only shown after an explicit 'chattr +x', even if XFS was mounted with dax option, e.g.
Of course. lsattr reports the *on-disk inode flag state*. It does not report whether the file is using that feature or not, just whether the flag is set on disk.
Remember, lsattr does not report a "sum of all config states for a feature". It reports on disk state and on disk state only. It follows the same semantics as other flags that have equivalent mount options like dirsync, sync, noatime, etc.
Quoted 9 lines
Quoted 8 lines
# mkfs -t xfs -f /dev/ram0 # mount -o dax /dev/ram0 /mnt/xfs # echo "test" > /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------- /mnt/xfs/testfile # xfs_io -c "chattr +x" /mnt/xfs/testfile # xfs_io -c "lsattr" /mnt/xfs/testfile ---------------x /mnt/xfs/testfile
Quoted 7 lines
XFS actually still works this way, you just don't get dax now when you chattr +x. :-/ But the inode flag is actually still there, gets updated by chattr and can be listed with lsattr.
Actually, that feels like a really bad situation to be in - Christoph & Dave, should we do more to remove the flag as long as it's not working? i.e. remove it from the lsattr output and make "chattr +x" fail with
-EINVALor similar?
How about we concentrate on fixing the problem that led us to disable it temporarily in XFS? Then it can be re-enabled and problems like this go away...
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
In reply to Ross ZwislerQuoted 10 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>
.....
Quoted 4 lines
tests/xfs/431 new file +4
-0+pgsize=`$here/src/feature -s` + +# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_on
tests/xfs/431 line 69
Quoted 10 lines
tests/xfs/431 new file +10
-0+echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enable + +# enable tracing for our one mmap I/O, then see if dax was used +echo 1 > /sys/kernel/debug/tracing/tracing_on +xfs_io -t -c "truncate $pgsize" \ + -c "chattr -x" \ + -c "mmap -r 0 $pgsize" -c "mread 0 $pgsize" -c "munmap" \ + -f $SCRATCH_MNT/testfile >> $seqres.full +echo 0 > /sys/kernel/debug/tracing/tracing_on
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 30 lines · Dave Chinner
Quoted 10 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>.....
Quoted 4 lines
tests/xfs/431 new file +4
-0+pgsize=`$here/src/feature -s` + +# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_ontests/xfs/431 line 69
Quoted 10 lines
tests/xfs/431 new file +10
-0+echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enable + +# enable tracing for our one mmap I/O, then see if dax was used +echo 1 > /sys/kernel/debug/tracing/tracing_on +xfs_io -t -c "truncate $pgsize" \ + -c "chattr -x" \ + -c "mmap -r 0 $pgsize" -c "mread 0 $pgsize" -c "munmap" \ + -f $SCRATCH_MNT/testfile >> $seqres.full +echo 0 > /sys/kernel/debug/tracing/tracing_onSo what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, these are both valid concerns. I ended up trying to get around both of them by using perf instead in my v2, as suggested by Dan:
https://www.spinics.net/lists/linux-xfs/msg10420.html
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 31 lines · Ross Zwisler
Quoted 30 lines · Dave Chinner
Quoted 10 lines · Ross Zwisler
This adds a regression test for the following kernel patch:
xfs: always use DAX if mount option is used
This test will also pass with kernel v4.14-rc1 and beyond because the XFS DAX I/O mount option has been disabled (but not removed), so the "chattr -x" to turn off DAX doesn't actually do anything.
Signed-off-by Ross Zwisler <[email protected]>Suggested-by Christoph Hellwig <[email protected]>.....
Quoted 4 lines
tests/xfs/431 new file +4
-0+pgsize=`$here/src/feature -s` + +# disable tracing, clear the existing trace buffer and turn on dax tracepoints +echo 0 > /sys/kernel/debug/tracing/tracing_ontests/xfs/431 line 69
Quoted 10 lines
tests/xfs/431 new file +10
-0+echo > /sys/kernel/debug/tracing/trace +echo 1 > /sys/kernel/debug/tracing/events/fs_dax/enable + +# enable tracing for our one mmap I/O, then see if dax was used +echo 1 > /sys/kernel/debug/tracing/tracing_on +xfs_io -t -c "truncate $pgsize" \ + -c "chattr -x" \ + -c "mmap -r 0 $pgsize" -c "mread 0 $pgsize" -c "munmap" \ + -f $SCRATCH_MNT/testfile >> $seqres.full +echo 0 > /sys/kernel/debug/tracing/tracing_onSo what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
Quoted 7 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, these are both valid concerns. I ended up trying to get around both of them by using perf instead in my v2, as suggested by Dan:
I think similar concerns exist with using perf, too....
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
<>
Quoted 1 line
I think similar concerns exist with using perf, too....
I though that using perf addressed both concerns?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
By using perf we isolate our tracing from whatever other tracing is happening in the system. So, unlike the case where we were messing with a system-wide ftrace knob, we run perf on our executable, and someone else can run perf on their executable, and they don't collide.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, you're right that this adds a dependency on perf. But unfortunately, without using either perf or ftrace, I don't know of a way to detect whether or not DAX is actually being used. Can you think of another way?
I tried to do this correctly and just skip the test with _notrun if perf isn't available on the host system. This is the same thing that happens if you are missing other dependencies for a test (some other command (chacl, getfattr, setfattr) not present, quota tools not installed, required users not present, etc).
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 8 lines · Ross Zwisler
On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
<>
Quoted 1 line
I think similar concerns exist with using perf, too....
I though that using perf addressed both concerns?
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
By using perf we isolate our tracing from whatever other tracing is happening in the system. So, unlike the case where we were messing with a system-wide ftrace knob, we run perf on our executable, and someone else can run perf on their executable, and they don't collide.
Yes, you've addressed the "gets inteh way of other tracing concern, but it's doesn't address the "it's an ugly way to determine a feature is active" concerns. It also creates an implicit stable tracepoint out of whatever you are looking at. i.e. that tracepoint can't go away, nor can it change functionality once a userspace app depends on it's current semantics to function correctly.
And....
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, you're right that this adds a dependency on perf. But unfortunately, without using either perf or ftrace, I don't know of a way to detect whether or not DAX is actually being used. Can you think of another way?
... if there isn't a programmatic interface to tell applications that DAX is in use, then perhaps we're missing a formal userspace API that we should have, yes?
Quoted 5 lines
I tried to do this correctly and just skip the test with
_notrunif perf isn't available on the host system. This is the same thing that happens if you are missing other dependencies for a test (some other command (chacl, getfattr, setfattr) not present, quota tools not installed, required users not present, etc).
Sure, but if we have user configurable functionality, then using something like ftrace/perf to discover if it's turned is indicative of a bigger problem. i.e. that the user can't tell if the functionality they turned on/off is actually active or not.
That's a bug that needs fixing, not working around with ftrace/perf in xfstests...
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 43 lines · Dave Chinner
Quoted 8 lines · Ross Zwisler
On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
<>
Quoted 1 line
I think similar concerns exist with using perf, too....
I though that using perf addressed both concerns?
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
By using perf we isolate our tracing from whatever other tracing is happening in the system. So, unlike the case where we were messing with a system-wide ftrace knob, we run perf on our executable, and someone else can run perf on their executable, and they don't collide.
Yes, you've addressed the "gets inteh way of other tracing concern, but it's doesn't address the "it's an ugly way to determine a feature is active" concerns. It also creates an implicit stable tracepoint out of whatever you are looking at. i.e. that tracepoint can't go away, nor can it change functionality once a userspace app depends on it's current semantics to function correctly.
And....
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, you're right that this adds a dependency on perf. But unfortunately, without using either perf or ftrace, I don't know of a way to detect whether or not DAX is actually being used. Can you think of another way?
... if there isn't a programmatic interface to tell applications that DAX is in use, then perhaps we're missing a formal userspace API that we should have, yes?
Quoted 5 lines
I tried to do this correctly and just skip the test with
_notrunif perf isn't available on the host system. This is the same thing that happens if you are missing other dependencies for a test (some other command (chacl, getfattr, setfattr) not present, quota tools not installed, required users not present, etc).
Quoted 8 lines
Sure, but if we have user configurable functionality, then using something like ftrace/perf to discover if it's turned is indicative of a bigger problem. i.e. that the user can't tell if the functionality they turned on/off is actually active or not.
That's a bug that needs fixing, not working around with ftrace/perf in xfstests...
Yes, we need a way to determine that DAX is enabled, but certainly not a VM_DAX flag as that was NAKd by you here:
https://lkml.org/lkml/2016/9/16/13
I think one consideration is to just build this into MAP_SYNC. MAP_SYNC can only succeed if DAX is enabled and that it is a reliable way for userspace to assume DAX. There is some consensus for this proposal here:
https://lists.01.org/pipermail/linux-nvdimm/2017-August/012086.html
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 49 lines · Dan Williams
Quoted 48 lines · Dave Chinner
Quoted 8 lines · Ross Zwisler
On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
<>
Quoted 1 line
I think similar concerns exist with using perf, too....
I though that using perf addressed both concerns?
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
By using perf we isolate our tracing from whatever other tracing is happening in the system. So, unlike the case where we were messing with a system-wide ftrace knob, we run perf on our executable, and someone else can run perf on their executable, and they don't collide.
Yes, you've addressed the "gets inteh way of other tracing concern, but it's doesn't address the "it's an ugly way to determine a feature is active" concerns. It also creates an implicit stable tracepoint out of whatever you are looking at. i.e. that tracepoint can't go away, nor can it change functionality once a userspace app depends on it's current semantics to function correctly.
And....
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, you're right that this adds a dependency on perf. But unfortunately, without using either perf or ftrace, I don't know of a way to detect whether or not DAX is actually being used. Can you think of another way?
... if there isn't a programmatic interface to tell applications that DAX is in use, then perhaps we're missing a formal userspace API that we should have, yes?
Quoted 5 lines
I tried to do this correctly and just skip the test with
_notrunif perf isn't available on the host system. This is the same thing that happens if you are missing other dependencies for a test (some other command (chacl, getfattr, setfattr) not present, quota tools not installed, required users not present, etc).Sure, but if we have user configurable functionality, then using something like ftrace/perf to discover if it's turned is indicative of a bigger problem. i.e. that the user can't tell if the functionality they turned on/off is actually active or not.
Quoted 8 lines
Quoted 3 lines
That's a bug that needs fixing, not working around with ftrace/perf in xfstests...
Yes, we need a way to determine that DAX is enabled, but certainly not a
VM_DAXflag as that was NAKd by you here:https://lkml.org/lkml/2016/9/16/13
Yeah, the on disk flag doesn't tell you if DAX is actually in use or not.
Quoted 6 lines
I think one consideration is to just build this into
MAP_SYNC.MAP_SYNCcan only succeed if DAX is enabled and that it is a reliable way for userspace to assume DAX. There is some consensus for this proposal here:https://lists.01.org/pipermail/linux-nvdimm/2017-August/012086.html
That'll probe that it can be used, not that it is currently being used.
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 72 lines · Dave Chinner
Quoted 49 lines · Dan Williams
Quoted 48 lines · Dave Chinner
Quoted 8 lines · Ross Zwisler
On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
<>
Quoted 1 line
I think similar concerns exist with using perf, too....
I though that using perf addressed both concerns?
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So what happens when the user is already tracing the test to find a bug and the test turns all their tracing off?
By using perf we isolate our tracing from whatever other tracing is happening in the system. So, unlike the case where we were messing with a system-wide ftrace knob, we run perf on our executable, and someone else can run perf on their executable, and they don't collide.
Yes, you've addressed the "gets inteh way of other tracing concern, but it's doesn't address the "it's an ugly way to determine a feature is active" concerns. It also creates an implicit stable tracepoint out of whatever you are looking at. i.e. that tracepoint can't go away, nor can it change functionality once a userspace app depends on it's current semantics to function correctly.
And....
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regardless of this screwing up developer bug triage, do we really want to add a dependency on kernel tracing into the test harness?
Yep, you're right that this adds a dependency on perf. But unfortunately, without using either perf or ftrace, I don't know of a way to detect whether or not DAX is actually being used. Can you think of another way?
... if there isn't a programmatic interface to tell applications that DAX is in use, then perhaps we're missing a formal userspace API that we should have, yes?
Quoted 5 lines
I tried to do this correctly and just skip the test with
_notrunif perf isn't available on the host system. This is the same thing that happens if you are missing other dependencies for a test (some other command (chacl, getfattr, setfattr) not present, quota tools not installed, required users not present, etc).Sure, but if we have user configurable functionality, then using something like ftrace/perf to discover if it's turned is indicative of a bigger problem. i.e. that the user can't tell if the functionality they turned on/off is actually active or not.
Quoted 8 lines
Quoted 3 lines
That's a bug that needs fixing, not working around with ftrace/perf in xfstests...
Yes, we need a way to determine that DAX is enabled, but certainly not a
VM_DAXflag as that was NAKd by you here:https://lkml.org/lkml/2016/9/16/13Yeah, the on disk flag doesn't tell you if DAX is actually in use or not.
Quoted 6 lines
I think one consideration is to just build this into
MAP_SYNC.MAP_SYNCcan only succeed if DAX is enabled and that it is a reliable way for userspace to assume DAX. There is some consensus for this proposal here:https://lists.01.org/pipermail/linux-nvdimm/2017-August/012086.htmlThat'll probe that it can be used, not that it is currently being used.
Quoted 7 lines
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
Of course this is not how xfstests works and it definitely shouldn't, but perhaps this specific test case is better suited in the libnvdimm unit test framework where it's enabled to dig behind the standard interfaces.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 11 lines · Dan Williams
Quoted 7 lines · Dave Chinner
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
Quoted 7 lines
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
That's horrible. OT, but how are distros or anyone backporting libnvdimm fixes and features supposed to test their kernels work correctly with such a test harness?
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 20 lines · Dave Chinner
Quoted 11 lines · Dan Williams
Quoted 7 lines · Dave Chinner
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
Quoted 7 lines
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
Quoted 3 lines
That's horrible. OT, but how are distros or anyone backporting libnvdimm fixes and features supposed to test their kernels work correctly with such a test harness?
The upstream kernel version for the test to assume can be overridden by an environment variable. It has worked well so far for me when I'm using it it to test backports, but I don't have much in the way of third-party feedback.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 21 lines · Dan Williams
Quoted 20 lines · Dave Chinner
Quoted 18 lines · Dan Williams
Quoted 7 lines · Dave Chinner
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
Quoted 8 lines
Quoted 3 lines
That's horrible. OT, but how are distros or anyone backporting libnvdimm fixes and features supposed to test their kernels work correctly with such a test harness?
The upstream kernel version for the test to assume can be overridden by an environment variable. It has worked well so far for me when I'm using it it to test backports, but I don't have much in the way of third-party feedback.
It sucks. :-) What we really want is to depend on a feature being available, not on a kernel version. We did discuss this a while ago. Let me go dig it up...
https://lists.01.org/pipermail/linux-nvdimm/2017-March/009253.html
We never came to any real conclusion on a good way forward, though.
Cheers,
Jeff
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 32 lines · Jeff Moyer
Quoted 21 lines · Dan Williams
Quoted 20 lines · Dave Chinner
Quoted 18 lines · Dan Williams
Quoted 7 lines · Dave Chinner
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
Quoted 8 lines
Quoted 3 lines
That's horrible. OT, but how are distros or anyone backporting libnvdimm fixes and features supposed to test their kernels work correctly with such a test harness?
The upstream kernel version for the test to assume can be overridden by an environment variable. It has worked well so far for me when I'm using it it to test backports, but I don't have much in the way of third-party feedback.
Quoted 6 lines
It sucks. :-) What we really want is to depend on a feature being available, not on a kernel version. We did discuss this a while ago. Let me go dig it up...
https://lists.01.org/pipermail/linux-nvdimm/2017-March/009253.htmlWe never came to any real conclusion on a good way forward, though.
I think I already said this before [1], but can't we make a "features" sysfs/debugfs attribute with one bit set for each feature implemented? This definitively would help when running the test-suite on a backport.
[1] https://lists.01.org/pipermail/linux-nvdimm/2016-March/004963.html
Byte,
Johannes
Signature · 10 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 38 lines · Johannes Thumshirn
Quoted 37 lines · Jeff Moyer
Quoted 21 lines · Dan Williams
Quoted 20 lines · Dave Chinner
Quoted 18 lines · Dan Williams
Quoted 7 lines · Dave Chinner
/me shrugs
I just don't like the concept of using tracepoints to as a definitive diagnostic test for something working because it'll break when the kernel implementation and tracepoints change. So while we can probe for perf being present, we can't probe whether the tracepoint we need behaves as the test expects it to...
That concern makes sense.
We handle that it a crude way in the libnvdimm unit tests by hard coding a required minimum kernel version and rolling a test forward to depend on a new kernel when assumptions about the kernel-internals change. The tests also inject out-of-tree kernel modules that let us go after specific kernel internal behavior. With this approach we don't end up creating userspace ABI since the test explicitly loads out-of-tree modules.
Quoted 8 lines
Quoted 3 lines
That's horrible. OT, but how are distros or anyone backporting libnvdimm fixes and features supposed to test their kernels work correctly with such a test harness?
The upstream kernel version for the test to assume can be overridden by an environment variable. It has worked well so far for me when I'm using it it to test backports, but I don't have much in the way of third-party feedback.
It sucks. :-) What we really want is to depend on a feature being available, not on a kernel version. We did discuss this a while ago. Let me go dig it up...
https://lists.01.org/pipermail/linux-nvdimm/2017-March/009253.html
Quoted 7 lines
Quoted 1 line
We never came to any real conclusion on a good way forward, though.
I think I already said this before [1], but can't we make a "features" sysfs/debugfs attribute with one bit set for each feature implemented? This definitively would help when running the test-suite on a backport.
[1] https://lists.01.org/pipermail/linux-nvdimm/2016-March/004963.html
What discouraged me from going that route in the past is the busy work of tracking / syncing these new debugfs feature gate flags across 2 source repositories. If we want to stop depending on kernel version in the test suite over time I think the only sane way to manage that tight integration is to get ndctl into the kernel tree proper.
...but please say a bit more about the actual pain points with the current environment variable approach. You want to be able to have a debugfs directory that has something like:
/featureA
/featureB
/featureC
/fixX
/fixY
/fixZ
/quirkQ
...where, depending on backport priorities, a subset of those may not exist? Does having the test suite in the kernel tree help or hurt what you're trying to do?
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 8 lines · Dan Williams
What discouraged me from going that route in the past is the busy work of tracking / syncing these new debugfs feature gate flags across 2 source repositories. If we want to stop depending on kernel version in the test suite over time I think the only sane way to manage that tight integration is to get ndctl into the kernel tree proper.
...but please say a bit more about the actual pain points with the current environment variable approach.
This means the person which executes the test-suite has to know on which feature level (upstream kernel version) the downstream kernel is, I can't demand such knowledge from QA.
Quoted 10 lines
You want to be able to have a debugfs directory that has something like:
/featureA
/featureB
/featureC
/fixX
/fixY
/fixZ
/quirkQ
Quoted 2 lines
...where, depending on backport priorities, a subset of those may not exist?
Yes, I thought of a bitmask in one (or two files but a file per feature is OK) as well. The idea is borrowed from Daniel Vetter's "Bothing up ioctl()s" blog entry [1]:
"Have a clear way for userspace to figure out whether your new ioctl or ioctl extension is supported on a given kernel. If you can't rely on old kernels rejecting the new flags/modes or ioctls (since doing that was botched in the past) then you need a driver feature flag or revision number somewhere."
Quoted 2 lines
Does having the test suite in the kernel tree help or hurt what you're trying to do?
Having the test suite in the kernel is problematic when you want to distribute it to somewhere. I can only talk about my workflow here, but I build an rpm on my build server and then scp it to my test host. With the current test-suite I have to bring the other modules over as well and install them before the kernel rpm to have the linker wrapper trick working. That's OKish for developer testing, but for QA (or even testing at partners or customers) this gets cumbersome and I really want to have even end customers being able to run the test-suite to verify their kernel is working properly.
[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
Byte,
Johannes
Signature · 10 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 7 lines · Johannes Thumshirn
Quoted 6 lines · Dan Williams
What discouraged me from going that route in the past is the busy work of tracking / syncing these new debugfs feature gate flags across 2 source repositories. If we want to stop depending on kernel version in the test suite over time I think the only sane way to manage that tight integration is to get ndctl into the kernel tree proper.
Quoted 6 lines
Quoted 2 lines
...but please say a bit more about the actual pain points with the current environment variable approach.
This means the person which executes the test-suite has to know on which feature level (upstream kernel version) the downstream kernel is, I can't demand such knowledge from QA.
Ok, that makes sense. The libnvdimm unit test suite needs to grow up into something QA can run and not require developers with internals knowledge.
Quoted 14 lines
Quoted 11 lines
You want to be able to have a debugfs directory that has something like:
/featureA
/featureB
/featureC
/fixX
/fixY
/fixZ
/quirkQQuoted 2 lines
...where, depending on backport priorities, a subset of those may not exist?
Quoted 8 lines
Yes, I thought of a bitmask in one (or two files but a file per feature is OK) as well. The idea is borrowed from Daniel Vetter's "Bothing up ioctl()s" blog entry [1]:
"Have a clear way for userspace to figure out whether your new ioctl or ioctl extension is supported on a given kernel. If you can't rely on old kernels rejecting the new flags/modes or ioctls (since doing that was botched in the past) then you need a driver feature flag or revision number somewhere."
Yes, but what we're worried about in the libnvdimm/nfit_test case is internals behind sysfs interfaces. But point taken, we can at least use the presence of certain attributes to gate tests. However, it does mean we lose the tests for when an attribute is missing due to an incomplete backport.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] xfs: add regression test for DAX mount option usage
Quoted 5 lines · Dan Williams
Yes, but what we're worried about in the libnvdimm/nfit_test case is internals behind sysfs interfaces. But point taken, we can at least use the presence of certain attributes to gate tests. However, it does mean we lose the tests for when an attribute is missing due to an incomplete backport.
Of cause, but all we loose is a group of testcases to be run and this can be an indication of a bad backport as well, so I think this is quite helpful.
Byte,
Johannes
Signature · 10 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 2/2 xfs: validate bdev support for DAX inode flag
In reply to Ross ZwislerCurrently only the blocksize is checked, but we should really be calling bdev_dax_supported() which also tests to make sure we can get a struct dax_device and that the dax_direct_access() path is working.
This is the same check that we do for the "-o dax" mount option in xfs_fs_fill_super().
This does not fix the race issues that caused the XFS DAX inode option to be disabled, so that option will still be disabled. If/when we re-enable it, though, I think we will want this issue to have been fixed. I also do think that we want to fix this in stable kernels.
--- fs/xfs/xfs_ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/xfs/xfs_ioctl.c +2 -1
@@ -1088,6 +1088,7 @@ xfs_ioctl_setattr_dax_invalidate( int *join_flags) {struct inode *inode =VFS_I (ip); +struct super_block *sb = inode->i_sb;int error; *join_flags =0 ;
@@ -1100,7 +1101,7 @@ xfs_ioctl_setattr_dax_invalidate(if (fa->fsx_xflags &FS_XFLAG_DAX ) {if (!(S_ISREG (inode->i_mode) ||S_ISDIR (inode->i_mode)))return -EINVAL ; -if (ip->i_mount->m_sb.sb_blocksize !=PAGE_SIZE ) +if (bdev_dax_supported (sb, sb->s_blocksize) <0 )return -EINVAL ; }
Signature · 6 lines
2.9.5 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/2] xfs: validate bdev support for DAX inode flag
Looks good,
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm