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

[Jeff liu]

在 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 at 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 at sgi.com>
> 
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs