On 07/31/2012 04:00 AM, Mark Tinguely wrote:
> On 07/26/12 10:32, Jeff Liu wrote:
>> This function is called by xfs_seek_data() and xfs_seek_hole() to find
>> the desired offset from page cache.
>>
>> Signed-off-by: Jie Liu<jeff.liu@xxxxxxxxxx>
>
>
> Hopefully, I am not being a pain....
Never. :)
>
> I just noticed that if trylock() failed you return found==0.
> Wouldn't it be safer/more correct to assume a page that failed a
> try_lock to be data?
I'm afraid to assume a page is data per lock failed will cause an inaccurate result, because it might be a hole.
>
>
>> + if (nr_pages == 0) {
>> + if (type == HOLE_OFF) {
>> + if (coff == *offset)
>> + found = true;
>
> is this necessary? wouldn't the next test also cover the above condition?
They are two different scenarios in this point as I have mentioned in comments, but they can be merged into one line,
i.e, in either case, for searching holes, "*offset = coff and found = true".
>
>> + if (coff< endoff) {
>> + found = true;
>> + *offset = coff;
>> + }
>> + }
>
>
> I like informative comments, but some are bit verbose. I will pick on
> this one:
>
>
> + /*
> + * Page index is out of range, we need to deal with
> + * hole search condition in paticular if that is the
> + * desired type for the lookup.
> + * stepping into the block buffer checkup, it probably
> + * means that there is no page mapped at all in the
> + * specified range to search, so we found a hole.
> + * If we have already done some block buffer checking
> + * and found one or more data buffers before, in this
> + * case, the coff is already updated and it point to
> + * the end of the last data buffer, so the left range
> + * behind it might be a hole. In either case, we will
> + * return the coff to indicate a hole's location because
> + * it must be greater than or equal to the search start.
> + */
>
> just a crude simplification - maybe it is too terse:
> /*
> * coff is the current offset of the page being tested.
> * If the next page index is beyond the extent of interest,
> * then we are done searching with the data search is
> * false and hole search is true at the last coff.
> */
Exactly, thank you!
>
> For holes you are looking for (page->index != coff) for every page, but
> in a indirect way. It had me a little confused, but eventually I figured
> it out. I am not sure if a doing that comparison directly would overly
> complicate the data search path.
The current implements really looks complex, I will revise it combine with Dave's comments.
Hopefully, those things would looks a bit simpler for my next try.
Thanks,
-Jeff
>
> Good work.
>
> --Mark.
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
