xfs
[Top] [All Lists]

Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buff

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
From: Jeff liu <jeff.liu@xxxxxxxxxx>
Date: Sat, 28 Jul 2012 12:09:19 +0800
Cc: xfs@xxxxxxxxxxx
In-reply-to: <501308E6.3030207@xxxxxxx>
References: <50110630.7020902@xxxxxxxxxx> <50116322.2020005@xxxxxxxxxx> <501308E6.3030207@xxxxxxx>
在 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@xxxxxxxxxx>
> 
> 
> ...
> 
> 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@xxxxxxx>
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>