| Message ID | 20170907210832.17969-2-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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!
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))
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(-)