Re: [PATCH V4 7/8] fs/ext4: Introduce DAX inode flag

From: Jan Kara
Date: Fri May 22 2020 - 07:48:54 EST


On Thu 21-05-20 12:13:12, 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 with the
> exception of if VERITY or ENCRYPT is set.
>
> Disallow setting VERITY or ENCRYPT if DAX is set.
>
> 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>

...

> @@ -303,6 +318,16 @@ static int ext4_ioctl_setflags(struct inode *inode,
> unsigned int jflag;
> struct super_block *sb = inode->i_sb;
>
> + if (ext4_test_inode_flag(inode, EXT4_INODE_DAX)) {
> + if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY) ||
> + ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT) ||
> + ext4_test_inode_state(inode,
> + EXT4_STATE_VERITY_IN_PROGRESS)) {
> + err = -EOPNOTSUPP;
> + goto flags_out;
> + }
> + }

The way this check is implemented wouldn't IMO do what we need... It
doesn't check the flags that are being set but just the current inode
state. I think it should rather be:

if ((flags ^ oldflags) & EXT4_INODE_DAX_FL) {
...
}

And perhaps move this to a place in ext4_ioctl_setflags() where we check
other similar conflicts.

And then we should check conflicts with the journal flag as well, as I
mentioned in reply to the first patch. There it is more complicated by the
fact that we should disallow setting of both EXT4_INODE_DAX_FL and
EXT4_JOURNAL_DATA_FL at the same time so the checks will be somewhat more
complicated.

Honza

> +
> /* Is it quota file? Do not allow user to mess with it */
> if (ext4_is_quota_file(inode))
> goto flags_out;
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR