On Thu, Mar 14, 2019 at 09:17:14AM +0200, Nikolay Borisov wrote: > > > 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? The errors that can be returned from add_to_page_cache_lru are deep inside the MM code, I traced it to the xarray helpers that now manage the page cache. > > > > 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. My concerns were about errors that could affect logic of the readpages, for the hard errors like ENOMEM we can't do much. > > 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@xxxxxxxx> > > > > <snip>
