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@xxxxxxxxxx>
>>
>> ---
>> 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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
