squashfs-devel

Re: [Squashfs-devel] [PATCH 2/2] squashfs: implement readahead

[kernel t. r. <lk...@in...>]

Hi Hsin-Yi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220513]
[cannot apply to akpm-mm/mm-everything v5.18-rc7 v5.18-rc6 v5.18-rc5 v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
base:    1e1b28b936aed946122b4e0991e7144fdbbfd77e
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220516/202...@in.../config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
        git checkout 573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/squashfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lk...@in...>

All warnings (new ones prefixed by >>):

   fs/squashfs/file.c: In function 'squashfs_readahead':
   fs/squashfs/file.c:526:17: error: implicit declaration of function 'squashfs_page_actor_init_special'; did you mean 'squashfs_page_actor_init'? [-Werror=implicit-function-declaration]
     526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                 squashfs_page_actor_init
>> fs/squashfs/file.c:526:15: warning: assignment to 'struct squashfs_page_actor *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
         |               ^
   fs/squashfs/file.c: At top level:
   fs/squashfs/file.c:577:9: error: request for member 'readahead' in something not a structure or union
     577 |         .readahead = squashfs_readahead
         |         ^
   cc1: some warnings being treated as errors


vim +526 fs/squashfs/file.c

   498	
   499	static void squashfs_readahead(struct readahead_control *ractl)
   500	{
   501		struct inode *inode = ractl->mapping->host;
   502		struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
   503		size_t mask = (1UL << msblk->block_log) - 1;
   504		size_t shift = msblk->block_log - PAGE_SHIFT;
   505		loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
   506		loff_t start = readahead_pos(ractl) &~ mask;
   507		size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
   508		struct squashfs_page_actor *actor;
   509		unsigned int nr_pages = 0;
   510		struct page **pages;
   511		u64 block = 0;
   512		int bsize, res, i, index;
   513		int file_end = i_size_read(inode) >> msblk->block_log;
   514		unsigned int max_pages = 1UL << shift;
   515	
   516		readahead_expand(ractl, start, (len | mask) + 1);
   517	
   518		if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
   519		    file_end == 0)
   520			return;
   521	
   522		pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
   523		if (!pages)
   524			return;
   525	
 > 526		actor = squashfs_page_actor_init_special(pages, max_pages, 0);
   527		if (!actor)
   528			goto out;
   529	
   530		for (;;) {
   531			nr_pages = __readahead_batch(ractl, pages, max_pages);
   532			if (!nr_pages)
   533				break;
   534	
   535			if (readahead_pos(ractl) >= i_size_read(inode) ||
   536			    nr_pages < max_pages)
   537				goto skip_pages;
   538	
   539			index = pages[0]->index >> shift;
   540			if ((pages[nr_pages - 1]->index >> shift) != index)
   541				goto skip_pages;
   542	
   543			bsize = read_blocklist(inode, index, &block);
   544			if (bsize == 0)
   545				goto skip_pages;
   546	
   547			res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
   548						 actor);
   549	
   550			if (res >= 0)
   551				for (i = 0; i < nr_pages; i++)
   552					SetPageUptodate(pages[i]);
   553	
   554			for (i = 0; i < nr_pages; i++) {
   555				unlock_page(pages[i]);
   556				put_page(pages[i]);
   557			}
   558		}
   559	
   560		kfree(actor);
   561		kfree(pages);
   562		return;
   563	
   564	skip_pages:
   565		for (i = 0; i < nr_pages; i++) {
   566			unlock_page(pages[i]);
   567			put_page(pages[i]);
   568		}
   569	
   570		kfree(actor);
   571	out:
   572		kfree(pages);
   573	}
   574	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[Squashfs-devel] [PATCH 0/3] Implement readahead for squashfs

[Hsin-Yi W. <hs...@ch...>]

Commit c1f6925e1091("mm: put readahead pages in cache earlier") requires
fs to implement readahead callback. Otherwise there will be a
performance regression.

