Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
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
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.


  reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-08 19:34 ira.weiny
2020-02-08 19:34 ` [PATCH v3 01/12] fs/stat: Define DAX statx attribute ira.weiny
2020-02-08 19:34 ` [PATCH v3 02/12] fs/xfs: Isolate the physical DAX flag from effective ira.weiny
2020-02-08 19:34 ` [PATCH v3 03/12] fs/xfs: Separate functionality of xfs_inode_supports_dax() ira.weiny
2020-02-11  5:47   ` Dave Chinner
2020-02-11 16:13     ` Ira Weiny
2020-02-08 19:34 ` [PATCH v3 04/12] fs/xfs: Clean up DAX support check ira.weiny
2020-02-11  5:57   ` Dave Chinner
2020-02-11 16:28     ` Ira Weiny
2020-02-11 20:38       ` Dave Chinner
2020-02-08 19:34 ` [PATCH v3 05/12] fs: remove unneeded IS_DAX() check ira.weiny
2020-02-11  5:34   ` Dave Chinner
2020-02-11 16:38     ` Ira Weiny
2020-02-11 20:41       ` Dave Chinner
2020-02-12 16:04         ` Ira Weiny
2020-02-08 19:34 ` [PATCH v3 06/12] fs/xfs: Check if the inode supports DAX under lock ira.weiny
2020-02-11  6:16   ` Dave Chinner
2020-02-11 17:55     ` Ira Weiny
2020-02-11 20:42       ` Dave Chinner
2020-02-12 16:10         ` Ira Weiny
2020-02-08 19:34 ` [PATCH v3 07/12] fs: Add locking for a dynamic DAX state ira.weiny
2020-02-11  8:00   ` Dave Chinner
2020-02-11 20:14     ` Ira Weiny
2020-02-11 20:59       ` Dan Williams
2020-02-11 21:49       ` Dave Chinner
2020-02-12  6:31         ` Darrick J. Wong
2020-02-08 19:34 ` [PATCH v3 08/12] fs/xfs: Clarify lockdep dependency for xfs_isilocked() ira.weiny
2020-02-08 19:34 ` [PATCH v3 09/12] fs/xfs: Add write DAX lock to xfs layer ira.weiny
2020-02-08 19:34 ` [PATCH v3 10/12] fs: Prevent DAX state change if file is mmap'ed ira.weiny
2020-02-08 19:34 ` [PATCH v3 11/12] fs/xfs: Clean up locking in dax invalidate ira.weiny
2020-02-08 19:34 ` [PATCH v3 12/12] fs/xfs: Allow toggle of effective DAX flag ira.weiny
2020-02-10 15:15 ` [PATCH v3 00/12] Enable per-file/directory DAX operations V3 Jeff Moyer
2020-02-11 20:17   ` Ira Weiny
2020-02-12 19:49     ` Jeff Moyer
2020-02-13 19:01       ` Ira Weiny
2020-02-13 19:05         ` Ira Weiny
2020-02-13 19:58           ` Darrick J. Wong
2020-02-13 23:29             ` Ira Weiny
2020-02-14  0:16               ` Dan Williams
2020-02-14 20:06                 ` Ira Weiny
2020-02-14 21:23                   ` Jeff Moyer
2020-02-14 21:58                     ` Ira Weiny
2020-02-14 22:06                       ` Jeff Moyer
2020-02-14 22:58                         ` Jeff Moyer
2020-02-14 23:03                           ` Jeff Moyer
2020-02-18  2:35                           ` Ira Weiny
2020-02-18 14:22                             ` Jeff Moyer
2020-02-18 23:54                               ` Ira Weiny
2020-02-20 16:20                                 ` Ira Weiny [this message]
2020-02-20 16:30                                   ` Darrick J. Wong
2020-02-20 16:49                                     ` Ira Weiny
2020-02-20 17:00                                       ` Darrick J. Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200220162027.GA20772@iweiny-DESK2.sc.intel.com \
    --to=ira.weiny@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=jack@suse.cz \
    --cc=jmoyer@redhat.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git