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

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

[Jeff Liu]

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;
+		}
+
+		/*
+		 * 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;
+		start = XFS_FSB_TO_B(mp, fsbno);
+		if (start >= isize) {
+			error = ENXIO;
+			goto out_unlock;
+		}
 	}
 
 	if (offset != file->f_pos)
-- 
1.7.4.1

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

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

[Mark Tinguely]

On 08/03/12 01:11, 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>
>
> ---

Looks great.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

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

[Dave Chinner]

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

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

[Jie Liu]

On 08/10/12 07:37, Dave Chinner wrote:
> 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;
> 		}
It looks very nice to wrap up those check ups in loop.

Thanks,
-Jeff
>
>> +
>> +		/*
>> +		 * 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.

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