Commit 9eec1d897139("squashfs: provide backing_dev_info in order to
disable read-ahead") mitigates the performance drop issue for squashfs
by closing readahead for it.

This series implements readahead callback for squashfs. The previous
discussion are in [1] and [2].

[1] https://lore.kernel.org/all/CAJMQK-g9G6KQmH-V=BRG...@ma.../T/
[2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m4af4473b94f98a4996cb11756b633a07e5e059d1

Hsin-Yi Wang (3):
  Revert "squashfs: Convert squashfs to read_folio"
  Revert "squashfs: provide backing_dev_info in order to disable
    read-ahead"
  squashfs: implement readahead

 fs/squashfs/file.c    | 82 +++++++++++++++++++++++++++++++++++++++++--
 fs/squashfs/super.c   | 33 -----------------
 fs/squashfs/symlink.c |  5 ++-
 3 files changed, 81 insertions(+), 39 deletions(-)

-- 
2.36.0.550.gb090851708-goog

Re: [Squashfs-devel] [PATCH 0/3] Implement readahead for squashfs

[Hsin-Yi W. <hs...@ch...>]

On Mon, May 16, 2022 at 6:42 PM Hsin-Yi Wang <hs...@ch...> wrote:
>
> Commit c1f6925e1091("mm: put readahead pages in cache earlier") requires
> fs to implement readahead callback. Otherwise there will be a
> performance regression.
>
> Commit 9eec1d897139("squashfs: provide backing_dev_info in order to
> disable read-ahead") mitigates the performance drop issue for squashfs
> by closing readahead for it.
>
> This series implements readahead callback for squashfs. The previous
> discussion are in [1] and [2].
>
> [1] https://lore.kernel.org/all/CAJMQK-g9G6KQmH-V=BRG...@ma.../T/
> [2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m4af4473b94f98a4996cb11756b633a07e5e059d1
>
> Hsin-Yi Wang (3):
>   Revert "squashfs: Convert squashfs to read_folio"
>   Revert "squashfs: provide backing_dev_info in order to disable
>     read-ahead"
>   squashfs: implement readahead
>
>  fs/squashfs/file.c    | 82 +++++++++++++++++++++++++++++++++++++++++--
>  fs/squashfs/super.c   | 33 -----------------
>  fs/squashfs/symlink.c |  5 ++-
>  3 files changed, 81 insertions(+), 39 deletions(-)
>
> --
> 2.36.0.550.gb090851708-goog
>

hi, please ignore this for now. I'll resend a correct cover letter.

[Squashfs-devel] [PATCH v2 0/3] Implement readahead for squashfs

[Hsin-Yi W. <hs...@ch...>]

Commit c1f6925e1091("mm: put readahead pages in cache earlier") requires
fs to implement readahead callback. Otherwise there will be a
performance regression.

Commit 9eec1d897139("squashfs: provide backing_dev_info in order to
disable read-ahead") mitigates the performance drop issue for squashfs
by closing readahead for it.

This series implements readahead callback for squashfs. The previous
discussion are in [1] and [2].

[1] https://lore.kernel.org/all/CAJMQK-g9G6KQmH-V=BRG...@ma.../T/
[2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m4af4473b94f98a4996cb11756b633a07e5e059d1

Hsin-Yi Wang (2):
  Revert "squashfs: provide backing_dev_info in order to disable
    read-ahead"
  squashfs: implement readahead

Phillip Lougher (1):
  squashfs: always build "file direct" version of page actor

 fs/squashfs/Makefile     |  4 +--
 fs/squashfs/file.c       | 77 +++++++++++++++++++++++++++++++++++++++-
 fs/squashfs/page_actor.h | 41 ---------------------
 fs/squashfs/super.c      | 33 -----------------
 4 files changed, 78 insertions(+), 77 deletions(-)

-- 
2.36.0.550.gb090851708-goog

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Xiongwei S. <sx...@gm...>]

On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
>   datablock.
> Otherwise pages will be marked as uptodate. The unhandled pages will be
> updated by readpage later.
>
> Suggested-by: Matthew Wilcox <wi...@in...>
> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> Reported-by: Matthew Wilcox <wi...@in...>
> Reported-by: Phillip Lougher <ph...@sq...>
> Reported-by: Xiongwei Song <sx...@gm...>

"xio...@wi..." too.

> ---
> v1->v2: remove unused check on readahead_expand().
> v1: https://lore.kernel.org/lkml/202...@ch.../
> ---
>  fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> index a8e495d8eb86..e10a55c5b1eb 100644
> --- a/fs/squashfs/file.c
> +++ b/fs/squashfs/file.c
> @@ -39,6 +39,7 @@
>  #include "squashfs_fs_sb.h"
>  #include "squashfs_fs_i.h"
>  #include "squashfs.h"
> +#include "page_actor.h"
>
>  /*
>   * Locate cache slot in range [offset, index] for specified inode.  If
> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>         return 0;
>  }
>
> +static void squashfs_readahead(struct readahead_control *ractl)
> +{
> +       struct inode *inode = ractl->mapping->host;
> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
> +       size_t mask = (1UL << msblk->block_log) - 1;
> +       size_t shift = msblk->block_log - PAGE_SHIFT;
> +       loff_t start = readahead_pos(ractl) &~ mask;
> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> +       struct squashfs_page_actor *actor;
> +       unsigned int nr_pages = 0;
> +       struct page **pages;
> +       u64 block = 0;
> +       int bsize, res, i, index;
> +       int file_end = i_size_read(inode) >> msblk->block_log;
> +       unsigned int max_pages = 1UL << shift;
> +
> +       readahead_expand(ractl, start, (len | mask) + 1);
> +
> +       if (file_end == 0)
> +               return;
> +
> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
> +       if (!pages)
> +               return;
> +
> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> +       if (!actor)
> +               goto out;
> +
> +       for (;;) {
> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
> +               if (!nr_pages)
> +                       break;
> +
> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
> +                   nr_pages < max_pages)
> +                       goto skip_pages;
> +
> +               index = pages[0]->index >> shift;
> +               if ((pages[nr_pages - 1]->index >> shift) != index)
> +                       goto skip_pages;
> +
> +               bsize = read_blocklist(inode, index, &block);
> +               if (bsize == 0)
> +                       goto skip_pages;
> +
> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
> +                                        actor);
> +
> +               if (res >= 0)
> +                       for (i = 0; i < nr_pages; i++)
> +                               SetPageUptodate(pages[i]);
> +
> +               for (i = 0; i < nr_pages; i++) {
> +                       unlock_page(pages[i]);
> +                       put_page(pages[i]);
> +               }
> +       }
> +
> +       kfree(actor);
> +       kfree(pages);
> +       return;
> +
> +skip_pages:
> +       for (i = 0; i < nr_pages; i++) {
> +               unlock_page(pages[i]);
> +               put_page(pages[i]);
> +       }
> +
> +       kfree(actor);
> +out:
> +       kfree(pages);
> +}
>
>  const struct address_space_operations squashfs_aops = {
> -       .read_folio = squashfs_read_folio
> +       .read_folio = squashfs_read_folio,
> +       .readahead = squashfs_readahead
>  };
> --
> 2.36.0.550.gb090851708-goog
>
>

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

On Tue, May 17, 2022 at 5:17 PM Xiongwei Song <sx...@gm...> wrote:
>
> On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
> >   datablock.
> > Otherwise pages will be marked as uptodate. The unhandled pages will be
> > updated by readpage later.
> >
> > Suggested-by: Matthew Wilcox <wi...@in...>
> > Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> > Reported-by: Matthew Wilcox <wi...@in...>
> > Reported-by: Phillip Lougher <ph...@sq...>
> > Reported-by: Xiongwei Song <sx...@gm...>
>
> "xio...@wi..." too.
>
ack.
<snip>

[Squashfs-devel] [PATCH v2 1/3] Revert "squashfs: provide backing_dev_info in order to disable read-ahead"

[Hsin-Yi W. <hs...@ch...>]

This reverts commit 9eec1d897139e5de287af5d559a02b811b844d82.

Revert closing the readahead to squashfs since the readahead callback
for squashfs is implemented.

Suggested-by: Xiongwei Song <sx...@gm...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
 fs/squashfs/super.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 6d594ba2ed28..32565dafa7f3 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -29,7 +29,6 @@
 #include <linux/module.h>
 #include <linux/magic.h>
 #include <linux/xattr.h>
-#include <linux/backing-dev.h>
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -113,24 +112,6 @@ static const struct squashfs_decompressor *supported_squashfs_filesystem(
 	return decompressor;
 }
 
-static int squashfs_bdi_init(struct super_block *sb)
-{
-	int err;
-	unsigned int major = MAJOR(sb->s_dev);
-	unsigned int minor = MINOR(sb->s_dev);
-
-	bdi_put(sb->s_bdi);
-	sb->s_bdi = &noop_backing_dev_info;
-
-	err = super_setup_bdi_name(sb, "squashfs_%u_%u", major, minor);
-	if (err)
-		return err;
-
-	sb->s_bdi->ra_pages = 0;
-	sb->s_bdi->io_pages = 0;
-
-	return 0;
-}
 
 static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
@@ -146,20 +127,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	TRACE("Entered squashfs_fill_superblock\n");
 
-	/*
-	 * squashfs provides 'backing_dev_info' in order to disable read-ahead. For
-	 * squashfs, I/O is not deferred, it is done immediately in read_folio,
-	 * which means the user would always have to wait their own I/O. So the effect
-	 * of readahead is very weak for squashfs. squashfs_bdi_init will set
-	 * sb->s_bdi->ra_pages and sb->s_bdi->io_pages to 0 and close readahead for
-	 * squashfs.
-	 */
-	err = squashfs_bdi_init(sb);
-	if (err) {
-		errorf(fc, "squashfs init bdi failed");
-		return err;
-	}
-
 	sb->s_fs_info = kzalloc(sizeof(*msblk), GFP_KERNEL);
 	if (sb->s_fs_info == NULL) {
 		ERROR("Failed to allocate squashfs_sb_info\n");
-- 
2.36.0.550.gb090851708-goog

Re: [Squashfs-devel] [PATCH v2 1/3] Revert "squashfs: provide backing_dev_info in order to disable read-ahead"

[Xiongwei S. <sx...@gm...>]

On Tue, May 17, 2022 at 4:27 PM Hsin-Yi Wang <hs...@ch...> wrote:
>
> This reverts commit 9eec1d897139e5de287af5d559a02b811b844d82.
>
> Revert closing the readahead to squashfs since the readahead callback
> for squashfs is implemented.
>
> Suggested-by: Xiongwei Song <sx...@gm...>

I'd like to use "xio...@wi..." in my tag.

> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> ---
>  fs/squashfs/super.c | 33 ---------------------------------
>  1 file changed, 33 deletions(-)
>
> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
> index 6d594ba2ed28..32565dafa7f3 100644
> --- a/fs/squashfs/super.c
> +++ b/fs/squashfs/super.c
> @@ -29,7 +29,6 @@
>  #include <linux/module.h>
>  #include <linux/magic.h>
>  #include <linux/xattr.h>
> -#include <linux/backing-dev.h>
>
>  #include "squashfs_fs.h"
>  #include "squashfs_fs_sb.h"
> @@ -113,24 +112,6 @@ static const struct squashfs_decompressor *supported_squashfs_filesystem(
>         return decompressor;
>  }
>
> -static int squashfs_bdi_init(struct super_block *sb)
> -{
> -       int err;
> -       unsigned int major = MAJOR(sb->s_dev);
> -       unsigned int minor = MINOR(sb->s_dev);
> -
> -       bdi_put(sb->s_bdi);
> -       sb->s_bdi = &noop_backing_dev_info;
> -
> -       err = super_setup_bdi_name(sb, "squashfs_%u_%u", major, minor);
> -       if (err)
> -               return err;
> -
> -       sb->s_bdi->ra_pages = 0;
> -       sb->s_bdi->io_pages = 0;
> -
> -       return 0;
> -}
>
>  static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
> @@ -146,20 +127,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>
>         TRACE("Entered squashfs_fill_superblock\n");
>
> -       /*
> -        * squashfs provides 'backing_dev_info' in order to disable read-ahead. For
> -        * squashfs, I/O is not deferred, it is done immediately in read_folio,
> -        * which means the user would always have to wait their own I/O. So the effect
> -        * of readahead is very weak for squashfs. squashfs_bdi_init will set
> -        * sb->s_bdi->ra_pages and sb->s_bdi->io_pages to 0 and close readahead for
> -        * squashfs.
> -        */
> -       err = squashfs_bdi_init(sb);
> -       if (err) {
> -               errorf(fc, "squashfs init bdi failed");
> -               return err;
> -       }
> -
>         sb->s_fs_info = kzalloc(sizeof(*msblk), GFP_KERNEL);
>         if (sb->s_fs_info == NULL) {
>                 ERROR("Failed to allocate squashfs_sb_info\n");
> --
> 2.36.0.550.gb090851708-goog
>
>

[Squashfs-devel] [PATCH v2 2/3] squashfs: always build "file direct" version of page actor

[Hsin-Yi W. <hs...@ch...>]

From: Phillip Lougher <ph...@sq...>

Squashfs_readahead uses the "file direct" version of the page
actor, and so build it unconditionally.

Reported-by: kernel test robot <lk...@in...>
Signed-off-by: Phillip Lougher <ph...@sq...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
 fs/squashfs/Makefile     |  4 ++--
 fs/squashfs/page_actor.h | 41 ----------------------------------------
 2 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile
index 7bd9b8b856d0..477c89a519ee 100644
--- a/fs/squashfs/Makefile
+++ b/fs/squashfs/Makefile
@@ -5,9 +5,9 @@
 
 obj-$(CONFIG_SQUASHFS) += squashfs.o
 squashfs-y += block.o cache.o dir.o export.o file.o fragment.o id.o inode.o
-squashfs-y += namei.o super.o symlink.o decompressor.o
+squashfs-y += namei.o super.o symlink.o decompressor.o page_actor.o
 squashfs-$(CONFIG_SQUASHFS_FILE_CACHE) += file_cache.o
-squashfs-$(CONFIG_SQUASHFS_FILE_DIRECT) += file_direct.o page_actor.o
+squashfs-$(CONFIG_SQUASHFS_FILE_DIRECT) += file_direct.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_SINGLE) += decompressor_single.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI) += decompressor_multi.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU) += decompressor_multi_percpu.o
diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
index 2e3073ace009..26e07373af8a 100644
--- a/fs/squashfs/page_actor.h
+++ b/fs/squashfs/page_actor.h
@@ -6,46 +6,6 @@
  * Phillip Lougher <ph...@sq...>
  */
 
-#ifndef CONFIG_SQUASHFS_FILE_DIRECT
-struct squashfs_page_actor {
-	void	**page;
-	int	pages;
-	int	length;
-	int	next_page;
-};
-
-static inline struct squashfs_page_actor *squashfs_page_actor_init(void **page,
-	int pages, int length)
-{
-	struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL);
-
-	if (actor == NULL)
-		return NULL;
-
-	actor->length = length ? : pages * PAGE_SIZE;
-	actor->page = page;
-	actor->pages = pages;
-	actor->next_page = 0;
-	return actor;
-}
-
-static inline void *squashfs_first_page(struct squashfs_page_actor *actor)
-{
-	actor->next_page = 1;
-	return actor->page[0];
-}
-
-static inline void *squashfs_next_page(struct squashfs_page_actor *actor)
-{
-	return actor->next_page == actor->pages ? NULL :
-		actor->page[actor->next_page++];
-}
-
-static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
-{
-	/* empty */
-}
-#else
 struct squashfs_page_actor {
 	union {
 		void		**buffer;
@@ -76,4 +36,3 @@ static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
 	actor->squashfs_finish_page(actor);
 }
 #endif
-#endif
-- 
2.36.0.550.gb090851708-goog

[Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

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 or not enough in a
  datablock.
Otherwise pages will be marked as uptodate. The unhandled pages will be
updated by readpage later.

Suggested-by: Matthew Wilcox <wi...@in...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
Reported-by: Matthew Wilcox <wi...@in...>
Reported-by: Phillip Lougher <ph...@sq...>
Reported-by: Xiongwei Song <sx...@gm...>
---
v1->v2: remove unused check on readahead_expand().
v1: https://lore.kernel.org/lkml/202...@ch.../
---
 fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index a8e495d8eb86..e10a55c5b1eb 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -39,6 +39,7 @@
 #include "squashfs_fs_sb.h"
 #include "squashfs_fs_i.h"
 #include "squashfs.h"
+#include "page_actor.h"
 
 /*
  * Locate cache slot in range [offset, index] for specified inode.  If
@@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
 	return 0;
 }
 
+static void squashfs_readahead(struct readahead_control *ractl)
+{
+	struct inode *inode = ractl->mapping->host;
+	struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
+	size_t mask = (1UL << msblk->block_log) - 1;
+	size_t shift = msblk->block_log - PAGE_SHIFT;
+	loff_t start = readahead_pos(ractl) &~ mask;
+	size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
+	struct squashfs_page_actor *actor;
+	unsigned int nr_pages = 0;
+	struct page **pages;
+	u64 block = 0;
+	int bsize, res, i, index;
+	int file_end = i_size_read(inode) >> msblk->block_log;
+	unsigned int max_pages = 1UL << shift;
+
+	readahead_expand(ractl, start, (len | mask) + 1);
+
+	if (file_end == 0)
+		return;
+
+	pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
+	if (!pages)
+		return;
+
+	actor = squashfs_page_actor_init_special(pages, max_pages, 0);
+	if (!actor)
+		goto out;
+
+	for (;;) {
+		nr_pages = __readahead_batch(ractl, pages, max_pages);
+		if (!nr_pages)
+			break;
+
+		if (readahead_pos(ractl) >= i_size_read(inode) ||
+		    nr_pages < max_pages)
+			goto skip_pages;
+
+		index = pages[0]->index >> shift;
+		if ((pages[nr_pages - 1]->index >> shift) != index)
+			goto skip_pages;
+
+		bsize = read_blocklist(inode, index, &block);
+		if (bsize == 0)
+			goto skip_pages;
+
+		res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
+					 actor);
+
+		if (res >= 0)
+			for (i = 0; i < nr_pages; i++)
+				SetPageUptodate(pages[i]);
+
+		for (i = 0; i < nr_pages; i++) {
+			unlock_page(pages[i]);
+			put_page(pages[i]);
+		}
+	}
+
+	kfree(actor);
+	kfree(pages);
+	return;
+
+skip_pages:
+	for (i = 0; i < nr_pages; i++) {
+		unlock_page(pages[i]);
+		put_page(pages[i]);
+	}
+
+	kfree(actor);
+out:
+	kfree(pages);
+}
 
 const struct address_space_operations squashfs_aops = {
-	.read_folio = squashfs_read_folio
+	.read_folio = squashfs_read_folio,
+	.readahead = squashfs_readahead
 };
-- 
2.36.0.550.gb090851708-goog

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
>   datablock.
> Otherwise pages will be marked as uptodate. The unhandled pages will be
> updated by readpage later.
>
> Suggested-by: Matthew Wilcox <wi...@in...>
> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> Reported-by: Matthew Wilcox <wi...@in...>
> Reported-by: Phillip Lougher <ph...@sq...>
> Reported-by: Xiongwei Song <sx...@gm...>
> ---
> v1->v2: remove unused check on readahead_expand().
> v1: https://lore.kernel.org/lkml/202...@ch.../
> ---

Hi Phillip and Matthew,

Regarding the performance issue of this patch, I saw a possible
performance gain if we only read the first block instead of reading
until nr_pages == 0.

To be more clear, apply the following diff (Please ignore the skipping
of nr_pages check first. This is a demonstration of "only read and
update the first block per readahead call"):

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index aad6823f0615..c52f7c4a7cfe 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -524,10 +524,8 @@ static void squashfs_readahead(struct
readahead_control *ractl)
        if (!actor)
                goto out;

-       for (;;) {
+       {
                nr_pages = __readahead_batch(ractl, pages, max_pages);
-               if (!nr_pages)
-                       break;

                if (readahead_pos(ractl) >= i_size_read(inode) ||
                    nr_pages < max_pages)


All the performance numbers:
1. original: 39s
2. revert "mm: put readahead pages in cache earlier": 2.8s
3. v2 of this patch: 2.7s
4. v2 of this patch and apply the diff: 1.8s

In my testing data, normally it reads and updates 1~2 blocks per
readahead call. The change might not make sense since the performance
improvement may only happen in certain cases.

What do you think? Or is the performance of the current patch
considered reasonable?

Thanks.

testing env:
- arm64 on kernel 5.10
- data: ~ 300K pack file contains some android files

>  fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> index a8e495d8eb86..e10a55c5b1eb 100644
> --- a/fs/squashfs/file.c
> +++ b/fs/squashfs/file.c
> @@ -39,6 +39,7 @@
>  #include "squashfs_fs_sb.h"
>  #include "squashfs_fs_i.h"
>  #include "squashfs.h"
> +#include "page_actor.h"
>
>  /*
>   * Locate cache slot in range [offset, index] for specified inode.  If
> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>         return 0;
>  }
>
> +static void squashfs_readahead(struct readahead_control *ractl)
> +{
> +       struct inode *inode = ractl->mapping->host;
> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
> +       size_t mask = (1UL << msblk->block_log) - 1;
> +       size_t shift = msblk->block_log - PAGE_SHIFT;
> +       loff_t start = readahead_pos(ractl) &~ mask;
> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> +       struct squashfs_page_actor *actor;
> +       unsigned int nr_pages = 0;
> +       struct page **pages;
> +       u64 block = 0;
> +       int bsize, res, i, index;
> +       int file_end = i_size_read(inode) >> msblk->block_log;
> +       unsigned int max_pages = 1UL << shift;
> +
> +       readahead_expand(ractl, start, (len | mask) + 1);
> +
> +       if (file_end == 0)
> +               return;
> +
> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
> +       if (!pages)
> +               return;
> +
> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> +       if (!actor)
> +               goto out;
> +
> +       for (;;) {
> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
> +               if (!nr_pages)
> +                       break;
> +
> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
> +                   nr_pages < max_pages)
> +                       goto skip_pages;
> +
> +               index = pages[0]->index >> shift;
> +               if ((pages[nr_pages - 1]->index >> shift) != index)
> +                       goto skip_pages;
> +
> +               bsize = read_blocklist(inode, index, &block);
> +               if (bsize == 0)
> +                       goto skip_pages;
> +
> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
> +                                        actor);
> +
> +               if (res >= 0)
> +                       for (i = 0; i < nr_pages; i++)
> +                               SetPageUptodate(pages[i]);
> +
> +               for (i = 0; i < nr_pages; i++) {
> +                       unlock_page(pages[i]);
> +                       put_page(pages[i]);
> +               }
> +       }
> +
> +       kfree(actor);
> +       kfree(pages);
> +       return;
> +
> +skip_pages:
> +       for (i = 0; i < nr_pages; i++) {
> +               unlock_page(pages[i]);
> +               put_page(pages[i]);
> +       }
> +
> +       kfree(actor);
> +out:
> +       kfree(pages);
> +}
>
>  const struct address_space_operations squashfs_aops = {
> -       .read_folio = squashfs_read_folio
> +       .read_folio = squashfs_read_folio,
> +       .readahead = squashfs_readahead
>  };
> --
> 2.36.0.550.gb090851708-goog
>

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Phillip L. <ph...@sq...>]

On 19/05/2022 09:09, Hsin-Yi Wang wrote:
> On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
>>    datablock.
>> Otherwise pages will be marked as uptodate. The unhandled pages will be
>> updated by readpage later.
>>
>> Suggested-by: Matthew Wilcox <wi...@in...>
>> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
>> Reported-by: Matthew Wilcox <wi...@in...>
>> Reported-by: Phillip Lougher <ph...@sq...>
>> Reported-by: Xiongwei Song <sx...@gm...>
>> ---
>> v1->v2: remove unused check on readahead_expand().
>> v1: https://lore.kernel.org/lkml/202...@ch.../
>> ---
> 
> Hi Phillip and Matthew,
> 
> Regarding the performance issue of this patch, I saw a possible
> performance gain if we only read the first block instead of reading
> until nr_pages == 0.
> 
> To be more clear, apply the following diff (Please ignore the skipping
> of nr_pages check first. This is a demonstration of "only read and
> update the first block per readahead call"):
> 
> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> index aad6823f0615..c52f7c4a7cfe 100644
> --- a/fs/squashfs/file.c
> +++ b/fs/squashfs/file.c
> @@ -524,10 +524,8 @@ static void squashfs_readahead(struct
> readahead_control *ractl)
>          if (!actor)
>                  goto out;
> 
> -       for (;;) {
> +       {
>                  nr_pages = __readahead_batch(ractl, pages, max_pages);
> -               if (!nr_pages)
> -                       break;
> 
>                  if (readahead_pos(ractl) >= i_size_read(inode) ||
>                      nr_pages < max_pages)
> 
> 
> All the performance numbers:
> 1. original: 39s
> 2. revert "mm: put readahead pages in cache earlier": 2.8s
> 3. v2 of this patch: 2.7s
> 4. v2 of this patch and apply the diff: 1.8s
> 
> In my testing data, normally it reads and updates 1~2 blocks per
> readahead call. The change might not make sense since the performance
> improvement may only happen in certain cases.
> What do you think? Or is the performance of the current patch
> considered reasonable?

It entirely depends on where the speed improvement comes from.

 From experience, the speed improvement is probably worthwhile,
and probably isn't gained at the expense of worse performance
on other work-loads.

But this is a guestimate, based on the fact timings 2 and 3
(2.8s v 2.7s) are almost identical.  Which implies the v2
patch isn't now doing any more work than the previous
baseline before the "mm: put readahead pages in cache earlier"
patch (*).

As such the speed improvement must be coming from increased
parallelism.  Such as moving from serially reading the
readahead blocks to parallel reading.

But, without looking at any trace output, that is just a
guestimate.

Phillip

(*) multiply decompressing the same blocks, which
     is the cause of the performance regression.
> 
> Thanks.
> 
> testing env:
> - arm64 on kernel 5.10
> - data: ~ 300K pack file contains some android files
> 
>>   fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
>> index a8e495d8eb86..e10a55c5b1eb 100644
>> --- a/fs/squashfs/file.c
>> +++ b/fs/squashfs/file.c
>> @@ -39,6 +39,7 @@
>>   #include "squashfs_fs_sb.h"
>>   #include "squashfs_fs_i.h"
>>   #include "squashfs.h"
>> +#include "page_actor.h"
>>
>>   /*
>>    * Locate cache slot in range [offset, index] for specified inode.  If
>> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>>          return 0;
>>   }
>>
>> +static void squashfs_readahead(struct readahead_control *ractl)
>> +{
>> +       struct inode *inode = ractl->mapping->host;
>> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
>> +       size_t mask = (1UL << msblk->block_log) - 1;
>> +       size_t shift = msblk->block_log - PAGE_SHIFT;
>> +       loff_t start = readahead_pos(ractl) &~ mask;
>> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
>> +       struct squashfs_page_actor *actor;
>> +       unsigned int nr_pages = 0;
>> +       struct page **pages;
>> +       u64 block = 0;
>> +       int bsize, res, i, index;
>> +       int file_end = i_size_read(inode) >> msblk->block_log;
>> +       unsigned int max_pages = 1UL << shift;
>> +
>> +       readahead_expand(ractl, start, (len | mask) + 1);
>> +
>> +       if (file_end == 0)
>> +               return;
>> +
>> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
>> +       if (!pages)
>> +               return;
>> +
>> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
>> +       if (!actor)
>> +               goto out;
>> +
>> +       for (;;) {
>> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
>> +               if (!nr_pages)
>> +                       break;
>> +
>> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
>> +                   nr_pages < max_pages)
>> +                       goto skip_pages;
>> +
>> +               index = pages[0]->index >> shift;
>> +               if ((pages[nr_pages - 1]->index >> shift) != index)
>> +                       goto skip_pages;
>> +
>> +               bsize = read_blocklist(inode, index, &block);
>> +               if (bsize == 0)
>> +                       goto skip_pages;
>> +
>> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
>> +                                        actor);
>> +
>> +               if (res >= 0)
>> +                       for (i = 0; i < nr_pages; i++)
>> +                               SetPageUptodate(pages[i]);
>> +
>> +               for (i = 0; i < nr_pages; i++) {
>> +                       unlock_page(pages[i]);
>> +                       put_page(pages[i]);
>> +               }
>> +       }
>> +
>> +       kfree(actor);
>> +       kfree(pages);
>> +       return;
>> +
>> +skip_pages:
>> +       for (i = 0; i < nr_pages; i++) {
>> +               unlock_page(pages[i]);
>> +               put_page(pages[i]);
>> +       }
>> +
>> +       kfree(actor);
>> +out:
>> +       kfree(pages);
>> +}
>>
>>   const struct address_space_operations squashfs_aops = {
>> -       .read_folio = squashfs_read_folio
>> +       .read_folio = squashfs_read_folio,
>> +       .readahead = squashfs_readahead
>>   };
>> --
>> 2.36.0.550.gb090851708-goog
>>

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

On Fri, May 20, 2022 at 11:02 AM Phillip Lougher
<ph...@sq...> wrote:
>
> On 19/05/2022 09:09, Hsin-Yi Wang wrote:
> > On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
> >>    datablock.
> >> Otherwise pages will be marked as uptodate. The unhandled pages will be
> >> updated by readpage later.
> >>
> >> Suggested-by: Matthew Wilcox <wi...@in...>
> >> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> >> Reported-by: Matthew Wilcox <wi...@in...>
> >> Reported-by: Phillip Lougher <ph...@sq...>
> >> Reported-by: Xiongwei Song <sx...@gm...>
> >> ---
> >> v1->v2: remove unused check on readahead_expand().
> >> v1: https://lore.kernel.org/lkml/202...@ch.../
> >> ---
> >
> > Hi Phillip and Matthew,
> >
> > Regarding the performance issue of this patch, I saw a possible
> > performance gain if we only read the first block instead of reading
> > until nr_pages == 0.
> >
> > To be more clear, apply the following diff (Please ignore the skipping
> > of nr_pages check first. This is a demonstration of "only read and
> > update the first block per readahead call"):
> >
> > diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> > index aad6823f0615..c52f7c4a7cfe 100644
> > --- a/fs/squashfs/file.c
> > +++ b/fs/squashfs/file.c
> > @@ -524,10 +524,8 @@ static void squashfs_readahead(struct
> > readahead_control *ractl)
> >          if (!actor)
> >                  goto out;
> >
> > -       for (;;) {
> > +       {
> >                  nr_pages = __readahead_batch(ractl, pages, max_pages);
> > -               if (!nr_pages)
> > -                       break;
> >
> >                  if (readahead_pos(ractl) >= i_size_read(inode) ||
> >                      nr_pages < max_pages)
> >
> >
> > All the performance numbers:
> > 1. original: 39s
> > 2. revert "mm: put readahead pages in cache earlier": 2.8s
> > 3. v2 of this patch: 2.7s
> > 4. v2 of this patch and apply the diff: 1.8s
> >
> > In my testing data, normally it reads and updates 1~2 blocks per
> > readahead call. The change might not make sense since the performance
> > improvement may only happen in certain cases.
> > What do you think? Or is the performance of the current patch
> > considered reasonable?
>
> It entirely depends on where the speed improvement comes from.
>
>  From experience, the speed improvement is probably worthwhile,
> and probably isn't gained at the expense of worse performance
> on other work-loads.
>
> But this is a guestimate, based on the fact timings 2 and 3
> (2.8s v 2.7s) are almost identical.  Which implies the v2
> patch isn't now doing any more work than the previous
> baseline before the "mm: put readahead pages in cache earlier"
> patch (*).
>
> As such the speed improvement must be coming from increased
> parallelism.  Such as moving from serially reading the
> readahead blocks to parallel reading.
>
Thanks for the idea. I checked this by offlining other cores until
only one core exists. Removing loops still results in less time.

But after counting the #traces lines in squashfs_read_data():
If we remove the for loop (timings 4), the logs are less: 2.3K lines,
while v2 (timings 3) has 3.7K (other timings are also around 3.7K), so
removing loop doesn't look right.

I think v2 should be fine considering the slightly to none regression
compared to before.

Hi Matthew, what do you think? Do you have other comments? If not,
should I send a v3 to change Xiongwei Song's email address or can you
help modify it?

Thanks

> But, without looking at any trace output, that is just a
> guestimate.
>
> Phillip
>
> (*) multiply decompressing the same blocks, which
>      is the cause of the performance regression.
> >
> > Thanks.
> >
> > testing env:
> > - arm64 on kernel 5.10
> > - data: ~ 300K pack file contains some android files
> >
> >>   fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 76 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> >> index a8e495d8eb86..e10a55c5b1eb 100644
> >> --- a/fs/squashfs/file.c
> >> +++ b/fs/squashfs/file.c
> >> @@ -39,6 +39,7 @@
> >>   #include "squashfs_fs_sb.h"
> >>   #include "squashfs_fs_i.h"
> >>   #include "squashfs.h"
> >> +#include "page_actor.h"
> >>
> >>   /*
> >>    * Locate cache slot in range [offset, index] for specified inode.  If
> >> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
> >>          return 0;
> >>   }
> >>
> >> +static void squashfs_readahead(struct readahead_control *ractl)
> >> +{
> >> +       struct inode *inode = ractl->mapping->host;
> >> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
> >> +       size_t mask = (1UL << msblk->block_log) - 1;
> >> +       size_t shift = msblk->block_log - PAGE_SHIFT;
> >> +       loff_t start = readahead_pos(ractl) &~ mask;
> >> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> >> +       struct squashfs_page_actor *actor;
> >> +       unsigned int nr_pages = 0;
> >> +       struct page **pages;
> >> +       u64 block = 0;
> >> +       int bsize, res, i, index;
> >> +       int file_end = i_size_read(inode) >> msblk->block_log;
> >> +       unsigned int max_pages = 1UL << shift;
> >> +
> >> +       readahead_expand(ractl, start, (len | mask) + 1);
> >> +
> >> +       if (file_end == 0)
> >> +               return;
> >> +
> >> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
> >> +       if (!pages)
> >> +               return;
> >> +
> >> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> >> +       if (!actor)
> >> +               goto out;
> >> +
> >> +       for (;;) {
> >> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
> >> +               if (!nr_pages)
> >> +                       break;
> >> +
> >> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
> >> +                   nr_pages < max_pages)
> >> +                       goto skip_pages;
> >> +
> >> +               index = pages[0]->index >> shift;
> >> +               if ((pages[nr_pages - 1]->index >> shift) != index)
> >> +                       goto skip_pages;
> >> +
> >> +               bsize = read_blocklist(inode, index, &block);
> >> +               if (bsize == 0)
> >> +                       goto skip_pages;
> >> +
> >> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
> >> +                                        actor);
> >> +
> >> +               if (res >= 0)
> >> +                       for (i = 0; i < nr_pages; i++)
> >> +                               SetPageUptodate(pages[i]);
> >> +
> >> +               for (i = 0; i < nr_pages; i++) {
> >> +                       unlock_page(pages[i]);
> >> +                       put_page(pages[i]);
> >> +               }
> >> +       }
> >> +
> >> +       kfree(actor);
> >> +       kfree(pages);
> >> +       return;
> >> +
> >> +skip_pages:
> >> +       for (i = 0; i < nr_pages; i++) {
> >> +               unlock_page(pages[i]);
> >> +               put_page(pages[i]);
> >> +       }
> >> +
> >> +       kfree(actor);
> >> +out:
> >> +       kfree(pages);
> >> +}
> >>
> >>   const struct address_space_operations squashfs_aops = {
> >> -       .read_folio = squashfs_read_folio
> >> +       .read_folio = squashfs_read_folio,
> >> +       .readahead = squashfs_readahead
> >>   };
> >> --
> >> 2.36.0.550.gb090851708-goog
> >>
>

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Phillip L. <ph...@sq...>]

On 20/05/2022 08:38, Hsin-Yi Wang wrote:
> On Fri, May 20, 2022 at 11:02 AM Phillip Lougher
> <ph...@sq...> wrote:
>>
>> On 19/05/2022 09:09, Hsin-Yi Wang wrote:
>>> On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
>>>>     datablock.
>>>> Otherwise pages will be marked as uptodate. The unhandled pages will be
>>>> updated by readpage later.
>>>>
>>>> Suggested-by: Matthew Wilcox <wi...@in...>
>>>> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
>>>> Reported-by: Matthew Wilcox <wi...@in...>
>>>> Reported-by: Phillip Lougher <ph...@sq...>
>>>> Reported-by: Xiongwei Song <sx...@gm...>
>>>> ---
>>>> v1->v2: remove unused check on readahead_expand().
>>>> v1: https://lore.kernel.org/lkml/202...@ch.../
>>>> ---
>>>
>>> Hi Phillip and Matthew,
>>>
>>> Regarding the performance issue of this patch, I saw a possible
>>> performance gain if we only read the first block instead of reading
>>> until nr_pages == 0.
>>>
>>> To be more clear, apply the following diff (Please ignore the skipping
>>> of nr_pages check first. This is a demonstration of "only read and
>>> update the first block per readahead call"):
>>>
>>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
>>> index aad6823f0615..c52f7c4a7cfe 100644
>>> --- a/fs/squashfs/file.c
>>> +++ b/fs/squashfs/file.c
>>> @@ -524,10 +524,8 @@ static void squashfs_readahead(struct
>>> readahead_control *ractl)
>>>           if (!actor)
>>>                   goto out;
>>>
>>> -       for (;;) {
>>> +       {
>>>                   nr_pages = __readahead_batch(ractl, pages, max_pages);
>>> -               if (!nr_pages)
>>> -                       break;
>>>
>>>                   if (readahead_pos(ractl) >= i_size_read(inode) ||
>>>                       nr_pages < max_pages)
>>>
>>>
>>> All the performance numbers:
>>> 1. original: 39s
>>> 2. revert "mm: put readahead pages in cache earlier": 2.8s
>>> 3. v2 of this patch: 2.7s
>>> 4. v2 of this patch and apply the diff: 1.8s
>>>
>>> In my testing data, normally it reads and updates 1~2 blocks per
>>> readahead call. The change might not make sense since the performance
>>> improvement may only happen in certain cases.
>>> What do you think? Or is the performance of the current patch
>>> considered reasonable?
>>
>> It entirely depends on where the speed improvement comes from.
>>
>>   From experience, the speed improvement is probably worthwhile,
>> and probably isn't gained at the expense of worse performance
>> on other work-loads.
>>
>> But this is a guestimate, based on the fact timings 2 and 3
>> (2.8s v 2.7s) are almost identical.  Which implies the v2
>> patch isn't now doing any more work than the previous
>> baseline before the "mm: put readahead pages in cache earlier"
>> patch (*).
>>
>> As such the speed improvement must be coming from increased
>> parallelism.  Such as moving from serially reading the
>> readahead blocks to parallel reading.
>>
> Thanks for the idea. I checked this by offlining other cores until
> only one core exists. Removing loops still results in less time.
> 
> But after counting the #traces lines in squashfs_read_data():
> If we remove the for loop (timings 4), the logs are less: 2.3K lines,
> while v2 (timings 3) has 3.7K (other timings are also around 3.7K), so
> removing loop doesn't look right.

If a lot less data is being read than the other timings, then this does
look incorrect.

> 
> I think v2 should be fine considering the slightly to none regression
> compared to before.
> 

The fact the timings are almost identical implies all that needs
to be done to remove the performance regression has been done.

There are two things missing from the patch which need to
be handled.  These are not related to performance but error
handling and correctness.  So I have waited until now to
raise it.

If you look at the code for file_direct.c::squashfs_readpage_block()

https://elixir.bootlin.com/linux/latest/source/fs/squashfs/file_direct.c#L93

****
	res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor);
	if (res < 0)
		goto mark_errored;

	if (res != expected) {
		res = -EIO;
		goto mark_errored;
	}
****

You will see that it checks for two return conditions from 
squashfs_read_data().

If the decompressor returns error, or if the decompressed block
is different in size to that expected, then this is an error situation
(e.g. corrupted filesystem), and the read is marked as bad.

The current V2 patch doesn't check that the block decompressed
to the correct size (res != expected), and this could mean
filesystem corruption is not detected, which will be an
error handling regression.

Secondly, if you look at
https://elixir.bootlin.com/linux/latest/source/fs/squashfs/file_direct.c#L102

****
	/* Last page may have trailing bytes not filled */
	bytes = res % PAGE_SIZE;
	if (bytes) {
		pageaddr = kmap_atomic(page[pages - 1]);
		memset(pageaddr + bytes, 0, PAGE_SIZE - bytes);
		kunmap_atomic(pageaddr);
	}
****

The V2 patch is always reading max_pages, but at the
end of a file the last page may not be a full page.  This
is because the last block may not be complete (i.e. it is
only 126 Kbytes rather than the block_size of 128 Kbytes).

This will leave part of the last page unfilled by the decompressor,
and it should be zero filled, to avoid leaking data to user-space.

Phillip


> Hi Matthew, what do you think? Do you have other comments? If not,
> should I send a v3 to change Xiongwei Song's email address or can you
> help modify it?
> 
> Thanks
> 
>> But, without looking at any trace output, that is just a
>> guestimate.
>>
>> Phillip
>>
>> (*) multiply decompressing the same blocks, which
>>       is the cause of the performance regression.
>>>
>>> Thanks.
>>>
>>> testing env:
>>> - arm64 on kernel 5.10
>>> - data: ~ 300K pack file contains some android files
>>>
>>>>    fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 76 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
>>>> index a8e495d8eb86..e10a55c5b1eb 100644
>>>> --- a/fs/squashfs/file.c
>>>> +++ b/fs/squashfs/file.c
>>>> @@ -39,6 +39,7 @@
>>>>    #include "squashfs_fs_sb.h"
>>>>    #include "squashfs_fs_i.h"
>>>>    #include "squashfs.h"
>>>> +#include "page_actor.h"
>>>>
>>>>    /*
>>>>     * Locate cache slot in range [offset, index] for specified inode.  If
>>>> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>>>>           return 0;
>>>>    }
>>>>
>>>> +static void squashfs_readahead(struct readahead_control *ractl)
>>>> +{
>>>> +       struct inode *inode = ractl->mapping->host;
>>>> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
>>>> +       size_t mask = (1UL << msblk->block_log) - 1;
>>>> +       size_t shift = msblk->block_log - PAGE_SHIFT;
>>>> +       loff_t start = readahead_pos(ractl) &~ mask;
>>>> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
>>>> +       struct squashfs_page_actor *actor;
>>>> +       unsigned int nr_pages = 0;
>>>> +       struct page **pages;
>>>> +       u64 block = 0;
>>>> +       int bsize, res, i, index;
>>>> +       int file_end = i_size_read(inode) >> msblk->block_log;
>>>> +       unsigned int max_pages = 1UL << shift;
>>>> +
>>>> +       readahead_expand(ractl, start, (len | mask) + 1);
>>>> +
>>>> +       if (file_end == 0)
>>>> +               return;
>>>> +
>>>> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
>>>> +       if (!pages)
>>>> +               return;
>>>> +
>>>> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
>>>> +       if (!actor)
>>>> +               goto out;
>>>> +
>>>> +       for (;;) {
>>>> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
>>>> +               if (!nr_pages)
>>>> +                       break;
>>>> +
>>>> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
>>>> +                   nr_pages < max_pages)
>>>> +                       goto skip_pages;
>>>> +
>>>> +               index = pages[0]->index >> shift;
>>>> +               if ((pages[nr_pages - 1]->index >> shift) != index)
>>>> +                       goto skip_pages;
>>>> +
>>>> +               bsize = read_blocklist(inode, index, &block);
>>>> +               if (bsize == 0)
>>>> +                       goto skip_pages;
>>>> +
>>>> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
>>>> +                                        actor);
>>>> +
>>>> +               if (res >= 0)
>>>> +                       for (i = 0; i < nr_pages; i++)
>>>> +                               SetPageUptodate(pages[i]);
>>>> +
>>>> +               for (i = 0; i < nr_pages; i++) {
>>>> +                       unlock_page(pages[i]);
>>>> +                       put_page(pages[i]);
>>>> +               }
>>>> +       }
>>>> +
>>>> +       kfree(actor);
>>>> +       kfree(pages);
>>>> +       return;
>>>> +
>>>> +skip_pages:
>>>> +       for (i = 0; i < nr_pages; i++) {
>>>> +               unlock_page(pages[i]);
>>>> +               put_page(pages[i]);
>>>> +       }
>>>> +
>>>> +       kfree(actor);
>>>> +out:
>>>> +       kfree(pages);
>>>> +}
>>>>
>>>>    const struct address_space_operations squashfs_aops = {
>>>> -       .read_folio = squashfs_read_folio
>>>> +       .read_folio = squashfs_read_folio,
>>>> +       .readahead = squashfs_readahead
>>>>    };
>>>> --
>>>> 2.36.0.550.gb090851708-goog
>>>>
>>

Re: [Squashfs-devel] [PATCH v2 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

On Sat, May 21, 2022 at 4:22 AM Phillip Lougher <ph...@sq...> wrote:
>
> On 20/05/2022 08:38, Hsin-Yi Wang wrote:
> > On Fri, May 20, 2022 at 11:02 AM Phillip Lougher
> > <ph...@sq...> wrote:
> >>
> >> On 19/05/2022 09:09, Hsin-Yi Wang wrote:
> >>> On Tue, May 17, 2022 at 4:28 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
> >>>>     datablock.
> >>>> Otherwise pages will be marked as uptodate. The unhandled pages will be
> >>>> updated by readpage later.
> >>>>
> >>>> Suggested-by: Matthew Wilcox <wi...@in...>
> >>>> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> >>>> Reported-by: Matthew Wilcox <wi...@in...>
> >>>> Reported-by: Phillip Lougher <ph...@sq...>
> >>>> Reported-by: Xiongwei Song <sx...@gm...>
> >>>> ---
> >>>> v1->v2: remove unused check on readahead_expand().
> >>>> v1: https://lore.kernel.org/lkml/202...@ch.../
> >>>> ---
> >>>
> >>> Hi Phillip and Matthew,
> >>>
> >>> Regarding the performance issue of this patch, I saw a possible
> >>> performance gain if we only read the first block instead of reading
> >>> until nr_pages == 0.
> >>>
> >>> To be more clear, apply the following diff (Please ignore the skipping
> >>> of nr_pages check first. This is a demonstration of "only read and
> >>> update the first block per readahead call"):
> >>>
> >>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> >>> index aad6823f0615..c52f7c4a7cfe 100644
> >>> --- a/fs/squashfs/file.c
> >>> +++ b/fs/squashfs/file.c
> >>> @@ -524,10 +524,8 @@ static void squashfs_readahead(struct
> >>> readahead_control *ractl)
> >>>           if (!actor)
> >>>                   goto out;
> >>>
> >>> -       for (;;) {
> >>> +       {
> >>>                   nr_pages = __readahead_batch(ractl, pages, max_pages);
> >>> -               if (!nr_pages)
> >>> -                       break;
> >>>
> >>>                   if (readahead_pos(ractl) >= i_size_read(inode) ||
> >>>                       nr_pages < max_pages)
> >>>
> >>>
> >>> All the performance numbers:
> >>> 1. original: 39s
> >>> 2. revert "mm: put readahead pages in cache earlier": 2.8s
> >>> 3. v2 of this patch: 2.7s
> >>> 4. v2 of this patch and apply the diff: 1.8s
> >>>
> >>> In my testing data, normally it reads and updates 1~2 blocks per
> >>> readahead call. The change might not make sense since the performance
> >>> improvement may only happen in certain cases.
> >>> What do you think? Or is the performance of the current patch
> >>> considered reasonable?
> >>
> >> It entirely depends on where the speed improvement comes from.
> >>
> >>   From experience, the speed improvement is probably worthwhile,
> >> and probably isn't gained at the expense of worse performance
> >> on other work-loads.
> >>
> >> But this is a guestimate, based on the fact timings 2 and 3
> >> (2.8s v 2.7s) are almost identical.  Which implies the v2
> >> patch isn't now doing any more work than the previous
> >> baseline before the "mm: put readahead pages in cache earlier"
> >> patch (*).
> >>
> >> As such the speed improvement must be coming from increased
> >> parallelism.  Such as moving from serially reading the
> >> readahead blocks to parallel reading.
> >>
> > Thanks for the idea. I checked this by offlining other cores until
> > only one core exists. Removing loops still results in less time.
> >
> > But after counting the #traces lines in squashfs_read_data():
> > If we remove the for loop (timings 4), the logs are less: 2.3K lines,
> > while v2 (timings 3) has 3.7K (other timings are also around 3.7K), so
> > removing loop doesn't look right.
>
> If a lot less data is being read than the other timings, then this does
> look incorrect.
>
> >
> > I think v2 should be fine considering the slightly to none regression
> > compared to before.
> >
>
> The fact the timings are almost identical implies all that needs
> to be done to remove the performance regression has been done.
>
> There are two things missing from the patch which need to
> be handled.  These are not related to performance but error
> handling and correctness.  So I have waited until now to
> raise it.
>
> If you look at the code for file_direct.c::squashfs_readpage_block()
>
> https://elixir.bootlin.com/linux/latest/source/fs/squashfs/file_direct.c#L93
>
> ****
>         res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor);
>         if (res < 0)
>                 goto mark_errored;
>
>         if (res != expected) {
>                 res = -EIO;
>                 goto mark_errored;
>         }
> ****
>
> You will see that it checks for two return conditions from
> squashfs_read_data().
>
> If the decompressor returns error, or if the decompressed block
> is different in size to that expected, then this is an error situation
> (e.g. corrupted filesystem), and the read is marked as bad.
>
> The current V2 patch doesn't check that the block decompressed
> to the correct size (res != expected), and this could mean
> filesystem corruption is not detected, which will be an
> error handling regression.
>
Thanks for the review. In v3:

Added the check to see if the returned size is expected too. Since we
didn't mark error pages here (in readahead), if the size wasn't
expected, we just don't mark the page as Uptodate.

> Secondly, if you look at
> https://elixir.bootlin.com/linux/latest/source/fs/squashfs/file_direct.c#L102
>
> ****
>         /* Last page may have trailing bytes not filled */
>         bytes = res % PAGE_SIZE;
>         if (bytes) {
>                 pageaddr = kmap_atomic(page[pages - 1]);
>                 memset(pageaddr + bytes, 0, PAGE_SIZE - bytes);
>                 kunmap_atomic(pageaddr);
>         }
> ****
>
> The V2 patch is always reading max_pages, but at the
> end of a file the last page may not be a full page.  This
> is because the last block may not be complete (i.e. it is
> only 126 Kbytes rather than the block_size of 128 Kbytes).
>
> This will leave part of the last page unfilled by the decompressor,
> and it should be zero filled, to avoid leaking data to user-space.

If the size was expected, further check if it's not a full page. If
so, fill it with zeros at the end.

Performance is the same as v2.


>
> Phillip
>
>
> > Hi Matthew, what do you think? Do you have other comments? If not,
> > should I send a v3 to change Xiongwei Song's email address or can you
> > help modify it?
> >
> > Thanks
> >
> >> But, without looking at any trace output, that is just a
> >> guestimate.
> >>
> >> Phillip
> >>
> >> (*) multiply decompressing the same blocks, which
> >>       is the cause of the performance regression.
> >>>
> >>> Thanks.
> >>>
> >>> testing env:
> >>> - arm64 on kernel 5.10
> >>> - data: ~ 300K pack file contains some android files
> >>>
> >>>>    fs/squashfs/file.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 76 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> >>>> index a8e495d8eb86..e10a55c5b1eb 100644
> >>>> --- a/fs/squashfs/file.c
> >>>> +++ b/fs/squashfs/file.c
> >>>> @@ -39,6 +39,7 @@
> >>>>    #include "squashfs_fs_sb.h"
> >>>>    #include "squashfs_fs_i.h"
> >>>>    #include "squashfs.h"
> >>>> +#include "page_actor.h"
> >>>>
> >>>>    /*
> >>>>     * Locate cache slot in range [offset, index] for specified inode.  If
> >>>> @@ -495,7 +496,81 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
> >>>>           return 0;
> >>>>    }
> >>>>
> >>>> +static void squashfs_readahead(struct readahead_control *ractl)
> >>>> +{
> >>>> +       struct inode *inode = ractl->mapping->host;
> >>>> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
> >>>> +       size_t mask = (1UL << msblk->block_log) - 1;
> >>>> +       size_t shift = msblk->block_log - PAGE_SHIFT;
> >>>> +       loff_t start = readahead_pos(ractl) &~ mask;
> >>>> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> >>>> +       struct squashfs_page_actor *actor;
> >>>> +       unsigned int nr_pages = 0;
> >>>> +       struct page **pages;
> >>>> +       u64 block = 0;
> >>>> +       int bsize, res, i, index;
> >>>> +       int file_end = i_size_read(inode) >> msblk->block_log;
> >>>> +       unsigned int max_pages = 1UL << shift;
> >>>> +
> >>>> +       readahead_expand(ractl, start, (len | mask) + 1);
> >>>> +
> >>>> +       if (file_end == 0)
> >>>> +               return;
> >>>> +
> >>>> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
> >>>> +       if (!pages)
> >>>> +               return;
> >>>> +
> >>>> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> >>>> +       if (!actor)
> >>>> +               goto out;
> >>>> +
> >>>> +       for (;;) {
> >>>> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
> >>>> +               if (!nr_pages)
> >>>> +                       break;
> >>>> +
> >>>> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
> >>>> +                   nr_pages < max_pages)
> >>>> +                       goto skip_pages;
> >>>> +
> >>>> +               index = pages[0]->index >> shift;
> >>>> +               if ((pages[nr_pages - 1]->index >> shift) != index)
> >>>> +                       goto skip_pages;
> >>>> +
> >>>> +               bsize = read_blocklist(inode, index, &block);
> >>>> +               if (bsize == 0)
> >>>> +                       goto skip_pages;
> >>>> +
> >>>> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
> >>>> +                                        actor);
> >>>> +
> >>>> +               if (res >= 0)
> >>>> +                       for (i = 0; i < nr_pages; i++)
> >>>> +                               SetPageUptodate(pages[i]);
> >>>> +
> >>>> +               for (i = 0; i < nr_pages; i++) {
> >>>> +                       unlock_page(pages[i]);
> >>>> +                       put_page(pages[i]);
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       kfree(actor);
> >>>> +       kfree(pages);
> >>>> +       return;
> >>>> +
> >>>> +skip_pages:
> >>>> +       for (i = 0; i < nr_pages; i++) {
> >>>> +               unlock_page(pages[i]);
> >>>> +               put_page(pages[i]);
> >>>> +       }
> >>>> +
> >>>> +       kfree(actor);
> >>>> +out:
> >>>> +       kfree(pages);
> >>>> +}
> >>>>
> >>>>    const struct address_space_operations squashfs_aops = {
> >>>> -       .read_folio = squashfs_read_folio
> >>>> +       .read_folio = squashfs_read_folio,
> >>>> +       .readahead = squashfs_readahead
> >>>>    };
> >>>> --
> >>>> 2.36.0.550.gb090851708-goog
> >>>>
> >>
>

[Squashfs-devel] [PATCH v3 0/3] Implement readahead for squashfs

[Hsin-Yi W. <hs...@ch...>]

Commit c1f6925e1091("mm: put readahead pages in cache earlier") requires
fs to implement readahead callback. Otherwise there will be a
performance regression.

Commit 9eec1d897139("squashfs: provide backing_dev_info in order to
disable read-ahead") mitigates the performance drop issue for squashfs
by closing readahead for it.

This series implements readahead callback for squashfs. The previous
discussion are in [1] and [2].

[1] https://lore.kernel.org/all/CAJMQK-g9G6KQmH-V=BRG...@ma.../T/
[2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m4af4473b94f98a4996cb11756b633a07e5e059d1

Hsin-Yi Wang (2):
  Revert "squashfs: provide backing_dev_info in order to disable
    read-ahead"
  squashfs: implement readahead

Phillip Lougher (1):
  squashfs: always build "file direct" version of page actor

 fs/squashfs/Makefile     |  4 +-
 fs/squashfs/file.c       | 91 +++++++++++++++++++++++++++++++++++++++-
 fs/squashfs/page_actor.h | 41 ------------------
 fs/squashfs/super.c      | 33 ---------------
 4 files changed, 92 insertions(+), 77 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog

[Squashfs-devel] [PATCH v3 1/3] Revert "squashfs: provide backing_dev_info in order to disable read-ahead"

[Hsin-Yi W. <hs...@ch...>]

This reverts commit 9eec1d897139e5de287af5d559a02b811b844d82.

Revert closing the readahead to squashfs since the readahead callback
for squashfs is implemented.

Suggested-by: Xiongwei Song <Xio...@wi...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
 fs/squashfs/super.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 6d594ba2ed28..32565dafa7f3 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -29,7 +29,6 @@
 #include <linux/module.h>
 #include <linux/magic.h>
 #include <linux/xattr.h>
-#include <linux/backing-dev.h>
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -113,24 +112,6 @@ static const struct squashfs_decompressor *supported_squashfs_filesystem(
 	return decompressor;
 }
 
-static int squashfs_bdi_init(struct super_block *sb)
-{
-	int err;
-	unsigned int major = MAJOR(sb->s_dev);
-	unsigned int minor = MINOR(sb->s_dev);
-
-	bdi_put(sb->s_bdi);
-	sb->s_bdi = &noop_backing_dev_info;
-
-	err = super_setup_bdi_name(sb, "squashfs_%u_%u", major, minor);
-	if (err)
-		return err;
-
-	sb->s_bdi->ra_pages = 0;
-	sb->s_bdi->io_pages = 0;
-
-	return 0;
-}
 
 static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
@@ -146,20 +127,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	TRACE("Entered squashfs_fill_superblock\n");
 
-	/*
-	 * squashfs provides 'backing_dev_info' in order to disable read-ahead. For
-	 * squashfs, I/O is not deferred, it is done immediately in read_folio,
-	 * which means the user would always have to wait their own I/O. So the effect
-	 * of readahead is very weak for squashfs. squashfs_bdi_init will set
-	 * sb->s_bdi->ra_pages and sb->s_bdi->io_pages to 0 and close readahead for
-	 * squashfs.
-	 */
-	err = squashfs_bdi_init(sb);
-	if (err) {
-		errorf(fc, "squashfs init bdi failed");
-		return err;
-	}
-
 	sb->s_fs_info = kzalloc(sizeof(*msblk), GFP_KERNEL);
 	if (sb->s_fs_info == NULL) {
 		ERROR("Failed to allocate squashfs_sb_info\n");
-- 
2.36.1.124.g0e6072fb45-goog

[Squashfs-devel] [PATCH v3 2/3] squashfs: always build "file direct" version of page actor

[Hsin-Yi W. <hs...@ch...>]

From: Phillip Lougher <ph...@sq...>

Squashfs_readahead uses the "file direct" version of the page
actor, and so build it unconditionally.

Reported-by: kernel test robot <lk...@in...>
Signed-off-by: Phillip Lougher <ph...@sq...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
 fs/squashfs/Makefile     |  4 ++--
 fs/squashfs/page_actor.h | 41 ----------------------------------------
 2 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile
index 7bd9b8b856d0..477c89a519ee 100644
--- a/fs/squashfs/Makefile
+++ b/fs/squashfs/Makefile
@@ -5,9 +5,9 @@
 
 obj-$(CONFIG_SQUASHFS) += squashfs.o
 squashfs-y += block.o cache.o dir.o export.o file.o fragment.o id.o inode.o
-squashfs-y += namei.o super.o symlink.o decompressor.o
+squashfs-y += namei.o super.o symlink.o decompressor.o page_actor.o
 squashfs-$(CONFIG_SQUASHFS_FILE_CACHE) += file_cache.o
-squashfs-$(CONFIG_SQUASHFS_FILE_DIRECT) += file_direct.o page_actor.o
+squashfs-$(CONFIG_SQUASHFS_FILE_DIRECT) += file_direct.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_SINGLE) += decompressor_single.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI) += decompressor_multi.o
 squashfs-$(CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU) += decompressor_multi_percpu.o
diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
index 2e3073ace009..26e07373af8a 100644
--- a/fs/squashfs/page_actor.h
+++ b/fs/squashfs/page_actor.h
@@ -6,46 +6,6 @@
  * Phillip Lougher <ph...@sq...>
  */
 
-#ifndef CONFIG_SQUASHFS_FILE_DIRECT
-struct squashfs_page_actor {
-	void	**page;
-	int	pages;
-	int	length;
-	int	next_page;
-};
-
-static inline struct squashfs_page_actor *squashfs_page_actor_init(void **page,
-	int pages, int length)
-{
-	struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL);
-
-	if (actor == NULL)
-		return NULL;
-
-	actor->length = length ? : pages * PAGE_SIZE;
-	actor->page = page;
-	actor->pages = pages;
-	actor->next_page = 0;
-	return actor;
-}
-
-static inline void *squashfs_first_page(struct squashfs_page_actor *actor)
-{
-	actor->next_page = 1;
-	return actor->page[0];
-}
-
-static inline void *squashfs_next_page(struct squashfs_page_actor *actor)
-{
-	return actor->next_page == actor->pages ? NULL :
-		actor->page[actor->next_page++];
-}
-
-static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
-{
-	/* empty */
-}
-#else
 struct squashfs_page_actor {
 	union {
 		void		**buffer;
@@ -76,4 +36,3 @@ static inline void squashfs_finish_page(struct squashfs_page_actor *actor)
 	actor->squashfs_finish_page(actor);
 }
 #endif
-#endif
-- 
2.36.1.124.g0e6072fb45-goog

[Squashfs-devel] [PATCH v3 3/3] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

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 or not enough in a
  datablock.
- decompressor error.
Otherwise pages will be marked as uptodate. The unhandled pages will be
updated by readpage later.

Suggested-by: Matthew Wilcox <wi...@in...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
Reported-by: Matthew Wilcox <wi...@in...>
Reported-by: Phillip Lougher <ph...@sq...>
Reported-by: Xiongwei Song <Xio...@wi...>
---
v2->v3: Add checks on
- decompressed block size.
- fill zeros if the last page is not a full page.

v2: https://lore.kernel.org/lkml/202...@ch.../
v1: https://lore.kernel.org/lkml/202...@ch.../
---
 fs/squashfs/file.c | 91 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index a8e495d8eb86..c311fc685fe4 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -39,6 +39,7 @@
 #include "squashfs_fs_sb.h"
 #include "squashfs_fs_i.h"
 #include "squashfs.h"
+#include "page_actor.h"
 
 /*
  * Locate cache slot in range [offset, index] for specified inode.  If
@@ -495,7 +496,95 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
 	return 0;
 }
 
+static void squashfs_readahead(struct readahead_control *ractl)
+{
+	struct inode *inode = ractl->mapping->host;
+	struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
+	size_t mask = (1UL << msblk->block_log) - 1;
+	size_t shift = msblk->block_log - PAGE_SHIFT;
+	loff_t start = readahead_pos(ractl) &~ mask;
+	size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
+	struct squashfs_page_actor *actor;
+	unsigned int nr_pages = 0;
+	struct page **pages;
+	u64 block = 0;
+	int bsize, res, i, index, bytes, expected;
+	int file_end = i_size_read(inode) >> msblk->block_log;
+	unsigned int max_pages = 1UL << shift;
+	void *pageaddr;
+
+	readahead_expand(ractl, start, (len | mask) + 1);
+
+	if (file_end == 0)
+		return;
+
+	pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
+	if (!pages)
+		return;
+
+	actor = squashfs_page_actor_init_special(pages, max_pages, 0);
+	if (!actor)
+		goto out;
+
+	for (;;) {
+		nr_pages = __readahead_batch(ractl, pages, max_pages);
+		if (!nr_pages)
+			break;
+
+		if (readahead_pos(ractl) >= i_size_read(inode) ||
+		    nr_pages < max_pages)
+			goto skip_pages;
+
+		index = pages[0]->index >> shift;
+		if ((pages[nr_pages - 1]->index >> shift) != index)
+			goto skip_pages;
+
+		expected = index == file_end ?
+			   (i_size_read(inode) & (msblk->block_size - 1)) :
+			    msblk->block_size;
+
+		bsize = read_blocklist(inode, index, &block);
+		if (bsize == 0)
+			goto skip_pages;
+
+		res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
+					 actor);
+
+		if (res == expected) {
+			/* Last page may have trailing bytes not filled */
+			bytes = res % PAGE_SIZE;
+			if (bytes) {
+				pageaddr = kmap_atomic(pages[nr_pages - 1]);
+				memset(pageaddr + bytes, 0, PAGE_SIZE - bytes);
+				kunmap_atomic(pageaddr);
+			}
+
+			for (i = 0; i < nr_pages; i++)
+				SetPageUptodate(pages[i]);
+		}
+
+		for (i = 0; i < nr_pages; i++) {
+			unlock_page(pages[i]);
+			put_page(pages[i]);
+		}
+	}
+
+	kfree(actor);
+	kfree(pages);
+	return;
+
+skip_pages:
+	for (i = 0; i < nr_pages; i++) {
+		unlock_page(pages[i]);
+		put_page(pages[i]);
+	}
+
+	kfree(actor);
+out:
+	kfree(pages);
+}
 
 const struct address_space_operations squashfs_aops = {
-	.read_folio = squashfs_read_folio
+	.read_folio = squashfs_read_folio,
+	.readahead = squashfs_readahead
 };
-- 
2.36.1.124.g0e6072fb45-goog

[Squashfs-devel] [PATCH 0/2] Implement readahead for squashfs

[Hsin-Yi W. <hs...@ch...>]

Commit c1f6925e1091("mm: put readahead pages in cache earlier") requires
fs to implement readahead callback. Otherwise there will be a
performance regression.

Commit 9eec1d897139("squashfs: provide backing_dev_info in order to
disable read-ahead") mitigates the performance drop issue for squashfs
by closing readahead for it.

This series implements readahead callback for squashfs. The previous
discussion are in [1] and [2].

[1] https://lore.kernel.org/all/CAJMQK-g9G6KQmH-V=BRG...@ma.../T/
[2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m4af4473b94f98a4996cb11756b633a07e5e059d1

Hsin-Yi Wang (2):
  Revert "squashfs: provide backing_dev_info in order to disable
    read-ahead"
  squashfs: implement readahead

 fs/squashfs/file.c  | 77 +++++++++++++++++++++++++++++++++++++++++++++
 fs/squashfs/super.c | 33 -------------------
 2 files changed, 77 insertions(+), 33 deletions(-)

-- 
2.36.0.550.gb090851708-goog

[Squashfs-devel] [PATCH 1/2] Revert "squashfs: provide backing_dev_info in order to disable read-ahead"

[Hsin-Yi W. <hs...@ch...>]

This reverts commit 9eec1d897139e5de287af5d559a02b811b844d82.

Revert closing the readahead to squashfs since the readahead callback
for squashfs is implemented.

Suggested-by: Xiongwei Song <sx...@gm...>
Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
 fs/squashfs/super.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 6d594ba2ed28..32565dafa7f3 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -29,7 +29,6 @@
 #include <linux/module.h>
 #include <linux/magic.h>
 #include <linux/xattr.h>
-#include <linux/backing-dev.h>
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -113,24 +112,6 @@ static const struct squashfs_decompressor *supported_squashfs_filesystem(
 	return decompressor;
 }
 
-static int squashfs_bdi_init(struct super_block *sb)
-{
-	int err;
-	unsigned int major = MAJOR(sb->s_dev);
-	unsigned int minor = MINOR(sb->s_dev);
-
-	bdi_put(sb->s_bdi);
-	sb->s_bdi = &noop_backing_dev_info;
-
-	err = super_setup_bdi_name(sb, "squashfs_%u_%u", major, minor);
-	if (err)
-		return err;
-
-	sb->s_bdi->ra_pages = 0;
-	sb->s_bdi->io_pages = 0;
-
-	return 0;
-}
 
 static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
@@ -146,20 +127,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	TRACE("Entered squashfs_fill_superblock\n");
 
-	/*
-	 * squashfs provides 'backing_dev_info' in order to disable read-ahead. For
-	 * squashfs, I/O is not deferred, it is done immediately in read_folio,
-	 * which means the user would always have to wait their own I/O. So the effect
-	 * of readahead is very weak for squashfs. squashfs_bdi_init will set
-	 * sb->s_bdi->ra_pages and sb->s_bdi->io_pages to 0 and close readahead for
-	 * squashfs.
-	 */
-	err = squashfs_bdi_init(sb);
-	if (err) {
-		errorf(fc, "squashfs init bdi failed");
-		return err;
-	}
-
 	sb->s_fs_info = kzalloc(sizeof(*msblk), GFP_KERNEL);
 	if (sb->s_fs_info == NULL) {
 		ERROR("Failed to allocate squashfs_sb_info\n");
-- 
2.36.0.550.gb090851708-goog

[Squashfs-devel] [PATCH 2/2] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

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 or not enough in a
  datablock.
Otherwise pages will be marked as uptodate. The unhandled pages will be
updated by readpage later.

Signed-off-by: Hsin-Yi Wang <hs...@ch...>
---
Note that this patch was not formally sent to the list before. It's
attached to email thread for discussion as it's still under development.

- v1:
The patch outline was suggested by Matthew. It went through a few
reviews by Matthew offline.

- v2:
https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m442435c149d411c5c9d8019cff5915419b04bf10
This is a resend of v1.

- v3:
https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m55a709e6ba5ec59fe95323a67a7f3d6b1953e470
Fix page actor size to avoid a crash from squashfs_decompress().
Suggested by Phillip Lougher[1]
[1] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m687f82debb7667ff31982a05aef3eba081eb5039

- v4:
https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#mf93267690ec2e841dade6a494fe72c84b61328d9
Fix to free page after used. Suggested by Xiongwei Song[2]
Refactor the skip page logic to possible improve the performance.
Suggested by Phillip Lougher[3]
[2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m0e7b33d167b1ef0eb39b9f41c32ed3f80dfced18
[3] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m1e0a8f8e4a98d79d14c81b66e197b6dc0a3b77a1
---
 fs/squashfs/file.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index a8e495d8eb86..91dfec792f4c 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -39,6 +39,7 @@
 #include "squashfs_fs_sb.h"
 #include "squashfs_fs_i.h"
 #include "squashfs.h"
+#include "page_actor.h"
 
 /*
  * Locate cache slot in range [offset, index] for specified inode.  If
@@ -495,7 +496,83 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
 	return 0;
 }
 
+static void squashfs_readahead(struct readahead_control *ractl)
+{
+	struct inode *inode = ractl->mapping->host;
+	struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
+	size_t mask = (1UL << msblk->block_log) - 1;
+	size_t shift = msblk->block_log - PAGE_SHIFT;
+	loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
+	loff_t start = readahead_pos(ractl) &~ mask;
+	size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
+	struct squashfs_page_actor *actor;
+	unsigned int nr_pages = 0;
+	struct page **pages;
+	u64 block = 0;
+	int bsize, res, i, index;
+	int file_end = i_size_read(inode) >> msblk->block_log;
+	unsigned int max_pages = 1UL << shift;
+
+	readahead_expand(ractl, start, (len | mask) + 1);
+
+	if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
+	    file_end == 0)
+		return;
+
+	pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
+	if (!pages)
+		return;
+
+	actor = squashfs_page_actor_init_special(pages, max_pages, 0);
+	if (!actor)
+		goto out;
+
+	for (;;) {
+		nr_pages = __readahead_batch(ractl, pages, max_pages);
+		if (!nr_pages)
+			break;
+
+		if (readahead_pos(ractl) >= i_size_read(inode) ||
+		    nr_pages < max_pages)
+			goto skip_pages;
+
+		index = pages[0]->index >> shift;
+		if ((pages[nr_pages - 1]->index >> shift) != index)
+			goto skip_pages;
+
+		bsize = read_blocklist(inode, index, &block);
+		if (bsize == 0)
+			goto skip_pages;
+
+		res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
+					 actor);
+
+		if (res >= 0)
+			for (i = 0; i < nr_pages; i++)
+				SetPageUptodate(pages[i]);
+
+		for (i = 0; i < nr_pages; i++) {
+			unlock_page(pages[i]);
+			put_page(pages[i]);
+		}
+	}
+
+	kfree(actor);
+	kfree(pages);
+	return;
+
+skip_pages:
+	for (i = 0; i < nr_pages; i++) {
+		unlock_page(pages[i]);
+		put_page(pages[i]);
+	}
+
+	kfree(actor);
+out:
+	kfree(pages);
+}
 
 const struct address_space_operations squashfs_aops = {
 	.read_folio = squashfs_read_folio
+	.readahead = squashfs_readahead
 };
-- 
2.36.0.550.gb090851708-goog

Re: [Squashfs-devel] [PATCH 2/2] squashfs: implement readahead

[kernel t. r. <lk...@in...>]

Hi Hsin-Yi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220513]
[cannot apply to akpm-mm/mm-everything v5.18-rc7 v5.18-rc6 v5.18-rc5 v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
base:    1e1b28b936aed946122b4e0991e7144fdbbfd77e
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220516/202...@in.../config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
        git checkout 573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lk...@in...>

All errors (new ones prefixed by >>):

   fs/squashfs/file.c: In function 'squashfs_readahead':
>> fs/squashfs/file.c:526:17: error: implicit declaration of function 'squashfs_page_actor_init_special'; did you mean 'squashfs_page_actor_init'? [-Werror=implicit-function-declaration]
     526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                 squashfs_page_actor_init
   fs/squashfs/file.c:526:15: warning: assignment to 'struct squashfs_page_actor *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
         |               ^
   fs/squashfs/file.c: At top level:
>> fs/squashfs/file.c:577:9: error: request for member 'readahead' in something not a structure or union
     577 |         .readahead = squashfs_readahead
         |         ^
   cc1: some warnings being treated as errors


vim +526 fs/squashfs/file.c

   498	
   499	static void squashfs_readahead(struct readahead_control *ractl)
   500	{
   501		struct inode *inode = ractl->mapping->host;
   502		struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
   503		size_t mask = (1UL << msblk->block_log) - 1;
   504		size_t shift = msblk->block_log - PAGE_SHIFT;
   505		loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
   506		loff_t start = readahead_pos(ractl) &~ mask;
   507		size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
   508		struct squashfs_page_actor *actor;
   509		unsigned int nr_pages = 0;
   510		struct page **pages;
   511		u64 block = 0;
   512		int bsize, res, i, index;
   513		int file_end = i_size_read(inode) >> msblk->block_log;
   514		unsigned int max_pages = 1UL << shift;
   515	
   516		readahead_expand(ractl, start, (len | mask) + 1);
   517	
   518		if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
   519		    file_end == 0)
   520			return;
   521	
   522		pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
   523		if (!pages)
   524			return;
   525	
 > 526		actor = squashfs_page_actor_init_special(pages, max_pages, 0);
   527		if (!actor)
   528			goto out;
   529	
   530		for (;;) {
   531			nr_pages = __readahead_batch(ractl, pages, max_pages);
   532			if (!nr_pages)
   533				break;
   534	
   535			if (readahead_pos(ractl) >= i_size_read(inode) ||
   536			    nr_pages < max_pages)
   537				goto skip_pages;
   538	
   539			index = pages[0]->index >> shift;
   540			if ((pages[nr_pages - 1]->index >> shift) != index)
   541				goto skip_pages;
   542	
   543			bsize = read_blocklist(inode, index, &block);
   544			if (bsize == 0)
   545				goto skip_pages;
   546	
   547			res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
   548						 actor);
   549	
   550			if (res >= 0)
   551				for (i = 0; i < nr_pages; i++)
   552					SetPageUptodate(pages[i]);
   553	
   554			for (i = 0; i < nr_pages; i++) {
   555				unlock_page(pages[i]);
   556				put_page(pages[i]);
   557			}
   558		}
   559	
   560		kfree(actor);
   561		kfree(pages);
   562		return;
   563	
   564	skip_pages:
   565		for (i = 0; i < nr_pages; i++) {
   566			unlock_page(pages[i]);
   567			put_page(pages[i]);
   568		}
   569	
   570		kfree(actor);
   571	out:
   572		kfree(pages);
   573	}
   574	
   575	const struct address_space_operations squashfs_aops = {
   576		.read_folio = squashfs_read_folio
 > 577		.readahead = squashfs_readahead

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [Squashfs-devel] [PATCH 2/2] squashfs: implement readahead

[Phillip L. <ph...@sq...>]

On 16/05/2022 17:01, kernel test robot wrote:
> Hi Hsin-Yi,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on next-20220513]
> [cannot apply to akpm-mm/mm-everything v5.18-rc7 v5.18-rc6 v5.18-rc5 v5.18-rc7]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]

This is fixed by

[PATCH 3/2] squashfs: always build "file direct" version of page actor

Which I have just sent.

Phillip

> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
> base:    1e1b28b936aed946122b4e0991e7144fdbbfd77e
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220516/202...@in.../config)
> compiler: m68k-linux-gcc (GCC) 11.3.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/intel-lab-lkp/linux/commit/573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Hsin-Yi-Wang/Implement-readahead-for-squashfs/20220516-185438
>          git checkout 573e1f2ced0df097c30c595d5bf5a9e7a5fcb8d5
>          # save the config file
>          mkdir build_dir && cp config build_dir/.config
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lk...@in...>
> 
> All errors (new ones prefixed by >>):
> 
>     fs/squashfs/file.c: In function 'squashfs_readahead':
>>> fs/squashfs/file.c:526:17: error: implicit declaration of function 'squashfs_page_actor_init_special'; did you mean 'squashfs_page_actor_init'? [-Werror=implicit-function-declaration]
>       526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
>           |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                 squashfs_page_actor_init
>     fs/squashfs/file.c:526:15: warning: assignment to 'struct squashfs_page_actor *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>       526 |         actor = squashfs_page_actor_init_special(pages, max_pages, 0);
>           |               ^
>     fs/squashfs/file.c: At top level:
>>> fs/squashfs/file.c:577:9: error: request for member 'readahead' in something not a structure or union
>       577 |         .readahead = squashfs_readahead
>           |         ^
>     cc1: some warnings being treated as errors
> 
> 
> vim +526 fs/squashfs/file.c
> 
>     498	
>     499	static void squashfs_readahead(struct readahead_control *ractl)
>     500	{
>     501		struct inode *inode = ractl->mapping->host;
>     502		struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
>     503		size_t mask = (1UL << msblk->block_log) - 1;
>     504		size_t shift = msblk->block_log - PAGE_SHIFT;
>     505		loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
>     506		loff_t start = readahead_pos(ractl) &~ mask;
>     507		size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
>     508		struct squashfs_page_actor *actor;
>     509		unsigned int nr_pages = 0;
>     510		struct page **pages;
>     511		u64 block = 0;
>     512		int bsize, res, i, index;
>     513		int file_end = i_size_read(inode) >> msblk->block_log;
>     514		unsigned int max_pages = 1UL << shift;
>     515	
>     516		readahead_expand(ractl, start, (len | mask) + 1);
>     517	
>     518		if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
>     519		    file_end == 0)
>     520			return;
>     521	
>     522		pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
>     523		if (!pages)
>     524			return;
>     525	
>   > 526		actor = squashfs_page_actor_init_special(pages, max_pages, 0);
>     527		if (!actor)
>     528			goto out;
>     529	
>     530		for (;;) {
>     531			nr_pages = __readahead_batch(ractl, pages, max_pages);
>     532			if (!nr_pages)
>     533				break;
>     534	
>     535			if (readahead_pos(ractl) >= i_size_read(inode) ||
>     536			    nr_pages < max_pages)
>     537				goto skip_pages;
>     538	
>     539			index = pages[0]->index >> shift;
>     540			if ((pages[nr_pages - 1]->index >> shift) != index)
>     541				goto skip_pages;
>     542	
>     543			bsize = read_blocklist(inode, index, &block);
>     544			if (bsize == 0)
>     545				goto skip_pages;
>     546	
>     547			res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
>     548						 actor);
>     549	
>     550			if (res >= 0)
>     551				for (i = 0; i < nr_pages; i++)
>     552					SetPageUptodate(pages[i]);
>     553	
>     554			for (i = 0; i < nr_pages; i++) {
>     555				unlock_page(pages[i]);
>     556				put_page(pages[i]);
>     557			}
>     558		}
>     559	
>     560		kfree(actor);
>     561		kfree(pages);
>     562		return;
>     563	
>     564	skip_pages:
>     565		for (i = 0; i < nr_pages; i++) {
>     566			unlock_page(pages[i]);
>     567			put_page(pages[i]);
>     568		}
>     569	
>     570		kfree(actor);
>     571	out:
>     572		kfree(pages);
>     573	}
>     574	
>     575	const struct address_space_operations squashfs_aops = {
>     576		.read_folio = squashfs_read_folio
>   > 577		.readahead = squashfs_readahead
> 

Re: [Squashfs-devel] [PATCH 2/2] squashfs: implement readahead

[Hsin-Yi W. <hs...@ch...>]

On Mon, May 16, 2022 at 6:51 PM Hsin-Yi Wang <hs...@ch...> 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 or not enough in a
>   datablock.
> Otherwise pages will be marked as uptodate. The unhandled pages will be
> updated by readpage later.
>
Suggested-by: Matthew Wilcox <wi...@in...>
> Signed-off-by: Hsin-Yi Wang <hs...@ch...>
> ---
> Note that this patch was not formally sent to the list before. It's
> attached to email thread for discussion as it's still under development.
>
> - v1:
> The patch outline was suggested by Matthew. It went through a few
> reviews by Matthew offline.
>
> - v2:
> https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m442435c149d411c5c9d8019cff5915419b04bf10
> This is a resend of v1.
>
> - v3:
> https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m55a709e6ba5ec59fe95323a67a7f3d6b1953e470
> Fix page actor size to avoid a crash from squashfs_decompress().
> Suggested by Phillip Lougher[1]
> [1] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m687f82debb7667ff31982a05aef3eba081eb5039
>
> - v4:
> https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#mf93267690ec2e841dade6a494fe72c84b61328d9
> Fix to free page after used. Suggested by Xiongwei Song[2]
> Refactor the skip page logic to possible improve the performance.
> Suggested by Phillip Lougher[3]
> [2] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m0e7b33d167b1ef0eb39b9f41c32ed3f80dfced18
> [3] https://lore.kernel.org/linux-mm/Yn5...@ca.../t/#m1e0a8f8e4a98d79d14c81b66e197b6dc0a3b77a1
> ---
>  fs/squashfs/file.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
> index a8e495d8eb86..91dfec792f4c 100644
> --- a/fs/squashfs/file.c
> +++ b/fs/squashfs/file.c
> @@ -39,6 +39,7 @@
>  #include "squashfs_fs_sb.h"
>  #include "squashfs_fs_i.h"
>  #include "squashfs.h"
> +#include "page_actor.h"
>
>  /*
>   * Locate cache slot in range [offset, index] for specified inode.  If
> @@ -495,7 +496,83 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>         return 0;
>  }
>
> +static void squashfs_readahead(struct readahead_control *ractl)
> +{
> +       struct inode *inode = ractl->mapping->host;
> +       struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
> +       size_t mask = (1UL << msblk->block_log) - 1;
> +       size_t shift = msblk->block_log - PAGE_SHIFT;
> +       loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
> +       loff_t start = readahead_pos(ractl) &~ mask;
> +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> +       struct squashfs_page_actor *actor;
> +       unsigned int nr_pages = 0;
> +       struct page **pages;
> +       u64 block = 0;
> +       int bsize, res, i, index;
> +       int file_end = i_size_read(inode) >> msblk->block_log;
> +       unsigned int max_pages = 1UL << shift;
> +
> +       readahead_expand(ractl, start, (len | mask) + 1);
> +
> +       if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
> +           file_end == 0)
> +               return;
> +
> +       pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
> +       if (!pages)
> +               return;
> +
> +       actor = squashfs_page_actor_init_special(pages, max_pages, 0);
> +       if (!actor)
> +               goto out;
> +
> +       for (;;) {
> +               nr_pages = __readahead_batch(ractl, pages, max_pages);
> +               if (!nr_pages)
> +                       break;
> +
> +               if (readahead_pos(ractl) >= i_size_read(inode) ||
> +                   nr_pages < max_pages)
> +                       goto skip_pages;
> +
> +               index = pages[0]->index >> shift;
> +               if ((pages[nr_pages - 1]->index >> shift) != index)
> +                       goto skip_pages;
> +
> +               bsize = read_blocklist(inode, index, &block);
> +               if (bsize == 0)
> +                       goto skip_pages;
> +
> +               res = squashfs_read_data(inode->i_sb, block, bsize, NULL,
> +                                        actor);
> +
> +               if (res >= 0)
> +                       for (i = 0; i < nr_pages; i++)
> +                               SetPageUptodate(pages[i]);
> +
> +               for (i = 0; i < nr_pages; i++) {
> +                       unlock_page(pages[i]);
> +                       put_page(pages[i]);
> +               }
> +       }
> +
> +       kfree(actor);
> +       kfree(pages);
> +       return;
> +
> +skip_pages:
> +       for (i = 0; i < nr_pages; i++) {
> +               unlock_page(pages[i]);
> +               put_page(pages[i]);
> +       }
> +
> +       kfree(actor);
> +out:
> +       kfree(pages);
> +}
>
>  const struct address_space_operations squashfs_aops = {
>         .read_folio = squashfs_read_folio
> +       .readahead = squashfs_readahead
>  };
> --
> 2.36.0.550.gb090851708-goog
>