| Message ID | 20200414040030.1802884-7-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ext4 support for per-file/directory DAX operations | expand |
On Mon 13-04-20 21:00:28, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Change the logic of ext4_should_use_dax() to support using the inode dax > flag OR the overriding tri-state mount option. > > While we are at it change the function to ext4_enable_dax() as this > better reflects the ask. I disagree with the renaming. ext4_enable_dax() suggests it enables something. It does not. I'd either leave ext4_should_use_dax() or maybe change it to ext4_should_enable_dax() if you really like the "enable" word :). > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > fs/ext4/inode.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index fa0ff78dc033..e9d582e516bc 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4383,9 +4383,11 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc) > !ext4_test_inode_state(inode, EXT4_STATE_XATTR)); > } > > -static bool ext4_should_use_dax(struct inode *inode) > +static bool ext4_enable_dax(struct inode *inode) > { > - if (!test_opt(inode->i_sb, DAX)) > + unsigned int flags = EXT4_I(inode)->i_flags; > + > + if (test_opt2(inode->i_sb, NODAX)) > return false; > if (!S_ISREG(inode->i_mode)) > return false; > @@ -4397,7 +4399,13 @@ static bool ext4_should_use_dax(struct inode *inode) > return false; > if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY)) > return false; > - return true; > + if (!bdev_dax_supported(inode->i_sb->s_bdev, > + inode->i_sb->s_blocksize)) > + return false; > + if (test_opt(inode->i_sb, DAX)) > + return true; > + > + return (flags & EXT4_DAX_FL) == EXT4_DAX_FL; flags & EXT4_DAX_FL is enough here, isn't it? Honza > } > > void ext4_set_inode_flags(struct inode *inode) > @@ -4415,7 +4423,7 @@ void ext4_set_inode_flags(struct inode *inode) > new_fl |= S_NOATIME; > if (flags & EXT4_DIRSYNC_FL) > new_fl |= S_DIRSYNC; > - if (ext4_should_use_dax(inode)) > + if (ext4_enable_dax(inode)) > new_fl |= S_DAX; > if (flags & EXT4_ENCRYPT_FL) > new_fl |= S_ENCRYPTED; > -- > 2.25.1 >
On Wed, Apr 15, 2020 at 03:58:34PM +0200, Jan Kara wrote: > On Mon 13-04-20 21:00:28, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Change the logic of ext4_should_use_dax() to support using the inode dax > > flag OR the overriding tri-state mount option. > > > > While we are at it change the function to ext4_enable_dax() as this > > better reflects the ask. > > I disagree with the renaming. ext4_enable_dax() suggests it enables > something. It does not. I'd either leave ext4_should_use_dax() or maybe > change it to ext4_should_enable_dax() if you really like the "enable" word > :). Ok that does sound better. And I've changed it in the xfs series as well. but I kept Darrick's review on that patch... > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > fs/ext4/inode.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index fa0ff78dc033..e9d582e516bc 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -4383,9 +4383,11 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc) > > !ext4_test_inode_state(inode, EXT4_STATE_XATTR)); > > } > > > > -static bool ext4_should_use_dax(struct inode *inode) > > +static bool ext4_enable_dax(struct inode *inode) > > { > > - if (!test_opt(inode->i_sb, DAX)) > > + unsigned int flags = EXT4_I(inode)->i_flags; > > + > > + if (test_opt2(inode->i_sb, NODAX)) > > return false; > > if (!S_ISREG(inode->i_mode)) > > return false; > > @@ -4397,7 +4399,13 @@ static bool ext4_should_use_dax(struct inode *inode) > > return false; > > if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY)) > > return false; > > - return true; > > + if (!bdev_dax_supported(inode->i_sb->s_bdev, > > + inode->i_sb->s_blocksize)) > > + return false; > > + if (test_opt(inode->i_sb, DAX)) > > + return true; > > + > > + return (flags & EXT4_DAX_FL) == EXT4_DAX_FL; > > flags & EXT4_DAX_FL is enough here, isn't it? Yes, changed. Ira > > Honza > > > } > > > > void ext4_set_inode_flags(struct inode *inode) > > @@ -4415,7 +4423,7 @@ void ext4_set_inode_flags(struct inode *inode) > > new_fl |= S_NOATIME; > > if (flags & EXT4_DIRSYNC_FL) > > new_fl |= S_DIRSYNC; > > - if (ext4_should_use_dax(inode)) > > + if (ext4_enable_dax(inode)) > > new_fl |= S_DAX; > > if (flags & EXT4_ENCRYPT_FL) > > new_fl |= S_ENCRYPTED; > > -- > > 2.25.1 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index fa0ff78dc033..e9d582e516bc 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4383,9 +4383,11 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc) !ext4_test_inode_state(inode, EXT4_STATE_XATTR)); } -static bool ext4_should_use_dax(struct inode *inode) +static bool ext4_enable_dax(struct inode *inode) { - if (!test_opt(inode->i_sb, DAX)) + unsigned int flags = EXT4_I(inode)->i_flags; + + if (test_opt2(inode->i_sb, NODAX)) return false; if (!S_ISREG(inode->i_mode)) return false; @@ -4397,7 +4399,13 @@ static bool ext4_should_use_dax(struct inode *inode) return false; if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY)) return false; - return true; + if (!bdev_dax_supported(inode->i_sb->s_bdev, + inode->i_sb->s_blocksize)) + return false; + if (test_opt(inode->i_sb, DAX)) + return true; + + return (flags & EXT4_DAX_FL) == EXT4_DAX_FL; } void ext4_set_inode_flags(struct inode *inode) @@ -4415,7 +4423,7 @@ void ext4_set_inode_flags(struct inode *inode) new_fl |= S_NOATIME; if (flags & EXT4_DIRSYNC_FL) new_fl |= S_DIRSYNC; - if (ext4_should_use_dax(inode)) + if (ext4_enable_dax(inode)) new_fl |= S_DAX; if (flags & EXT4_ENCRYPT_FL) new_fl |= S_ENCRYPTED;