From: Ira Weiny <[email protected]>
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 <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
include/linux/fs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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)
--
2.21.0
On Sat, Feb 08, 2020 at 11:34:38AM -0800, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> 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.
--
Dave Chinner
[email protected]
On Tue, Feb 11, 2020 at 04:34:01PM +1100, Dave Chinner wrote:
> On Sat, Feb 08, 2020 at 11:34:38AM -0800, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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 <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
</commit msg>
Ira
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]