[v3,05/12] fs: remove unneeded IS_DAX() check
diff mbox series

Message ID 20200208193445.27421-6-ira.weiny@intel.com
State Not Applicable
Headers show
Series
  • Enable per-file/directory DAX operations V3
Related show

Commit Message

Ira Weiny Feb. 8, 2020, 7:34 p.m. UTC
From: Ira Weiny <ira.weiny@intel.com>

The IS_DAX() check in io_is_direct() causes a race between changing the
DAX state and creating the iocb flags.

Remove the check because DAX now emulates the page cache API and
therefore it does not matter if the file state is DAX or not when the
iocb flags are created.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/linux/fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Chinner Feb. 11, 2020, 5:34 a.m. UTC | #1
On Sat, Feb 08, 2020 at 11:34:38AM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> The IS_DAX() check in io_is_direct() causes a race between changing the
> DAX state and creating the iocb flags.
> 
> Remove the check because DAX now emulates the page cache API and
> therefore it does not matter if the file state is DAX or not when the
> iocb flags are created.

This statement is ... weird.

DAX doesn't "emulate" the page cache API at all - it has it's own
read/write methods that filesystems call based on the iomap
infrastructure (dax_iomap_rw()). i.e. there are 3 different IO paths
through the filesystems: the DAX IO path, the direct IO path, and
the buffered IO path.

Indeed, it seems like this works a bit by luck: Ext4 and XFS always
check IS_DAX(inode) in the read/write_iter methods before checking
for IOCB_DIRECT, and hence the IOCB_DIRECT flag is ignored by the
filesystems. i.e. when we got rid of the O_DIRECT paths from DAX, we
forgot to clean up io_is_direct() and it's only due to the ordering
of checks that we went down the DAX path correctly....

That said, the code change is good, but the commit message needs a
rewrite.

Cheers,

Dave.
Ira Weiny Feb. 11, 2020, 4:38 p.m. UTC | #2
On Tue, Feb 11, 2020 at 04:34:01PM +1100, Dave Chinner wrote:
> On Sat, Feb 08, 2020 at 11:34:38AM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The IS_DAX() check in io_is_direct() causes a race between changing the
> > DAX state and creating the iocb flags.
> > 
> > Remove the check because DAX now emulates the page cache API and
> > therefore it does not matter if the file state is DAX or not when the
> > iocb flags are created.
> 
> This statement is ... weird.
> 
> DAX doesn't "emulate" the page cache API at all

ah...  yea emulate is a bad word here.

> - it has it's own
> read/write methods that filesystems call based on the iomap
> infrastructure (dax_iomap_rw()). i.e. there are 3 different IO paths
> through the filesystems: the DAX IO path, the direct IO path, and
> the buffered IO path.
> 
> Indeed, it seems like this works a bit by luck: Ext4 and XFS always
> check IS_DAX(inode) in the read/write_iter methods before checking
> for IOCB_DIRECT, and hence the IOCB_DIRECT flag is ignored by the
> filesystems. i.e. when we got rid of the O_DIRECT paths from DAX, we
> forgot to clean up io_is_direct() and it's only due to the ordering
> of checks that we went down the DAX path correctly....
> 
> That said, the code change is good, but the commit message needs a
> rewrite.

How about?

<commit msg>
  fs: Remove unneeded IS_DAX() check
  
  The IS_DAX() check in io_is_direct() causes a race between changing the
  DAX state and creating the iocb flags.

  Remove the check because DAX now has it's own read/write methods and
  file systems which support DAX check IS_DAX() prior to IOCB_DIRECT.
  Therefore, it does not matter if the file state is DAX when the iocb
  flags are created, and we can avoid the race.

  Reviewed-by: Jan Kara <jack@suse.cz>
  Signed-off-by: Ira Weiny <ira.weiny@intel.com>
