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.
Possible error is EEXSIT, here it's clear that we don't need to read the
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>
>
> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
>
> I've been running with this patch for the past 3 months and haven't encountered
> any issues with it.
> fs/btrfs/extent_io.c | 58 +++++++++++---------------------------------
> 1 file changed, 14 insertions(+), 44 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b20700ad8752..551dd21d7351 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3066,7 +3066,7 @@ static int __do_readpage(struct extent_io_tree *tree,
> return ret;
> }
>
> -static inline void __do_contiguous_readpages(struct extent_io_tree *tree,
> +static inline void contiguous_readpages(struct extent_io_tree *tree,
> struct page *pages[], int nr_pages,
> u64 start, u64 end,
> struct extent_map **em_cached,
> @@ -3097,46 +3097,6 @@ static inline void __do_contiguous_readpages(struct extent_io_tree *tree,
> }
> }
>
> -static void __extent_readpages(struct extent_io_tree *tree,
> - struct page *pages[],
> - int nr_pages,
> - struct extent_map **em_cached,
> - struct bio **bio, unsigned long *bio_flags,
> - u64 *prev_em_start)
> -{
> - u64 start = 0;
> - u64 end = 0;
> - u64 page_start;
> - int index;
> - int first_index = 0;
> -
> - for (index = 0; index < nr_pages; index++) {
> - page_start = page_offset(pages[index]);
> - if (!end) {
> - start = page_start;
> - end = start + PAGE_SIZE - 1;
> - first_index = index;
> - } else if (end + 1 == page_start) {
> - end += PAGE_SIZE;
> - } else {
> - __do_contiguous_readpages(tree, &pages[first_index],
> - index - first_index, start,
> - end, em_cached,
> - bio, bio_flags,
> - prev_em_start);
> - start = page_start;
> - end = start + PAGE_SIZE - 1;
> - first_index = index;
> - }
> - }
> -
> - if (end)
> - __do_contiguous_readpages(tree, &pages[first_index],
> - index - first_index, start,
> - end, em_cached, bio,
> - bio_flags, prev_em_start);
> -}
> -
> static int __extent_read_full_page(struct extent_io_tree *tree,
> struct page *page,
> get_extent_t *get_extent,
> @@ -4098,6 +4058,8 @@ int extent_readpages(struct address_space *mapping, struct list_head *pages,
> u64 prev_em_start = (u64)-1;
>
> while (!list_empty(pages)) {
> + u64 contig_end = 0;
> +
> for (nr = 0; nr < ARRAY_SIZE(pagepool) && !list_empty(pages);) {
> struct page *page = lru_to_page(pages);
>
> @@ -4106,14 +4068,22 @@ int extent_readpages(struct address_space *mapping, struct list_head *pages,
> if (add_to_page_cache_lru(page, mapping, page->index,
> readahead_gfp_mask(mapping))) {
> put_page(page);
> - continue;
> + break;
> }
>
> pagepool[nr++] = page;
> + contig_end = page_offset(page) + PAGE_SIZE - 1;
> }
>
> - __extent_readpages(tree, pagepool, nr, &em_cached, &bio,
> - &bio_flags, &prev_em_start);
> + if (nr) {
> + u64 contig_start = page_offset(pagepool[0]);
> +
> + ASSERT(contig_start + (nr*PAGE_SIZE) - 1 == contig_end);
> +
> + contiguous_readpages(tree, pagepool, nr, contig_start,
> + contig_end, &em_cached, &bio, &bio_flags,
> + &prev_em_start);
> + }
> }
>
> if (em_cached)
> --
> 2.17.1
