From: Ross Zwisler Subject: Re: [PATCH 08/18] ext4: Pre-zero allocated blocks for DAX IO Date: Fri, 29 Apr 2016 12:01:58 -0600 Message-ID: <20160429180158.GC5888@linux.intel.com> References: <1461015341-20153-1-git-send-email-jack@suse.cz> <1461015341-20153-9-git-send-email-jack@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-mm@kvack.org, Ross Zwisler , Dan Williams , linux-nvdimm@lists.01.org, Matthew Wilcox To: Jan Kara Return-path: Content-Disposition: inline In-Reply-To: <1461015341-20153-9-git-send-email-jack@suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org On Mon, Apr 18, 2016 at 11:35:31PM +0200, Jan Kara wrote: > Currently ext4 treats DAX IO the same way as direct IO. I.e., it > allocates unwritten extents before IO is done and converts unwritten > extents afterwards. However this way DAX IO can race with page fault to > the same area: > > ext4_ext_direct_IO() dax_fault() > dax_io() > get_block() - allocates unwritten extent > copy_from_iter_pmem() > get_block() - converts > unwritten block to > written and zeroes it > out > ext4_convert_unwritten_extents() > > So data written with DAX IO gets lost. Similarly dax_new_buf() called > from dax_io() can overwrite data that has been already written to the > block via mmap. > > Fix the problem by using pre-zeroed blocks for DAX IO the same way as we > use them for DAX mmap. The downside of this solution is that every > allocating write writes each block twice (once zeros, once data). Fixing > the race with locking is possible as well however we would need to > lock-out faults for the whole range written to by DAX IO. And that is > not easy to do without locking-out faults for the whole file which seems > too aggressive. > > Signed-off-by: Jan Kara Just a couple of simplifications - feel free to ignore them if you don't think they are worth the effort. > --- > fs/ext4/ext4.h | 11 +++++++++-- > fs/ext4/file.c | 4 ++-- > fs/ext4/inode.c | 42 +++++++++++++++++++++++++++++++++--------- > 3 files changed, 44 insertions(+), 13 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 35792b430fb6..173da8faff81 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -2521,8 +2521,8 @@ struct buffer_head *ext4_getblk(handle_t *, struct inode *, ext4_lblk_t, int); > struct buffer_head *ext4_bread(handle_t *, struct inode *, ext4_lblk_t, int); > int ext4_get_block_unwritten(struct inode *inode, sector_t iblock, > struct buffer_head *bh_result, int create); > -int ext4_dax_mmap_get_block(struct inode *inode, sector_t iblock, > - struct buffer_head *bh_result, int create); > +int ext4_dax_get_block(struct inode *inode, sector_t iblock, > + struct buffer_head *bh_result, int create); > int ext4_get_block(struct inode *inode, sector_t iblock, > struct buffer_head *bh_result, int create); > int ext4_dio_get_block(struct inode *inode, sector_t iblock, > @@ -3328,6 +3328,13 @@ static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end) > } > } > > +static inline bool ext4_aligned_io(struct inode *inode, loff_t off, loff_t len) > +{ > + int blksize = 1 << inode->i_blkbits; > + > + return IS_ALIGNED(off, blksize) && IS_ALIGNED(off + len, blksize); This could be just a tiny bit simpler by doing return IS_ALIGNED(off, blksize) && IS_ALIGNED(len, blksize); ^^^ You've already made sure 'off' is aligned, so if 'len' is aligned 'off+len' will be aligned. > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 23fd0e0a9223..6d5d5c1db293 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3215,12 +3215,17 @@ static int ext4_releasepage(struct page *page, gfp_t wait) > } > > #ifdef CONFIG_FS_DAX > -int ext4_dax_mmap_get_block(struct inode *inode, sector_t iblock, > - struct buffer_head *bh_result, int create) > +/* > + * Get block function for DAX IO and mmap faults. It takes care of converting > + * unwritten extents to written ones and initializes new / converted blocks > + * to zeros. > + */ > +int ext4_dax_get_block(struct inode *inode, sector_t iblock, > + struct buffer_head *bh_result, int create) > { > int ret; > > - ext4_debug("ext4_dax_mmap_get_block: inode %lu, create flag %d\n", > + ext4_debug("ext4_dax_get_block: inode %lu, create flag %d\n", > inode->i_ino, create); This pattern could be improved by using "%s" and __func__ for the function name. That way you don't have to hunt through all your debug code and update strings when you rename a function. More importantly it prevents the strings from getting out of sync with the function name, resulting in confusing debug messages. > if (!create) > return _ext4_get_block(inode, iblock, bh_result, 0); > @@ -3233,9 +3238,9 @@ int ext4_dax_mmap_get_block(struct inode *inode, sector_t iblock, > > if (buffer_unwritten(bh_result)) { > /* > - * We are protected by i_mmap_sem so we know block cannot go > - * away from under us even though we dropped i_data_sem. > - * Convert extent to written and write zeros there. > + * We are protected by i_mmap_sem or i_mutex so we know block > + * cannot go away from under us even though we dropped > + * i_data_sem. Convert extent to written and write zeros there. > */ > ret = ext4_get_block_trans(inode, iblock, bh_result, > EXT4_GET_BLOCKS_CONVERT | > @@ -3250,6 +3255,14 @@ int ext4_dax_mmap_get_block(struct inode *inode, sector_t iblock, > clear_buffer_new(bh_result); > return 0; > } > +#else > +/* Just define empty function, it will never get called. */ > +int ext4_dax_get_block(struct inode *inode, sector_t iblock, > + struct buffer_head *bh_result, int create) > +{ > + BUG(); > + return 0; > +} You don't need this stub. All the uses of ext4_dax_get_block() are either within their own '#ifdef CONFIG_FS_DAX' sections, or they are in an "if (IS_DAX)" conditional. The latter will also be compiled out if CONFIG_FS_DAX isn't defined. This is because of the way that S_DAX is defined: #define S_DAX 8192 /* Direct Access, avoiding the page cache */ #else #define S_DAX 0 /* Make all the DAX code disappear */ #endif