Re: [PATCH v5 3/3] squashfs: implement readahead

[Hsin-Yi Wang <hsinyi@chromium.org>]

[-- Attachment #1: Type: text/plain, Size: 6942 bytes --]

On Sat, Jun 11, 2022 at 1:23 PM Phillip Lougher <phillip@squashfs.org.uk>
wrote:
>
> On 06/06/2022 16:03, Hsin-Yi Wang wrote:
> > Implement readahead callback for squashfs. It will read datablocks
> > which cover pages in readahead request. For a few cases it will
> > not mark page as uptodate, including:
> > - file end is 0.
> > - zero filled blocks.
> > - current batch of pages isn't in the same datablock.
> > - decompressor error.
> > Otherwise pages will be marked as uptodate. The unhandled pages will be
> > updated by readpage later.
> >
>
> Hi Hsin-Yi,
>
> I have reviewed, tested and instrumented the following patch.
>
> There are a number of problems with the patch including
> performance, unhandled issues, and bugs.
>
> In this email I'll concentrate on the performance aspects.
>
> The major change between this V5 patch and the previous patches
> (V4 etc), is that it now handles the case where
>
> + nr_pages = __readahead_batch(ractl, pages, max_pages);
>
> returns an "nr_pages" less than "max_pages".
>
> What this means is that the readahead code has returned a set
> of page cache pages which does not fully map the datablock to
> be decompressed.
>
> If this is passed to squashfs_read_data() using the current
> "page actor" code, the decompression will fail on the missing
> pages.
>
> In recognition of that fact, your V5 patch falls back to using
> the earlier intermediate buffer method, with
> squashfs_get_datablock() returning a buffer, which is then memcopied
> into the page cache pages.
>
> This is currently what is also done in the existing
> squashfs_readpage_block() function if the entire set of pages cannot
> be obtained.
>

hi Phillip,

I think there's still one difference between fallback to .readfolio (v4)
and v5:

If the remaining pages (nr_pages < max_pages) are fallbacked to .readfolio,
each single page will be handled by squashfs_readpage_block().
In the block that handles a single page, the for loop in function
squashfs_readpage_block() will fill the other 31 pages to null. Later in
squashfs_read_cache(), there's also a loop that will go through all 32
pages just to handle a page (other 31 are null).

In v5, we just need to run the for loop once to handle the remaining pages,
thus we can save a constant (32) for looping through null pages compared to
v4. But the impact might still be small, comparing to using intermediate
buffer.

I'll rebase this series on your series. Also thanks for providing the diff.


Hsin-Yi

> The problem with this fallback intermediate buffer is it is slow, both
> due to the additional memcopies, but, more importantly because it
> introduces contention on a single shared buffer.
>
> I have long had the intention to fix this performance issue in
> squashfs_readpage_block(), but, due it being a rare issue there, the
> additional work has seemed to be nice but not essential.
>
> The problem is we don't want the readahead code to be using this
> slow method, because the scenario will probably happen much more
> often, and for a performance improvement patch, falling back to
> an old slow method isn't very useful.
>
> So I have finally done the work to make the "page actor" code handle
> missing pages.
>
> This I have sent out in the following patch-set updating the
> squashfs_readpage_block() function to use it.
>
>
https://lore.kernel.org/lkml/20220611032133.5743-1-phillip@squashfs.org.uk/
>
> You can use this updated "page actor" code to eliminate the
> "nr_pages < max_pages" special case in your patch.  With the benefit
> that decompression is done directly into the page cache.
>
> I have updated your patch to use the new functionality.  The diff
> including a bug fix I have appended to this email.
>
> Phillip
>
> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> index b86b2f9d9ae6..721d35ecfca9 100644
> --- a/fs/squashfs/file.c
> +++ b/fs/squashfs/file.c
> @@ -519,10 +519,6 @@ static void squashfs_readahead(struct
> readahead_control *ractl)
>         if (!pages)
>                 return;
>
> -       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> -       if (!actor)
> -               goto out;
> -
>         for (;;) {
>                 pgoff_t index;
>                 int res, bsize;
> @@ -548,41 +544,21 @@ static void squashfs_readahead(struct
> readahead_control *ractl)
>                 if (bsize == 0)
>                         goto skip_pages;
>
> -               if (nr_pages < max_pages) {
> -                       struct squashfs_cache_entry *buffer;
> -                       unsigned int block_mask = max_pages - 1;
> -                       int offset = pages[0]->index - (pages[0]->index &
~block_mask);
> -
> -                       buffer = squashfs_get_datablock(inode->i_sb,
block,
> -                                                       bsize);
> -                       if (buffer->error) {
> -                               squashfs_cache_put(buffer);
> -                               goto skip_pages;
> -                       }
> -
> -                       expected -= offset * PAGE_SIZE;
> -                       for (i = 0; i < nr_pages && expected > 0; i++,
> -                                               expected -= PAGE_SIZE,
offset++) {
> -                               int avail = min_t(int, expected,
PAGE_SIZE);
> -
> -                               squashfs_fill_page(pages[i], buffer,
> -                                               offset * PAGE_SIZE,
avail);
> -                               unlock_page(pages[i]);
> -                       }
> -
> -                       squashfs_cache_put(buffer);
> -                       continue;
> -               }
> +               actor = squashfs_page_actor_init_special(msblk, pages,
nr_pages,
> expected);
> +               if (!actor)
> +                       goto out;
>
>                 res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
>                                          actor);
>
> +               kfree(actor);
> +
>                 if (res == expected) {
>                         int bytes;
>
> -                       /* Last page may have trailing bytes not filled */
> +                       /* Last page (if present) may have trailing bytes
not filled */
>                         bytes = res % PAGE_SIZE;
> -                       if (bytes) {
> +                       if (pages[nr_pages - 1]->index == file_end &&
bytes) {
>                                 void *pageaddr;
>
>                                 pageaddr = kmap_atomic(pages[nr_pages -
1]);
> @@ -602,7 +578,6 @@ static void squashfs_readahead(struct
> readahead_control *ractl)
>                 }
>         }
>
> -       kfree(actor);
>         kfree(pages);
>         return;
>
> @@ -612,7 +587,6 @@ static void squashfs_readahead(struct
> readahead_control *ractl)
>                 put_page(pages[i]);
>         }
>
> -       kfree(actor);
>   out:
>         kfree(pages);
>   }
> --
> 2.34.1

[-- Attachment #2: Type: text/html, Size: 9453 bytes --]