On Fri, Apr 10, 2015 at 11:38:00PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> generic_file_direct_write() does all sorts of things to make DIO
> work "sorta ok" with mixed buffered IO workloads. We already do
> most of this work in xfs_file_aio_dio_write() because of the locking
> requirements, so there's only a couple of things it does for us.
>
> The first thing is that it does a page cache invalidation after the
> ->direct_IO callout. This can easily be added to the XFS code.
>
> The second thing it does is that if data was written, it updates the
> iov_iter structure to reflect the data written, and then does EOF
> size updates if necessary. For XFS, these EOF size updates are now
> not necessary, as we do them safely and race-free in IO completion
> context. That leaves just the iov_iter update, and that's also exily
> moved to the XFS code.
>
> The result is that we don't need to call
> generic_file_direct_write(), and hence remove a redundant buffered
> writeback call and a redundant page cache invalidation call from the
> DIO submission path. We also remove a racy EOF size update, and make
> the DIO submission code in XFS much easier to follow. Wins all
> round, really.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
Seems fine to me:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> fs/xfs/xfs_file.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 7b872f4..7182cd2 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -665,6 +665,8 @@ xfs_file_dio_aio_write(
> int iolock;
> size_t count = iov_iter_count(from);
> loff_t pos = iocb->ki_pos;
> + loff_t end;
> + struct iov_iter data;
> struct xfs_buftarg *target = XFS_IS_REALTIME_INODE(ip) ?
> mp->m_rtdev_targp : mp->m_ddev_targp;
>
> @@ -704,10 +706,11 @@ xfs_file_dio_aio_write(
> if (ret)
> goto out;
> iov_iter_truncate(from, count);
> + end = pos + count - 1;
>
> if (mapping->nrpages) {
> ret = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> - pos, pos + count - 1);
> + pos, end);
> if (ret)
> goto out;
> /*
> @@ -717,7 +720,7 @@ xfs_file_dio_aio_write(
> */
> ret = invalidate_inode_pages2_range(VFS_I(ip)->i_mapping,
> pos >> PAGE_CACHE_SHIFT,
> - (pos + count - 1) >> PAGE_CACHE_SHIFT);
> + end >> PAGE_CACHE_SHIFT);
> WARN_ON_ONCE(ret);
> ret = 0;
> }
> @@ -734,8 +737,22 @@ xfs_file_dio_aio_write(
> }
>
> trace_xfs_file_direct_write(ip, count, iocb->ki_pos, 0);
> - ret = generic_file_direct_write(iocb, from, pos);
>
> + data = *from;
> + ret = mapping->a_ops->direct_IO(WRITE, iocb, &data, pos);
> +
> + /* see generic_file_direct_write() for why this is necessary */
> + if (mapping->nrpages) {
> + invalidate_inode_pages2_range(mapping,
> + pos >> PAGE_CACHE_SHIFT,
> + end >> PAGE_CACHE_SHIFT);
> + }
> +
> + if (ret > 0) {
> + pos += ret;
> + iov_iter_advance(from, ret);
> + iocb->ki_pos = pos;
> + }
> out:
> xfs_rw_iunlock(ip, iolock);
>
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
