* [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