Skip to contentSkip to conversation

xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.

PATCH Superseded Jeff Liu 3 participants 6 messages Jul 26, 2012 – Jul 31, 2012 XFS filesystem

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Jeff Liu

author
#0 lore raw

PATCH v5 3/4 xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.

Search data buffer offset for given range from page cache for unwritten extents.

Signed-off-by Jie Liu <[email protected]>
Reviewed-by Mark Tinguely <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Dave Chinner <[email protected]>
---
 fs/xfs/xfs_file.c |   88 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 70 insertions(+), 18 deletions(-)
fs/xfs/xfs_file.c +70 -18
index 43f5e61..15acd4d 100644
@@ -1177,8 +1177,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;

@@ -1194,34 +1192,88 @@ 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 {
-		if (nmap == 1) {
+		/* No extents at given offset, must be beyond EOF */
+		if (nmap == 0) {
 			error = ENXIO;
 			goto out_unlock;
 		}
 
 		offset = max_t(loff_t, start,
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM &&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode, &map[0],
+							DATA_OFF, &offset))
+					break;
+			}
+
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */
+			if (nmap == 1) {
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM &&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    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)
Signature · 6 lines
1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#1 lore raw

PATCH v5 3/4 xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.

Search data buffer offset for given range from page cache.

Signed-off-by Jie Liu <[email protected]>
Reviewed-by Mark Tinguely <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Dave Chinner <[email protected]>
---
 fs/xfs/xfs_file.c |   88 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 70 insertions(+), 18 deletions(-)
fs/xfs/xfs_file.c +70 -18
index 69965a4..b1158b3 100644
@@ -1182,8 +1182,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;

@@ -1199,34 +1197,88 @@ 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 {
-		if (nmap == 1) {
+		/* No extents at given offset, must be beyond EOF */
+		if (nmap == 0) {
 			error = ENXIO;
 			goto out_unlock;
 		}
 
 		offset = max_t(loff_t, start,
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM &&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode, &map[0],
+							DATA_OFF, &offset))
+					break;
+			}
+
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */
+			if (nmap == 1) {
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM &&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    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)
Signature · 6 lines
1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v5 3/4
#2 lore raw
Quoted 3 lines · Jeff Liu

Search data buffer offset for given range from page cache.

Signed-off-by Jie Liu<[email protected]>

...

This is the same as version 4.

It looks good.

There are a couple places the comment precedes the test and is not yet valid:

