Re: [PATCH v5 2/4]xfs: Introduce a new function to find the desired type of offset from page cache

[Jeff Liu <jeff.liu@oracle.com>]

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


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs