* [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
@ 2012-07-26 8:56 Jeff Liu
2012-07-26 15:32 ` Jeff Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Liu @ 2012-07-26 8:56 UTC (permalink / raw)
To: xfs
Search data buffer offset for given range from page cache for unwritten extents.
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_file.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 70 insertions(+), 18 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 43f5e61..15acd4d 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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)
--
1.7.4.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
2012-07-26 8:56 [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache Jeff Liu
@ 2012-07-26 15:32 ` Jeff Liu
2012-07-27 21:32 ` Mark Tinguely
2012-07-31 0:06 ` Dave Chinner
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Liu @ 2012-07-26 15:32 UTC (permalink / raw)
To: xfs
Search data buffer offset for given range from page cache.
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_file.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 70 insertions(+), 18 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 69965a4..b1158b3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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)
--
1.7.4.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
2012-07-26 15:32 ` Jeff Liu
@ 2012-07-27 21:32 ` Mark Tinguely
2012-07-28 4:09 ` Jeff liu
2012-07-31 0:06 ` Dave Chinner
1 sibling, 1 reply; 6+ messages in thread
From: Mark Tinguely @ 2012-07-27 21:32 UTC (permalink / raw)
To: jeff.liu; +Cc: xfs
On 07/26/12 10:32, Jeff Liu wrote:
> Search data buffer offset for given range from page cache.
>
> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
...
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:
> 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
> + if (map[0].br_startblock == DELAYSTARTBLOCK ||
> + map[0].br_state == XFS_EXT_UNWRITTEN) {
here is where it is unwritten extent
> + /* 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
> + if (nmap == 1) {
here is where there is nothing in map[1] and ...
> + 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.
> + */
it could be unwritten or a hole
> + if (map[1].br_startblock == DELAYSTARTBLOCK ||
> + map[1].br_state == XFS_EXT_UNWRITTEN) {
here is where we know it is unwritten.
> + 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 <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
2012-07-27 21:32 ` Mark Tinguely
@ 2012-07-28 4:09 ` Jeff liu
0 siblings, 0 replies; 6+ messages in thread
From: Jeff liu @ 2012-07-28 4:09 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
在 2012-7-28，上午5:32， Mark Tinguely 写道：
> On 07/26/12 10:32, Jeff Liu wrote:
>> Search data buffer offset for given range from page cache.
>>
>> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
>
>
> ...
>
> 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:
>
>
>> 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
>> + if (map[0].br_startblock == DELAYSTARTBLOCK ||
>> + map[0].br_state == XFS_EXT_UNWRITTEN) {
>
> here is where it is unwritten extent
>> + /* 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
>> + if (nmap == 1) {
> here is where there is nothing in map[1] and ...
>> + 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.
>> + */
>
> it could be unwritten or a hole
>> + if (map[1].br_startblock == DELAYSTARTBLOCK ||
>> + map[1].br_state == XFS_EXT_UNWRITTEN) {
>
> here is where we know it is unwritten.
>> + 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
>
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
2012-07-26 15:32 ` Jeff Liu
2012-07-27 21:32 ` Mark Tinguely
@ 2012-07-31 0:06 ` Dave Chinner
2012-07-31 8:03 ` Jie Liu
1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-07-31 0:06 UTC (permalink / raw)
To: Jeff Liu; +Cc: xfs
On Thu, Jul 26, 2012 at 11:32:50PM +0800, Jeff Liu wrote:
> Search data buffer offset for given range from page cache.
>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>
> ---
> fs/xfs/xfs_file.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 70 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 69965a4..b1158b3 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -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
Not correct - hole, delay or unwritten land here.
> + * 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....
> + /*
> + * 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;
> + }
> + }
And the if/elseif/else logic above can be repeated here.
> + }
> +
> + /*
> + * 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
2012-07-31 0:06 ` Dave Chinner
@ 2012-07-31 8:03 ` Jie Liu
0 siblings, 0 replies; 6+ messages in thread
From: Jie Liu @ 2012-07-31 8:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 07/31/12 08:06, Dave Chinner wrote:
> On Thu, Jul 26, 2012 at 11:32:50PM +0800, Jeff Liu wrote:
>> Search data buffer offset for given range from page cache.
>>
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> Reviewed-by: Mark Tinguely <tinguely@sgi.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>>
>> ---
>> fs/xfs/xfs_file.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------
>> 1 files changed, 70 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index 69965a4..b1158b3 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -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
> Not correct - hole, delay or unwritten land here.
Will fix it.
>
>> + * 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....
Just done a quick test with above modifications, it works to me, thanks.
>
>
>> + /*
>> + * 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;
>> + }
>> + }
> And the if/elseif/else logic above can be repeated here.
>
>> + }
>> +
>> + /*
>> + * 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
>
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-31 8:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-26 8:56 [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache Jeff Liu
2012-07-26 15:32 ` Jeff Liu
2012-07-27 21:32 ` Mark Tinguely
2012-07-28 4:09 ` Jeff liu
2012-07-31 0:06 ` Dave Chinner
2012-07-31 8:03 ` Jie Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox