[PATCH 0/2] xfs: some DAX fixes

[PATCH 0/2] xfs: some DAX fixes

[Ross Zwisler]

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

-- 
2.9.5

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/2] xfs: always use DAX if mount option is used

[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_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.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/xfs/xfs_ioctl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5049e8a..26faeb9 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -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))
-- 
2.9.5

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] xfs: always use DAX if mount option is used

[Christoph Hellwig]

On Thu, Sep 07, 2017 at 03:08:31PM -0600, Ross Zwisler wrote:
> 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.

Looks good, but can you please add a testcase to xfstests for this?

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] xfs: always use DAX if mount option is used

[Ross Zwisler]

On Fri, Sep 08, 2017 at 12:20:28AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 07, 2017 at 03:08:31PM -0600, Ross Zwisler wrote:
> > 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.
> 
> 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?

> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for the review!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] xfs: add regression test for DAX mount option usage

[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 <ross.zwisler@linux.intel.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
---
 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

diff --git a/tests/xfs/431 b/tests/xfs/431
new file mode 100755
index 0000000..4ff3a02
--- /dev/null
+++ b/tests/xfs/431
@@ -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 <ross.zwisler@linux.intel.com>
+#
+#-----------------------------------------------------------------------
+# 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
diff --git a/tests/xfs/431.out b/tests/xfs/431.out
new file mode 100644
index 0000000..265dc46
--- /dev/null
+++ b/tests/xfs/431.out
@@ -0,0 +1,3 @@
+QA output created by 431
+DAX was used
+Silence is golden
diff --git a/tests/xfs/group b/tests/xfs/group
index 0a449b9..4e7019c 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -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
-- 
2.9.5

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Ross Zwisler]

On Fri, Sep 08, 2017 at 03:21:53PM -0600, Ross Zwisler wrote:
> 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 <ross.zwisler@linux.intel.com>
> Suggested-by: Christoph Hellwig <hch@infradead.org>

<>

> +# 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Mon, Sep 11, 2017 at 8:16 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Fri, Sep 08, 2017 at 03:21:53PM -0600, Ross Zwisler wrote:
>> 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 <ross.zwisler@linux.intel.com>
>> Suggested-by: Christoph Hellwig <hch@infradead.org>
>
> <>
>
>> +# 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.

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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[fstests PATCH v2] xfs: add regression test for DAX mount option usage

[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 <ross.zwisler@linux.intel.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
---

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

diff --git a/tests/xfs/431 b/tests/xfs/431
new file mode 100755
index 0000000..2865a6d
--- /dev/null
+++ b/tests/xfs/431
@@ -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 <ross.zwisler@linux.intel.com>
+#
+#-----------------------------------------------------------------------
+# 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
diff --git a/tests/xfs/431.out b/tests/xfs/431.out
new file mode 100644
index 0000000..194ae1e
--- /dev/null
+++ b/tests/xfs/431.out
@@ -0,0 +1,2 @@
+QA output created by 431
+DAX was used
diff --git a/tests/xfs/group b/tests/xfs/group
index 0a449b9..4e7019c 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -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
-- 
2.9.5

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage

[Eryu Guan]

Hi Ross,

On Mon, Sep 11, 2017 at 02:01:03PM -0600, Ross Zwisler wrote:
> 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 <ross.zwisler@linux.intel.com>
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> ---
> 
> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage

[Ross Zwisler]

On Thu, Sep 14, 2017 at 02:57:41PM +0800, Eryu Guan wrote:
> Hi Ross,
> 
> On Mon, Sep 11, 2017 at 02:01:03PM -0600, Ross Zwisler wrote:
> > 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 <ross.zwisler@linux.intel.com>
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > ---
> > 
> > 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

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?

> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [fstests PATCH v2] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Fri, Sep 15, 2017 at 04:42:27PM -0600, Ross Zwisler wrote:
> On Thu, Sep 14, 2017 at 02:57:41PM +0800, Eryu Guan wrote:
> > Hi Ross,
> > 
> > On Mon, Sep 11, 2017 at 02:01:03PM -0600, Ross Zwisler wrote:
> > > 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 <ross.zwisler@linux.intel.com>
> > > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > > ---
> > > 
> > > 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.

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.

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

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.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Fri, Sep 08, 2017 at 03:21:53PM -0600, Ross Zwisler wrote:
> 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 <ross.zwisler@linux.intel.com>
> Suggested-by: Christoph Hellwig <hch@infradead.org>
.....
> +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

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.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Ross Zwisler]

On Tue, Sep 12, 2017 at 04:44:11PM +1000, Dave Chinner wrote:
> On Fri, Sep 08, 2017 at 03:21:53PM -0600, Ross Zwisler wrote:
> > 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 <ross.zwisler@linux.intel.com>
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> .....
> > +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
> 
> 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?

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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Tue, Sep 12, 2017 at 09:38:20AM -0600, Ross Zwisler wrote:
> On Tue, Sep 12, 2017 at 04:44:11PM +1000, Dave Chinner wrote:
> > On Fri, Sep 08, 2017 at 03:21:53PM -0600, Ross Zwisler wrote:
> > > 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 <ross.zwisler@linux.intel.com>
> > > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > .....
> > > +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
> > 
> > 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?
> 
> 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