fs/xfs/xfs_file.c xfs_seek_data( · line 1232

Quoted 13 lines

offset = max_t(loff_t, start,

fs/xfs/xfs_file.c +11 -1
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM&&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */

or it could be a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1234

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {

here is where it is unwritten extent

fs/xfs/xfs_file.c xfs_seek_data( · line 1244

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode,&map[0],
+							DATA_OFF,&offset))
+					break;
+			}
+
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */

here we did find a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1245

Quoted 1 line
fs/xfs/xfs_file.c +1 -0
+			if (nmap == 1) {

here is where there is nothing in map[1] and ...

Quoted 7 lines
fs/xfs/xfs_file.c +7 -0
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */

fs/xfs/xfs_file.c xfs_seek_data( · line 1262

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM&&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */

it could be unwritten or a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1264

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    map[1].br_state == XFS_EXT_UNWRITTEN) {

here is where we know it is unwritten.

fs/xfs/xfs_file.c xfs_seek_data( · line 1270

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+					if (xfs_find_get_desired_pgoff(inode,
+						&map[1], DATA_OFF,&offset))
+						break;
+				}
+			}
+		}

I am being really picky, but comments really help a quick read of the code.

Reviewed-by Mark Tinguely <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#3 lore raw
Quoted 71 lines · 在 2012-7-28，上午5:32， Mark Tinguely 写道：
Quoted 3 lines · Jeff Liu

Search data buffer offset for given range from page cache.

Signed-off-by Jie Liu<[email protected]>

...

This is the same as version 4.

It looks good.

There are a couple places the comment precedes the test and is not yet valid:

fs/xfs/xfs_file.c xfs_seek_data( · line 1232

Quoted 13 lines
fs/xfs/xfs_file.c +11 -1
 		offset = max_t(loff_t, start,
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM&&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */

or it could be a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1234

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {

here is where it is unwritten extent

fs/xfs/xfs_file.c xfs_seek_data( · line 1244

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode,&map[0],
+							DATA_OFF,&offset))
+					break;
+			}
+
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */

here we did find a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1245

Quoted 1 line
fs/xfs/xfs_file.c +1 -0
+			if (nmap == 1) {

here is where there is nothing in map[1] and ...

Quoted 7 lines
fs/xfs/xfs_file.c +7 -0
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */

fs/xfs/xfs_file.c xfs_seek_data( · line 1262

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM&&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */

it could be unwritten or a hole

fs/xfs/xfs_file.c xfs_seek_data( · line 1264

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    map[1].br_state == XFS_EXT_UNWRITTEN) {

here is where we know it is unwritten.

Quoted 8 lines

fs/xfs/xfs_file.c xfs_seek_data( · line 1270

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+					if (xfs_find_get_desired_pgoff(inode,
+						&map[1], DATA_OFF,&offset))
+						break;
+				}
+			}
+		}

I am being really picky, but comments really help a quick read of the code.

Thanks for your review. I'll fix above comments as well as them in xfs_seek_hole() to make patch better.

Thanks,
-Jeff

Quoted 7 lines
Reviewed-by Mark Tinguely <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
In reply to Jeff Liu
Quoted 63 lines · Jeff Liu

Search data buffer offset for given range from page cache.

Signed-off-by Jie Liu <[email protected]>
Reviewed-by Mark Tinguely <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Dave Chinner <[email protected]>
---
 fs/xfs/xfs_file.c |   88 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 70 insertions(+), 18 deletions(-)
fs/xfs/xfs_file.c +10 -17
index 69965a4..b1158b3 100644
@@ -1182,8 +1182,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;

@@ -1199,34 +1197,88 @@ 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 {
-		if (nmap == 1) {
+		/* No extents at given offset, must be beyond EOF */
+		if (nmap == 0) {
 			error = ENXIO;
 			goto out_unlock;
 		}

fs/xfs/xfs_file.c xfs_seek_data( · line 1228

Quoted 10 lines
fs/xfs/xfs_file.c +7 -1
 
 		offset = max_t(loff_t, start,
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM &&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data

Not correct - hole, delay or unwritten land here.

fs/xfs/xfs_file.c xfs_seek_data( · line 1239

Quoted 11 lines
fs/xfs/xfs_file.c +11 -0
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode, &map[0],
+							DATA_OFF, &offset))
+					break;
+			}

If is is a DELAYSTARTBLOCK, and it is within EOF, then it is guaranteed to contain data. There is no need for a page cache lookup to decide where the data starts - by definition the data starts at map[0].br_startblock. I think you can treat DELAYSTARTBLOCK exactly the same as allocated XFS_EXT_NORM extents.

That means the logic is:

		if (map[0].br_state == XFS_EXT_NORM &&
		    (!isnullstartblock(map[0].br_startblock) ||
		     map[0].br_startblock == DELAYSTARTBLOCK)) {
			break;
		} else if (map[0].br_state == XFS_EXT_UNWRITTEN) {
			/* Probing page cache start from offset */
			if (xfs_find_get_desired_pgoff(inode, &map[0],
						DATA_OFF, &offset))
				break;
		} else {
			/*
			 * If we find a hole in map[0] and nothing in map[1] it
			 * probably means that we are reading after EOF.
			 */
			 .....
		}

Which kills a level of indentation in the code....

Quoted 19 lines
fs/xfs/xfs_file.c +19 -0
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */
+			if (nmap == 1) {
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM &&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*

fs/xfs/xfs_file.c xfs_seek_data( · line 1269

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    map[1].br_state == XFS_EXT_UNWRITTEN) {
+					if (xfs_find_get_desired_pgoff(inode,
+						&map[1], DATA_OFF, &offset))
+						break;
+				}
+			}

And the if/elseif/else logic above can be repeated here.

fs/xfs/xfs_file.c xfs_seek_data( · line 1281

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+		}
+
+		/*
+		 * 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;
+		}

Shouldn't this check be done at the start of the loop?

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#5 lore raw
Quoted 65 lines · Dave Chinner
Quoted 64 lines · Jeff Liu

Search data buffer offset for given range from page cache.

Signed-off-by Jie Liu <[email protected]>
Reviewed-by Mark Tinguely <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Dave Chinner <[email protected]>
---
  fs/xfs/xfs_file.c |   88 ++++++++++++++++++++++++++++++++++++++++++-----------
  1 files changed, 70 insertions(+), 18 deletions(-)
fs/xfs/xfs_file.c +10 -17
index 69965a4..b1158b3 100644
@@ -1182,8 +1182,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;

@@ -1199,34 +1197,88 @@ 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 {
-		if (nmap == 1) {
+		/* No extents at given offset, must be beyond EOF */
+		if (nmap == 0) {
  			error = ENXIO;
  			goto out_unlock;
  		}
Quoted 10 lines

fs/xfs/xfs_file.c xfs_seek_data( · line 1228

Quoted 9 lines
fs/xfs/xfs_file.c +7 -1
  		offset = max_t(loff_t, start,
-			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+		if (map[0].br_state == XFS_EXT_NORM &&
+		    !isnullstartblock(map[0].br_startblock))
+			break;
+		else {
+			/*
+			 * Landed in an unwritten extent, try to lookup data

Not correct - hole, delay or unwritten land here.

Will fix it.

Quoted 37 lines

fs/xfs/xfs_file.c xfs_seek_data( · line 1239

Quoted 12 lines
fs/xfs/xfs_file.c +11 -0
+			 * buffer from the page cache before proceeding to
+			 * check the next extent map.  It's a hole if nothing
+			 * was found.
+			 */
+			if (map[0].br_startblock == DELAYSTARTBLOCK ||
+			    map[0].br_state == XFS_EXT_UNWRITTEN) {
+				/* Probing page cache start from offset */
+				if (xfs_find_get_desired_pgoff(inode, &map[0],
+							DATA_OFF, &offset))
+					break;
+			}

If is is a DELAYSTARTBLOCK, and it is within EOF, then it is guaranteed to contain data. There is no need for a page cache lookup to decide where the data starts - by definition the data starts at map[0].br_startblock. I think you can treat DELAYSTARTBLOCK exactly the same as allocated XFS_EXT_NORM extents.

That means the logic is:

		if (map[0].br_state == XFS_EXT_NORM &&
		    (!isnullstartblock(map[0].br_startblock) ||
		     map[0].br_startblock == DELAYSTARTBLOCK)) {
			break;
		} else if (map[0].br_state == XFS_EXT_UNWRITTEN) {
			/* Probing page cache start from offset */
			if (xfs_find_get_desired_pgoff(inode, &map[0],
						DATA_OFF, &offset))
				break;
		} else {
			/*
			 * If we find a hole in map[0] and nothing in map[1] it
			 * probably means that we are reading after EOF.
			 */
			 .....
		}
Quoted 1 line

Which kills a level of indentation in the code....

Just done a quick test with above modifications, it works to me, thanks.

Quoted 33 lines
Quoted 21 lines
fs/xfs/xfs_file.c +19 -0
+			/*
+			 * Found a hole in map[0] and nothing in map[1].
+			 * Probably means that we are reading after EOF.
+			 */
+			if (nmap == 1) {
+				error = ENXIO;
+				goto out_unlock;
+			}
+
+			/*
+			 * We have two mappings, proceed to check map[1].
+			 */
+			offset = max_t(loff_t, start,
+				       XFS_FSB_TO_B(mp, map[1].br_startoff));
+			if (map[1].br_state == XFS_EXT_NORM &&
+			    !isnullstartblock(map[1].br_startblock))
+				break;
+			else {
+				/*

fs/xfs/xfs_file.c xfs_seek_data( · line 1269

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				 * map[1] is also an unwritten extent, lookup
+				 * data buffer from page cache now.
+				 */
+				if (map[1].br_startblock == DELAYSTARTBLOCK ||
+				    map[1].br_state == XFS_EXT_UNWRITTEN) {
+					if (xfs_find_get_desired_pgoff(inode,
+						&map[1], DATA_OFF, &offset))
+						break;
+				}
+			}

And the if/elseif/else logic above can be repeated here.

Quoted 13 lines

fs/xfs/xfs_file.c xfs_seek_data( · line 1281

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+		}
+
+		/*
+		 * 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;
+		}

Shouldn't this check be done at the start of the loop?

If put this check at the beginning of the loop, the code block would looks like below:

    isize = i_size_read(inode);
        if (start >= isize) {
                error = ENXIO;
                goto out_unlock;
        }

    .......
    for (;;) {
                struct xfs_bmbt_irec    map[2];
                int                     nmap = 2;

                if (start >= isize) {
                        error = ENXIO;
                        goto out_unlock;
                }

                error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
                                       XFS_BMAPI_ENTIRE);
                if (error)
                        goto out_unlock;

                /* No extents at given offset, must be beyond EOF */
                if (nmap == 0) {
                        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;
                start = XFS_FSB_TO_B(mp, fsbno);
    }

But we have a similar check up at the begnning of xfs_seek_data(), will it looks a bit weird?

Thanks,
-Jeff

Quoted 4 lines

Cheers,

Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs