Re: [PATCH v3 2/6] ext2, ext4: only set S_DAX for regular inodes

Jan Kara
2016-02-17T21:33:12Z

On Tue 16-02-16 20:34:15, Ross Zwisler wrote:
> When S_DAX is set on an inode we assume that if there are pages attached
> to the mapping (mapping->nrpages != 0), those pages are clean zero pages
> that were used to service reads from holes. Any dirty data associated with
> the inode should be in the form of DAX exceptional entries
> (mapping->nrexceptional) that is written back via
> dax_writeback_mapping_range().
>
> With the current code, though, this isn't always true. For example, ext2
> and ext4 directory inodes can have S_DAX set, but have their dirty data
> stored as dirty page cache entries. For these types of inodes, having
> S_DAX set doesn't really make sense since their I/O doesn't actually happen
> through the DAX code path.
>
> Instead, only allow S_DAX to be set for regular inodes for ext2 and ext4.
> This allows us to have strict DAX vs non-DAX paths in the writeback code.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

Honza

> ---
> fs/ext2/inode.c | 2 +-
> fs/ext4/inode.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 338eefd..27e2cdd 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1296,7 +1296,7 @@ void ext2_set_inode_flags(struct inode *inode)
> inode->i_flags |= S_NOATIME;
> if (flags & EXT2_DIRSYNC_FL)
> inode->i_flags |= S_DIRSYNC;
> - if (test_opt(inode->i_sb, DAX))
> + if (test_opt(inode->i_sb, DAX) && S_ISREG(inode->i_mode))
> inode->i_flags |= S_DAX;
> }
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 83bc8bf..7088aa5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4127,7 +4127,7 @@ void ext4_set_inode_flags(struct inode *inode)
> new_fl |= S_NOATIME;
> if (flags & EXT4_DIRSYNC_FL)
> new_fl |= S_DIRSYNC;
> - if (test_opt(inode->i_sb, DAX))
> + if (test_opt(inode->i_sb, DAX) && S_ISREG(inode->i_mode))
> new_fl |= S_DAX;
> inode_set_flags(inode, new_fl,
> S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC|S_DAX);
> --
> 2.5.0
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR