xfs
[Top] [All Lists]

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

To: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH v3 2/6] ext2, ext4: only set S_DAX for regular inodes
From: Jan Kara <jack@xxxxxxx>
Date: Wed, 17 Feb 2016 22:33:25 +0100
Cc: linux-kernel@xxxxxxxxxxxxxxx, "J. Bruce Fields" <bfields@xxxxxxxxxxxx>, Theodore Ts'o <tytso@xxxxxxx>, Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>, Andreas Dilger <adilger.kernel@xxxxxxxxx>, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>, Dan Williams <dan.j.williams@xxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, Jan Kara <jack@xxxxxxxx>, Jeff Layton <jlayton@xxxxxxxxxxxxxxx>, Jens Axboe <axboe@xxxxxxxxx>, Matthew Wilcox <willy@xxxxxxxxxxxxxxx>, linux-block@xxxxxxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx, linux-nvdimm@xxxxxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1455680059-20126-3-git-send-email-ross.zwisler@xxxxxxxxxxxxxxx>
References: <1455680059-20126-1-git-send-email-ross.zwisler@xxxxxxxxxxxxxxx> <1455680059-20126-3-git-send-email-ross.zwisler@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
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@xxxxxxxxxxxxxxx>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

                                                                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@xxxxxxxx>
SUSE Labs, CR

<Prev in Thread] Current Thread [Next in Thread>