Re: [PATCH v6 3/4] xfs: xfs_seek_data() refinement with lookup data buffer offset from page cache

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 03, 2012 at 02:11:07PM +0800, Jeff Liu wrote:
> Refine xfs_seek_data() to check up data buffer offset from page cache for unwritten extents.
> 
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> 
> ---
>  fs/xfs/xfs_file.c |   77 ++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index aff0c30..e852e52 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1187,8 +1187,6 @@ xfs_seek_data(
>  	struct inode		*inode = file->f_mapping->host;
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_bmbt_irec	map[2];
> -	int			nmap = 2;
>  	loff_t			uninitialized_var(offset);
>  	xfs_fsize_t		isize;
>  	xfs_fileoff_t		fsbno;
> @@ -1204,34 +1202,77 @@ xfs_seek_data(
>  		goto out_unlock;
>  	}
>  
> -	fsbno = XFS_B_TO_FSBT(mp, start);
> -
>  	/*
>  	 * Try to read extents from the first block indicated
>  	 * by fsbno to the end block of the file.
>  	 */
> +	fsbno = XFS_B_TO_FSBT(mp, start);
>  	end = XFS_B_TO_FSB(mp, isize);
> +	for (;;) {
> +		struct xfs_bmbt_irec	map[2];
> +		int			nmap = 2;
>  
> -	error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
> -			       XFS_BMAPI_ENTIRE);
> -	if (error)
> -		goto out_unlock;
> +		error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
> +				       XFS_BMAPI_ENTIRE);
> +		if (error)
> +			goto out_unlock;
>  
> -	/*
> -	 * Treat unwritten extent as data extent since it might
> -	 * contains dirty data in page cache.
> -	 */
> -	if (map[0].br_startblock != HOLESTARTBLOCK) {
> -		offset = max_t(loff_t, start,
> -			       XFS_FSB_TO_B(mp, map[0].br_startoff));
> -	} else {
> +		/* No extents at given offset, must be beyond EOF */
> +		if (nmap == 0) {
> +			error = ENXIO;
> +			goto out_unlock;
> +		}
> +
> +		offset = start;
> +		/* Landed in a data extent */
> +		if (map[0].br_startblock == DELAYSTARTBLOCK ||
> +		    (map[0].br_state == XFS_EXT_NORM &&
> +		     !isnullstartblock(map[0].br_startblock)))
> +			break;
> +
> +		/*
> +		 * Landed in an unwritten extent, try to search data
> +		 * from page cache.
> +		 */
> +		if (map[0].br_state == XFS_EXT_UNWRITTEN) {
> +			if (xfs_find_get_desired_pgoff(inode, &map[0],
> +						       DATA_OFF, &offset))
> +				break;
> +		}
> +
> +		/*
> +		 * map[0] is hole or its an unwritten extent but
> +		 * without data in page cache.  Probably means that
> +		 * we are reading after EOF if nothing in map[1].
> +		 */
>  		if (nmap == 1) {
>  			error = ENXIO;
>  			goto out_unlock;
>  		}
>  
> -		offset = max_t(loff_t, start,
> -			       XFS_FSB_TO_B(mp, map[1].br_startoff));
> +		/* We have two mappings, proceed to check map[1] */
> +		offset = XFS_FSB_TO_B(mp, map[1].br_startoff);
> +		if (map[1].br_startblock == DELAYSTARTBLOCK ||
> +		    (map[1].br_state == XFS_EXT_NORM &&
> +		     !isnullstartblock(map[1].br_startblock)))
> +			break;
> +
> +		if (map[1].br_state == XFS_EXT_UNWRITTEN) {
> +			if (xfs_find_get_desired_pgoff(inode, &map[1],
> +						DATA_OFF, &offset))
> +				break;
> +		}

That's now duplicate code, so a loop would be much better to make it
simpler to maintain an enhance in future:

		for (i = 0; i < nmap; i++) {
			offset = max_t(loff_t, start,
				       XFS_FSB_TO_B(mp, map[i].br_startoff));

			/* Landed in a data extent */
			if (map[i].br_startblock == DELAYSTARTBLOCK ||
			    (map[i].br_state == XFS_EXT_NORM &&
			     !isnullstartblock(map[i].br_startblock)))
				break;

			/*
			 * Landed in an unwritten extent, try to search data
			 * from page cache.
			 */
			if (map[i].br_state == XFS_EXT_UNWRITTEN) {
				if (xfs_find_get_desired_pgoff(inode, &map[i],
							       DATA_OFF, &offset))
					break;
			}
		}

		/*
		 * map[0] is hole or its an unwritten extent but
		 * without data in page cache.  Probably means that
		 * we are reading after EOF if nothing in map[1].
		 */
		if (nmap == 1) {
			error = ENXIO;
			goto out_unlock;
		}

> +
> +		/*
> +		 * Nothing was found, proceed to the next round of search
> +		 * if reading offset not beyond or hit EOF.
> +		 */
> +		fsbno = map[1].br_startoff + map[1].br_blockcount;

		fsbno = map[i].br_startoff + map[i].br_blockcount;

> +		start = XFS_FSB_TO_B(mp, fsbno);
> +		if (start >= isize) {
> +			error = ENXIO;
> +			goto out_unlock;
> +		}
>  	}

Otherwise this looks just fine.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs