Re: [PATCH v3 00/12] Enable per-file/directory DAX operations V3

From: Ira Weiny <ira.weiny@intel.com>
To: Jeff Moyer <jmoyer@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@lst.de>,
	"Theodore Y. Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
	linux-ext4 <linux-ext4@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v3 00/12] Enable per-file/directory DAX operations V3
Date: Thu, 20 Feb 2020 08:20:28 -0800	[thread overview]
Message-ID: <20200220162027.GA20772@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200218235429.GB14509@iweiny-DESK2.sc.intel.com>

On Tue, Feb 18, 2020 at 03:54:30PM -0800, 'Ira Weiny' wrote:
> On Tue, Feb 18, 2020 at 09:22:58AM -0500, Jeff Moyer wrote:
> > Ira Weiny <ira.weiny@intel.com> writes:
> > > If my disassembly of read_pages is correct it looks like readpage is null which
> > > makes sense because all files should be IS_DAX() == true due to the mount option...
> > >
> > > But tracing code indicates that the patch:
> > >
> > > 	fs: remove unneeded IS_DAX() check
> > >
> > > ... may be the culprit and the following fix may work...
> > >
> > > diff --git a/mm/filemap.c b/mm/filemap.c
> > > index 3a7863ba51b9..7eaf74a2a39b 100644
> > > --- a/mm/filemap.c
> > > +++ b/mm/filemap.c
> > > @@ -2257,7 +2257,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> > >         if (!count)
> > >                 goto out; /* skip atime */
> > >  
> > > -       if (iocb->ki_flags & IOCB_DIRECT) {
> > > +       if (iocb->ki_flags & IOCB_DIRECT || IS_DAX(inode)) {
> > >                 struct file *file = iocb->ki_filp;
> > >                 struct address_space *mapping = file->f_mapping;
> > >                 struct inode *inode = mapping->host;
> > 
> > Well, you'll have to up-level the inode variable instantiation,
> > obviously.  That solves this particular issue.
> 
> Well...  This seems to be a random issue.  I've had BMC issues with
> my server most of the day...  But even with this patch I still get the failure
> in read_pages().  :-/
> 
> And I have gotten it to both succeed and fail with qemu...  :-/

... here is the fix.  I made the change in xfs_diflags_to_linux() early on with
out factoring in the flag logic changes we have agreed upon...

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 62d9f622bad1..d592949ad396 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1123,11 +1123,11 @@ xfs_diflags_to_linux(
                inode->i_flags |= S_NOATIME;
        else
                inode->i_flags &= ~S_NOATIME;
-       if (xflags & FS_XFLAG_DAX)
+
+       if (xfs_inode_enable_dax(ip))
                inode->i_flags |= S_DAX;
        else
                inode->i_flags &= ~S_DAX;
-
 }

But the one thing which tripped me up, and concerns me, is we have 2 functions
which set the inode flags.

xfs_diflags_to_iflags()
xfs_diflags_to_linux()

xfs_diflags_to_iflags() is geared toward initialization but logically they do
the same thing.  I see no reason to keep them separate.  Does anyone?

Based on this find, the discussion on behavior in this thread, and the comments
from Dave I'm reworking the series because the flag check/set functions have
all changed and I really want to be as clear as possible with both the patches
and the resulting code.[*]  So v4 should be out today including attempting to
document what we have discussed here and being as clear as possible on the
behavior.  :-D

Thanks so much for testing this!

Ira

[*] I will probably throw in a patch to remove xfs_diflags_to_iflags() as I
really don't see a reason to keep it.

