Re: [PATCH RFC 4/8] xfs: implement inline data read write code

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Jul 06, 2018 at 11:12:25AM +0800, Shan Hai wrote:
> Implement read/write functions for inline data access and use them
> for buffered/DIO/DAX operations.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_file.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 242 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index a3e7767a5715..55047928b720 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -27,6 +27,7 @@
>  #include "xfs_pnfs.h"
>  #include "xfs_iomap.h"
>  #include "xfs_reflink.h"
> +#include "xfs_defer.h"
>  
>  #include <linux/dcache.h>
>  #include <linux/falloc.h>
> @@ -175,6 +176,185 @@ xfs_file_fsync(
>  	return error;
>  }
>  
> +/*
> + * Read the inline data of a local format file.
> + */
> +STATIC ssize_t
> +xfs_file_inline_data_read(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct kiocb		*iocb,
> +	struct iov_iter		*to)
> +{
> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct inode		*inode = VFS_I(ip);
> +	loff_t			isize = i_size_read(inode);
> +	loff_t			*ppos = &iocb->ki_pos;
> +	size_t			count = iov_iter_count(to);
> +	ssize_t			ret = 0;
> +
> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
> +
> +	if (!count)
> +		return 0;
> +
> +	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
> +		return 0;
> +
> +	if (*ppos > isize)
> +		return 0;
> +
> +	iov_iter_truncate(to, inode->i_sb->s_maxbytes);
> +	count = (*ppos + count > isize) ? isize - *ppos : count;
> +	ret = copy_to_iter(ifp->if_u1.if_data + *ppos, count, to);
> +	*ppos += ret;
> +
> +	return ret;
> +}
> +
> +/*
> + * Try to inline the data into the xfs inode data fork when it has enough
> + * space to hold it. The data inlining happens at below points:
> + *
> + * - writeback: for buffered writes
> + * - write_iter: for dax/dio writes
> + *
> + * The extents to local format conversion is done here for DAX/DIO write,
> + * while the same conversion is done in the writeback path for buffered write.
> + * This function also converts inode from local to extents when the data
> + * fork could not hold the data inline anymore.
> + */
> +STATIC ssize_t
> +xfs_file_inline_data_write(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct kiocb		*iocb,
> +	struct iov_iter		*from)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct inode		*inode = VFS_I(ip);
> +	struct address_space	*mapping = inode->i_mapping;
> +	loff_t			pos = iocb->ki_pos;
> +	size_t			count = iov_iter_count(from);
> +	struct xfs_trans	*tp;
> +	struct xfs_defer_ops	dfops;
> +	xfs_fsblock_t		first_block;
> +	int			ret;
> +	int			error = 0;
> +	int			logflags = 0;
> +	loff_t			written = 0;
> +	xfs_fileoff_t		bno = 0;
> +	struct xfs_bmbt_irec	map;
> +	int			nmap;
> +	struct page		*page;
> +	char			*kaddr;
> +
> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 1, 0, 0, &tp);
> +	if (error) {
> +		ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +	xfs_defer_init(&dfops, &first_block);
> +
> +	if (pos + count > XFS_IFORK_DSIZE(ip)) {
> +		if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS) {
> +			error = 0;
> +			goto trans_cancel;
> +		}
> +
> +		page = find_or_create_page(mapping, pos >> PAGE_SHIFT, GFP_NOFS);
> +		if (!page) {
> +			error = -ENOMEM;
> +			goto trans_cancel;
> +		}
> +		kaddr = kmap_atomic(page);
> +		memcpy(kaddr, ifp->if_u1.if_data, ifp->if_bytes);
> +		kunmap_atomic(kaddr);
> +		SetPageUptodate(page);
> +		unlock_page(page);
> +		put_page(page);
> +
> +		xfs_bmap_forkoff_reset(ip, whichfork);
> +		ifp->if_flags &= ~XFS_IFINLINE;
> +		ifp->if_flags |= XFS_IFEXTENTS;
> +		ifp->if_u1.if_root = NULL;
> +		ifp->if_height = 0;
> +		ifp->if_bytes = 0;
> +		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> +
> +		written = ifp->if_bytes;
> +		error = xfs_bmap_first_unused(tp, ip, 1, &bno, whichfork);
> +		if (error)
> +			goto trans_cancel;
> +		nmap = 1;
> +		error = xfs_bmapi_write(tp, ip, bno, 1, XFS_BMAPI_ENTIRE,
> +				&first_block, 1, &map, &nmap, &dfops);
> +		if (error)
> +			goto trans_cancel;
> +
> +		goto out_local_to_extents;
> +	}
> +
> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
> +		xfs_idata_realloc(ip, (pos + count) - ifp->if_bytes, whichfork);
> +		ret = copy_from_iter(ifp->if_u1.if_data + pos, count, from);
> +		written = ret;
> +		goto out;
> +	}
> +
> +	if (IS_DAX(inode) || iocb->ki_flags & IOCB_DIRECT) {
> +		struct page	*page = alloc_page(GFP_KERNEL | GFP_NOFS);
> +		if (!page) {
> +			error = -ENOMEM;
> +			goto trans_cancel;
> +		}
> +
> +		kaddr = kmap_atomic(page);
> +		ret = copy_from_iter(kaddr + pos, count, from);
> +		kunmap_atomic(kaddr);
> +		written = ret;
> +		error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
> +							XFS_DATA_FORK, page);
> +		if (error) {
> +			__free_page(page);
> +			goto trans_cancel;
> +		}
> +
> +		__free_page(page);
> +		goto out;
> +	}
> +trans_cancel:
> +	xfs_defer_cancel(&dfops);
> +	xfs_trans_cancel(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return error;
> +out:
> +	if (pos + written > i_size_read(inode))
> +		i_size_write(inode, pos + written);
> +	ip->i_d.di_size = pos + written;
> +
> +out_local_to_extents:
> +	logflags |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
> +	xfs_trans_log_inode(tp, ip, logflags);
> +
> +	error = xfs_defer_finish(&tp, &dfops);
> +	if (error)
> +		goto trans_cancel;
> +	error = xfs_trans_commit(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +
> +	return written > 0 ? written : error;
> +}
> +
> +
>  STATIC ssize_t
>  xfs_file_dio_aio_read(
>  	struct kiocb		*iocb,
> @@ -192,7 +372,12 @@ xfs_file_dio_aio_read(
>  	file_accessed(iocb->ki_filp);
>  
>  	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);

I thought iomap was growing the ability to handle inline data?  At least
for gfs2?

Hmm, you might ask Christoph about how to support that... also these
read/write paths are about to get a lot of rip/smash this cycle.

> +	} else {
> +		ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
> +	}
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
>  	return ret;
> @@ -219,6 +404,13 @@ xfs_file_dax_read(
>  		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>  	}
>  
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
> +	} else {
> +		ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> +	}
> +
>  	ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
> @@ -242,7 +434,12 @@ xfs_file_buffered_aio_read(
>  	} else {
>  		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>  	}
> -	ret = generic_file_read_iter(iocb, to);
> +
> +	if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
> +	} else {
> +		ret = generic_file_read_iter(iocb, to);
> +	}
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
>  	return ret;
> @@ -487,6 +684,7 @@ xfs_file_dio_aio_write(
>  	size_t			count = iov_iter_count(from);
>  	struct xfs_buftarg      *target = XFS_IS_REALTIME_INODE(ip) ?
>  					mp->m_rtdev_targp : mp->m_ddev_targp;
> +	int			error;
>  
>  	/* DIO must be aligned to device logical sector size */
>  	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
> @@ -547,6 +745,22 @@ xfs_file_dio_aio_write(
>  	}
>  
>  	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
> +
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&

!= BTREE? Why not == INLINE?

> +		S_ISREG(inode->i_mode)) {
> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb,
> +							from);

