On Wed, May 13, 2020 at 04:47:06PM +0200, Jan Kara wrote:
> On Tue 12-05-20 22:43:23, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > Add a flag to preserve FS_XFLAG_DAX in the ext4 inode.
> >
> > Set the flag to be user visible and changeable. Set the flag to be
> > inherited. Allow applications to change the flag at any time.
> >
> > Finally, on regular files, flag the inode to not be cached to facilitate
> > changing S_DAX on the next creation of the inode.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > ---
> > Change from RFC:
> > use new d_mark_dontcache()
> > Allow caching if ALWAYS/NEVER is set
> > Rebased to latest Linus master
> > Change flag to unused 0x01000000
> > update ext4_should_enable_dax()
> > ---
> > fs/ext4/ext4.h | 13 +++++++++----
> > fs/ext4/inode.c | 4 +++-
> > fs/ext4/ioctl.c | 25 ++++++++++++++++++++++++-
> > 3 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 01d1de838896..715f8f2029b2 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -415,13 +415,16 @@ struct flex_groups {
> > #define EXT4_VERITY_FL 0x00100000 /* Verity protected inode */
> > #define EXT4_EA_INODE_FL 0x00200000 /* Inode used for large EA */
> > /* 0x00400000 was formerly EXT4_EOFBLOCKS_FL */
> > +
> > +#define EXT4_DAX_FL 0x01000000 /* Inode is DAX */
> > +
> > #define EXT4_INLINE_DATA_FL 0x10000000 /* Inode has inline data. */
> > #define EXT4_PROJINHERIT_FL 0x20000000 /* Create with parents projid */
> > #define EXT4_CASEFOLD_FL 0x40000000 /* Casefolded file */
> > #define EXT4_RESERVED_FL 0x80000000 /* reserved for ext4 lib */
> >
> > -#define EXT4_FL_USER_VISIBLE 0x705BDFFF /* User visible flags */
> > -#define EXT4_FL_USER_MODIFIABLE 0x604BC0FF /* User modifiable flags */
> > +#define EXT4_FL_USER_VISIBLE 0x715BDFFF /* User visible flags */
> > +#define EXT4_FL_USER_MODIFIABLE 0x614BC0FF /* User modifiable flags */
>
> Hum, I think this was already mentioned but there are also definitions in
> include/uapi/linux/fs.h which should be kept in sync... Also if DAX flag
> gets modified through FS_IOC_SETFLAGS, we should call ext4_doncache() as
> well, shouldn't we?
Ah yea it was mentioned. Sorry.
>
> > @@ -802,6 +807,21 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
> > return error;
> > }
> >
> > +static void ext4_dax_dontcache(struct inode *inode, unsigned int flags)
> > +{
> > + struct ext4_inode_info *ei = EXT4_I(inode);
> > +
> > + if (S_ISDIR(inode->i_mode))
> > + return;
> > +
> > + if (test_opt2(inode->i_sb, DAX_NEVER) ||
> > + test_opt(inode->i_sb, DAX_ALWAYS))
> > + return;
> > +
> > + if (((ei->i_flags ^ flags) & EXT4_DAX_FL) == EXT4_DAX_FL)
> > + d_mark_dontcache(inode);
> > +}
> > +
> > long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > {
> > struct inode *inode = file_inode(filp);
> > @@ -1267,6 +1287,9 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > return err;
> >
> > inode_lock(inode);
> > +
> > + ext4_dax_dontcache(inode, flags);
> > +
>
> I don't think we should set dontcache flag when setting of DAX flag fails -
> it could event be a security issue).
good point.
>
> So I think you'll have to check
> whether DAX flag is being changed,
ext4_dax_dontcache() does check if the flag is being changed.
> call vfs_ioc_fssetxattr_check(), and
> only if it succeeded and DAX flags was changing call ext4_dax_dontcache().
Yes I think it would be better to ensure all of the ioctl succeeds prior to
setting the don't cache. The logic is easier to follow.
Ira
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
