Re: Readahead regressed with c1f6925e1091("mm: put readahead pages in cache earlier") on multicore arm64 platforms

Re: Readahead regressed with c1f6925e1091("mm: put readahead pages in cache earlier") on multicore arm64 platforms

[Matthew Wilcox]

On Thu, Oct 07, 2021 at 03:08:38PM +0800, Hsin-Yi Wang wrote:
> This calls into squashfs_readpage().

Aha!  I hadn't looked at squashfs before, and now that I do, I can
see why this commit causes problems for squashfs.  (It would be
helpful if your report included more detail about which paths inside
squashfs were taken, but I think I can guess):

squashfs_readpage()
  squashfs_readpage_block()
    squashfs_copy_cache()
      grab_cache_page_nowait()

Before this patch, readahead of 1MB would allocate 256x4kB pages,
then add each one to the page cache and call ->readpage on it:

        for (page_idx = 0; page_idx < readahead_count(rac); page_idx++) {
                struct page *page = lru_to_page(pages);
                list_del(&page->lru);
                if (!add_to_page_cache_lru(page, rac->mapping, page->index,
                               gfp))
                        aops->readpage(rac->file, page);

When Squashfs sees it has more than 4kB of data, it calls
grab_cache_page_nowait(), which allocates more memory (ignoring the
other 255 pages which have been allocated, because they're not in the
page cache yet).  Then this loop frees the pages that readahead
allocated.

After this patch, the pages are already in the page cache when
->readpage is called the first time.  So the call to
grab_cache_page_nowait() fails and squashfs redoes the decompression for
each page.

Neither of these approaches are efficient.  Squashfs need to implement
->readahead.  Working on it now ...

Re: Readahead regressed with c1f6925e1091("mm: put readahead pages in cache earlier") on multicore arm64 platforms

[Hsin-Yi Wang]

On Thu, Oct 7, 2021 at 9:46 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Oct 07, 2021 at 03:08:38PM +0800, Hsin-Yi Wang wrote:
> > This calls into squashfs_readpage().
>
> Aha!  I hadn't looked at squashfs before, and now that I do, I can
> see why this commit causes problems for squashfs.  (It would be
> helpful if your report included more detail about which paths inside
> squashfs were taken, but I think I can guess):
>
> squashfs_readpage()
>   squashfs_readpage_block()
>     squashfs_copy_cache()
>       grab_cache_page_nowait()
>
Right, before the patch, push_page won't be null but after the patch,
grab_cache_page_nowait() fails.


> Before this patch, readahead of 1MB would allocate 256x4kB pages,
> then add each one to the page cache and call ->readpage on it:
>
>         for (page_idx = 0; page_idx < readahead_count(rac); page_idx++) {
>                 struct page *page = lru_to_page(pages);
>                 list_del(&page->lru);
>                 if (!add_to_page_cache_lru(page, rac->mapping, page->index,
>                                gfp))
>                         aops->readpage(rac->file, page);
>
> When Squashfs sees it has more than 4kB of data, it calls
> grab_cache_page_nowait(), which allocates more memory (ignoring the
> other 255 pages which have been allocated, because they're not in the
> page cache yet).  Then this loop frees the pages that readahead
> allocated.
>
> After this patch, the pages are already in the page cache when
> ->readpage is called the first time.  So the call to
> grab_cache_page_nowait() fails and squashfs redoes the decompression for
> each page.
>
> Neither of these approaches are efficient.  Squashfs need to implement
> ->readahead.  Working on it now ...
>