Re: [PATCH] btrfs: Exploit the fact pages passed to extent_readpages are always contiguous

[Nikolay Borisov <nborisov@suse.com>]

On 13.03.19 г. 19:27 ч., David Sterba wrote:
> On Mon, Mar 11, 2019 at 09:55:38AM +0200, Nikolay Borisov wrote:
>> Currently extent_readpages (called from btrfs_redpages) will always call
>> __extent_readpages which tries to create contiguous range of pages and
>> call __do_contiguous_readpages when such contiguous range is created.
>>
>> It turns out this is unnecessary due to the fact that generic VFS code
>> always calls filesystem's ->readpages callback (btrfs_readpages in
>> this case) with already contiguous pages. Armed with this knowledge it's
>> possible to simplify extent_readpages by eliminating the call to
>> __extent_readpages and directly calling contiguous_readpages. The only
>> edge case that needs to be handled is when add_to_page_cache_lru
>> fails. This is easy as all that is needed is to submit whatever is the
>> number of pages successfully added to the lru.
> 
> I'd like to have more details why this is correct. Submitting what we
> have so far seems ok, the reasons why add_to_page_cache_lru are unclear
> and go back to xarray.

I'm having a hard time parsing the sentence after the comma, could you
rephrase that?

> 
> Possible error is EEXSIT, here it's clear that we don't need to read the

Not only EEXIST, the code can also fail due to memory pressure from
mem_cgroup_try_charge in __add_to_page_cache_locked. In fact this code
was triggered by some of the xfstests I can't remember which one hence I
added the if (nr) check.

> pages (already there). A sequence of such pages will make it wildly hop
> from 1st for cycle iteration, to the if (nr) check and back.
> 
> Previously this looped still in the for-cycle, which was easier to
> follow but otherwise not due to __extent_readpages. So I think it's an
> improvement in the end, though a few comments would be good there.
> 
> Reviewed-by: David Sterba <dsterba@suse.com>
> 

<snip>