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 <jack@xxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