int != ssize_t... also, if we decided we needed extents format but the
file turned out to be extents format already then did we lose the write
here?

> +		if (error) {
> +			ret = error;
> +			goto out;
> +		}
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +			ret = count;
> +			goto out;
> +		}
> +	}
> +
>  	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
>  out:
>  	xfs_iunlock(ip, iolock);
> @@ -586,6 +800,20 @@ xfs_file_dax_write(
>  	count = iov_iter_count(from);
>  
>  	trace_xfs_file_dax_write(ip, count, pos);
> +
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +		ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
> +		S_ISREG(inode->i_mode)) {
> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK,
> +							iocb, from);
> +		if (error)
> +			goto out;
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +			ret = count;
> +			goto out;
> +		}
> +	}
> +
>  	ret = dax_iomap_rw(iocb, from, &xfs_iomap_ops);
>  	if (ret > 0 && iocb->ki_pos > i_size_read(inode)) {
>  		i_size_write(inode, iocb->ki_pos);
> @@ -614,6 +842,7 @@ xfs_file_buffered_aio_write(
>  	struct address_space	*mapping = file->f_mapping;
>  	struct inode		*inode = mapping->host;
>  	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
>  	ssize_t			ret;
>  	int			enospc = 0;
>  	int			iolock;
> @@ -629,6 +858,17 @@ xfs_file_buffered_aio_write(
>  	if (ret)
>  		goto out;
>  
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE) {
> +		ret = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb, from);
> +		if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) ==
> +							XFS_DINODE_FMT_LOCAL){

What's going on with the indenting here?

--D

> +			if (likely(ret >= 0))
> +				iocb->ki_pos += ret;
> +			goto out;
> +		}
> +	}
> +
>  	/* We can write back this queue in page reclaim */
>  	current->backing_dev_info = inode_to_bdi(inode);
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html