</commit msg>

Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner Feb. 11, 2020, 8:41 p.m. UTC | #3
On Tue, Feb 11, 2020 at 08:38:31AM -0800, Ira Weiny wrote:
> On Tue, Feb 11, 2020 at 04:34:01PM +1100, Dave Chinner wrote:
> > On Sat, Feb 08, 2020 at 11:34:38AM -0800, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > The IS_DAX() check in io_is_direct() causes a race between changing the
> > > DAX state and creating the iocb flags.
> > > 
> > > Remove the check because DAX now emulates the page cache API and
> > > therefore it does not matter if the file state is DAX or not when the
> > > iocb flags are created.
> > 
> > This statement is ... weird.
> > 
> > DAX doesn't "emulate" the page cache API at all
> 
> ah...  yea emulate is a bad word here.
> 
> > - it has it's own
> > read/write methods that filesystems call based on the iomap
> > infrastructure (dax_iomap_rw()). i.e. there are 3 different IO paths
> > through the filesystems: the DAX IO path, the direct IO path, and
> > the buffered IO path.
> > 
> > Indeed, it seems like this works a bit by luck: Ext4 and XFS always
> > check IS_DAX(inode) in the read/write_iter methods before checking
> > for IOCB_DIRECT, and hence the IOCB_DIRECT flag is ignored by the
> > filesystems. i.e. when we got rid of the O_DIRECT paths from DAX, we
> > forgot to clean up io_is_direct() and it's only due to the ordering
> > of checks that we went down the DAX path correctly....
> > 
> > That said, the code change is good, but the commit message needs a
> > rewrite.
> 
> How about?
> 
> <commit msg>
>   fs: Remove unneeded IS_DAX() check
>   
>   The IS_DAX() check in io_is_direct() causes a race between changing the
>   DAX state and creating the iocb flags.

This is irrelevant - the check is simply wrong and has been since
~2016 when we moved DAX to use the iomap infrastructure...

Cheers,

Dave.
Ira Weiny Feb. 12, 2020, 4:04 p.m. UTC | #4
On Wed, Feb 12, 2020 at 07:41:07AM +1100, Dave Chinner wrote:
> On Tue, Feb 11, 2020 at 08:38:31AM -0800, Ira Weiny wrote:
> > On Tue, Feb 11, 2020 at 04:34:01PM +1100, Dave Chinner wrote:
> > > On Sat, Feb 08, 2020 at 11:34:38AM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > The IS_DAX() check in io_is_direct() causes a race between changing the
> > > > DAX state and creating the iocb flags.
> > > > 
> > > > Remove the check because DAX now emulates the page cache API and
> > > > therefore it does not matter if the file state is DAX or not when the
> > > > iocb flags are created.
> > > 
> > > This statement is ... weird.
> > > 
> > > DAX doesn't "emulate" the page cache API at all
> > 
> > ah...  yea emulate is a bad word here.
> > 
> > > - it has it's own
> > > read/write methods that filesystems call based on the iomap
> > > infrastructure (dax_iomap_rw()). i.e. there are 3 different IO paths
> > > through the filesystems: the DAX IO path, the direct IO path, and
> > > the buffered IO path.
> > > 
> > > Indeed, it seems like this works a bit by luck: Ext4 and XFS always
> > > check IS_DAX(inode) in the read/write_iter methods before checking
> > > for IOCB_DIRECT, and hence the IOCB_DIRECT flag is ignored by the
> > > filesystems. i.e. when we got rid of the O_DIRECT paths from DAX, we
> > > forgot to clean up io_is_direct() and it's only due to the ordering
> > > of checks that we went down the DAX path correctly....
> > > 
> > > That said, the code change is good, but the commit message needs a
> > > rewrite.
> > 
> > How about?
> > 
> > <commit msg>
> >   fs: Remove unneeded IS_DAX() check
> >   
> >   The IS_DAX() check in io_is_direct() causes a race between changing the
> >   DAX state and creating the iocb flags.
> 
> This is irrelevant - the check is simply wrong and has been since
> ~2016 when we moved DAX to use the iomap infrastructure...

Deleted.
Ira

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com

Patch
diff mbox series

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3cd4fe6b845e..63d1e533a07d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3388,7 +3388,7 @@  extern int file_update_time(struct file *file);
 
 static inline bool io_is_direct(struct file *filp)
 {
-	return (filp->f_flags & O_DIRECT) || IS_DAX(filp->f_mapping->host);
+	return (filp->f_flags & O_DIRECT);
 }
 
 static inline bool vma_is_dax(struct vm_area_struct *vma)