I think similar concerns exist with using perf, too....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Ross Zwisler]

On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:

<>

> I think similar concerns exist with using perf, too....

I though that using perf addressed both concerns?

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

> > > 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Wed, Sep 13, 2017 at 08:42:15AM -0600, Ross Zwisler wrote:
> On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
> 
> <>
> 
> > I think similar concerns exist with using perf, too....
> 
> I though that using perf addressed both concerns?
> 
> > > > 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....

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

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

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.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Wed, Sep 13, 2017 at 3:01 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Sep 13, 2017 at 08:42:15AM -0600, Ross Zwisler wrote:
>> On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
>>
>> <>
>>
>> > I think similar concerns exist with using perf, too....
>>
>> I though that using perf addressed both concerns?
>>
>> > > > 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....
>
>> > > > 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?
>
>> 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).
>
> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Wed, Sep 13, 2017 at 03:23:39PM -0700, Dan Williams wrote:
> On Wed, Sep 13, 2017 at 3:01 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Sep 13, 2017 at 08:42:15AM -0600, Ross Zwisler wrote:
> >> On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
> >>
> >> <>
> >>
> >> > I think similar concerns exist with using perf, too....
> >>
> >> I though that using perf addressed both concerns?
> >>
> >> > > > 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....
> >
> >> > > > 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?
> >
> >> 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).
> >
> > 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

Yeah, the on disk flag doesn't tell you if DAX is actually in use or
not.

> 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

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.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Sep 13, 2017 at 03:23:39PM -0700, Dan Williams wrote:
>> On Wed, Sep 13, 2017 at 3:01 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Wed, Sep 13, 2017 at 08:42:15AM -0600, Ross Zwisler wrote:
>> >> On Wed, Sep 13, 2017 at 09:47:29AM +1000, Dave Chinner wrote:
>> >>
>> >> <>
>> >>
>> >> > I think similar concerns exist with using perf, too....
>> >>
>> >> I though that using perf addressed both concerns?
>> >>
>> >> > > > 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....
>> >
>> >> > > > 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?
>> >
>> >> 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).
>> >
>> > 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
>
> Yeah, the on disk flag doesn't tell you if DAX is actually in use or
> not.
>
>> 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
>
> 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...

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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dave Chinner]

On Wed, Sep 13, 2017 at 05:28:39PM -0700, Dan Williams wrote:
> On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
> > /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.

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.
-- 
Dave Chinner
david@fromorbit.com
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Wed, Sep 13, 2017 at 5:40 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Sep 13, 2017 at 05:28:39PM -0700, Dan Williams wrote:
>> On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > /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.
>
> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> On Wed, Sep 13, 2017 at 5:40 PM, Dave Chinner <david@fromorbit.com> wrote:
>> On Wed, Sep 13, 2017 at 05:28:39PM -0700, Dan Williams wrote:
>>> On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
>>> > /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.
>>
>> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Johannes Thumshirn]

On Thu, Sep 14, 2017 at 08:19:58AM -0400, Jeff Moyer wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > On Wed, Sep 13, 2017 at 5:40 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> On Wed, Sep 13, 2017 at 05:28:39PM -0700, Dan Williams wrote:
> >>> On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
> >>> > /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.
> >>
> >> 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.

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
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Thu, Sep 14, 2017 at 6:16 AM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On Thu, Sep 14, 2017 at 08:19:58AM -0400, Jeff Moyer wrote:
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > On Wed, Sep 13, 2017 at 5:40 PM, Dave Chinner <david@fromorbit.com> wrote:
>> >> On Wed, Sep 13, 2017 at 05:28:39PM -0700, Dan Williams wrote:
>> >>> On Wed, Sep 13, 2017 at 4:34 PM, Dave Chinner <david@fromorbit.com> wrote:
>> >>> > /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.
>> >>
>> >> 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.
>
> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Johannes Thumshirn]

On Thu, Sep 14, 2017 at 07:10:02AM -0700, Dan Williams wrote:
> 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.

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

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

> 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

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Dan Williams]

On Fri, Sep 15, 2017 at 2:18 AM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On Thu, Sep 14, 2017 at 07:10:02AM -0700, Dan Williams wrote:
>> 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.

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.

>
>> 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?
>
> 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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] xfs: add regression test for DAX mount option usage

[Johannes Thumshirn]

On Fri, Sep 15, 2017 at 10:39:05AM -0700, Dan Williams wrote:
> 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
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/2] xfs: validate bdev support for DAX inode flag

[Ross Zwisler]

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

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/xfs/xfs_ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 26faeb9..0433aef 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -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;
 	}
 
-- 
2.9.5

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] xfs: validate bdev support for DAX inode flag

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm