Re: [PATCH v2] xfs: update i_size after unwritten conversion in dio completion
From: Christoph Hellwig <hch@infradead.org>
Date: 2017-09-26 15:00:30
On Thu, Sep 21, 2017 at 11:52:46PM +0800, Eryu Guan wrote:
quoted hunk ↗ jump to hunk
Since commit d531d91d6990 ("xfs: always use unwritten extents for direct I/O writes"), we start allocating unwritten extents for all direct writes to allow appending aio in XFS. But for dio writes that could extend file size we update the in-core inode size first, then convert the unwritten extents to real allocations at dio completion time in xfs_dio_write_end_io(). Thus a racing direct read could see the new i_size and find the unwritten extents first and read zeros instead of actual data, if the direct writer also takes a shared iolock. Fix it by updating the in-core inode size after the unwritten extent conversion. To do this, introduce a new boolean argument to xfs_iomap_write_unwritten() to tell if we want to update in-core i_size or not. Suggested-by: Brian Foster <redacted> Reviewed-by: Brian Foster <redacted> Signed-off-by: Eryu Guan <redacted> --- v2: - fix comments by copying Brian's words :) - fix code style, remove unnecessary blank lines and braces v1: https://marc.info/?l=linux-xfs&m=150599032124565&w=2 fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_file.c | 33 +++++++++++++++++++-------------- fs/xfs/xfs_iomap.c | 7 +++++-- fs/xfs/xfs_iomap.h | 2 +- fs/xfs/xfs_pnfs.c | 2 +- 5 files changed, 28 insertions(+), 19 deletions(-)diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 29172609f2a3..f18e5932aec4 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c@@ -343,7 +343,8 @@ xfs_end_io( error = xfs_reflink_end_cow(ip, offset, size); break; case XFS_IO_UNWRITTEN: - error = xfs_iomap_write_unwritten(ip, offset, size); + /* writeback should never update isize */ + error = xfs_iomap_write_unwritten(ip, offset, size, false);
Can we expand this to /* writeback should never update the in-core inode size */
quoted hunk ↗ jump to hunk
@@ -459,20 +473,11 @@ xfs_dio_write_end_io( spin_lock(&ip->i_flags_lock); if (offset + size > i_size_read(inode)) { i_size_write(inode, offset + size); + spin_unlock(&ip->i_flags_lock); error = xfs_setfilesize(ip, offset, size); + } else { + spin_unlock(&ip->i_flags_lock); + }
I find the old update_isize scheme a little easier to read, but it
shouldn't matter in the end:
spin_lock(&ip->i_flags_lock);
if (offset + size > i_size_read(inode)) {
i_size_write(inode, offset + size);
update_isize = true;
}
spin_unlock(&ip->i_flags_lock);
if (!update_isize)
return 0;
return xfs_setfilesize(ip, offset, size);
quoted hunk ↗ jump to hunk
xfs_bmbt_irec_t imap; struct xfs_defer_ops dfops; + struct inode *inode = VFS_I(ip); xfs_fsize_t i_size; uint resblks; int error;@@ -899,7 +901,8 @@ xfs_iomap_write_unwritten( i_size = XFS_FSB_TO_B(mp, offset_fsb + count_fsb); if (i_size > offset + count) i_size = offset + count; - + if (update_isize && i_size > i_size_read(inode)) + i_size_write(inode, i_size); i_size = xfs_new_eof(ip, i_size); if (i_size) { ip->i_d.di_size = i_size;
I wonder if this might be cleaner if we expand xfs_new_eof for
the update_isize case. Something like the untested helper below:
static bool
xfs_alloc_update_isize(
struct xfs_inode *ip,
xfs_fileoff_t offset_fsb,
xfs_filblks_t count_fsb,
bool update_incore_isize)
{
xfs_fsize_t i_size;
i_size = XFS_FSB_TO_B(ip->i_mount, offset_fsb + count_fsb);
if (i_size > offset + count)
i_size = offset + count;
if (update_isize) {
if (i_size <= i_size_read(inode))
return;
i_size_write(inode, i_size);
} else {
i_size = xfs_new_eof(ip, i_size);
if (!i_size)
return;
}
ip->i_d.di_size = i_size;
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
}