[PATCH 1/2] erofs: avoid memory allocation failure during rolling decompression

[PATCH 1/2] erofs: avoid memory allocation failure during rolling decompression

[Huang Jianan via Linux-erofs]

It should be better to ensure memory allocation during rolling
decompression to avoid io error.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/erofs/decompressor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 49347e681a53..fb0fa4e5b9ea 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -86,7 +86,7 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
 			victim = availables[--top];
 			get_page(victim);
 		} else {
-			victim = erofs_allocpage(pagepool, GFP_KERNEL);
+			victim = erofs_allocpage(pagepool, GFP_KERNEL | __GFP_NOFAIL);
 			if (!victim)
 				return -ENOMEM;
 			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
-- 
2.25.1

[PATCH 2/2] erofs: decompress in endio if possible

[Huang Jianan via Linux-erofs]

z_erofs_decompressqueue_endio may not be executed in the interrupt
context, for example, when dm-verity is turned on. In this scenario,
io should be decompressed directly to avoid additional scheduling
overhead. Also there is no need to wait for endio to execute
synchronous decompression.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/erofs/internal.h |   3 ++
 fs/erofs/super.c    |   1 +
 fs/erofs/zdata.c    | 102 ++++++++++++++++++++++++--------------------
 3 files changed, 60 insertions(+), 46 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 77965490dced..a19bcbb681fc 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -71,6 +71,9 @@ struct erofs_sb_info {
 	/* pseudo inode to manage cached pages */
 	struct inode *managed_cache;
 
+	/* decide whether to decompress synchronously */
+	bool sync_decompress;
+
 	/* # of pages needed for EROFS lz4 rolling decompression */
 	u16 lz4_max_distance_pages;
 #endif	/* CONFIG_EROFS_FS_ZIP */
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 37f1cc9d28cc..5b9a21d10a30 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -188,6 +188,7 @@ static int erofs_read_superblock(struct super_block *sb)
 		goto out;
 	}
 
+	sbi->sync_decompress = false;
 	/* parse on-disk compression configurations */
 	z_erofs_load_lz4_config(sbi, dsb);
 	ret = 0;
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 6cb356c4217b..727dd01f55c1 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -706,56 +706,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	goto out;
 }
 
-static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
-				       bool sync, int bios)
-{
-	/* wake up the caller thread for sync decompression */
-	if (sync) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&io->u.wait.lock, flags);
-		if (!atomic_add_return(bios, &io->pending_bios))
-			wake_up_locked(&io->u.wait);
-		spin_unlock_irqrestore(&io->u.wait.lock, flags);
-		return;
-	}
-
-	if (!atomic_add_return(bios, &io->pending_bios))
-		queue_work(z_erofs_workqueue, &io->u.work);
-}
-
 static bool z_erofs_page_is_invalidated(struct page *page)
 {
 	return !page->mapping && !z_erofs_is_shortlived_page(page);
 }
 
-static void z_erofs_decompressqueue_endio(struct bio *bio)
-{
-	tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
-	struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
-	blk_status_t err = bio->bi_status;
-	struct bio_vec *bvec;
-	struct bvec_iter_all iter_all;
-
-	bio_for_each_segment_all(bvec, bio, iter_all) {
-		struct page *page = bvec->bv_page;
-
-		DBG_BUGON(PageUptodate(page));
-		DBG_BUGON(z_erofs_page_is_invalidated(page));
-
-		if (err)
-			SetPageError(page);
-
-		if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
-			if (!err)
-				SetPageUptodate(page);
-			unlock_page(page);
-		}
-	}
-	z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
-	bio_put(bio);
-}
-
 static int z_erofs_decompress_pcluster(struct super_block *sb,
 				       struct z_erofs_pcluster *pcl,
 				       struct list_head *pagepool)
@@ -991,6 +946,60 @@ static void z_erofs_decompressqueue_work(struct work_struct *work)
 	kvfree(bgq);
 }
 
+static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
+				       bool sync, int bios)
+{
+	struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
+
+	/* wake up the caller thread for sync decompression */
+	if (sync) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&io->u.wait.lock, flags);
+		if (!atomic_add_return(bios, &io->pending_bios))
+			wake_up_locked(&io->u.wait);
+		spin_unlock_irqrestore(&io->u.wait.lock, flags);
+		return;
+	}
+
+	if (!atomic_add_return(bios, &io->pending_bios)) {
+		if (in_atomic() || irqs_disabled()) {
+			queue_work(z_erofs_workqueue, &io->u.work);
+			if (unlikely(!sbi->sync_decompress))
+				sbi->sync_decompress = true;
+		}
+		else
+			z_erofs_decompressqueue_work(&io->u.work);
+	}
+}
+
+static void z_erofs_decompressqueue_endio(struct bio *bio)
+{
+	tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
+	struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
+	blk_status_t err = bio->bi_status;
+	struct bio_vec *bvec;
+	struct bvec_iter_all iter_all;
+
+	bio_for_each_segment_all(bvec, bio, iter_all) {
+		struct page *page = bvec->bv_page;
+
+		DBG_BUGON(PageUptodate(page));
+		DBG_BUGON(z_erofs_page_is_invalidated(page));
+
+		if (err)
+			SetPageError(page);
+
+		if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
+			if (!err)
+				SetPageUptodate(page);
+			unlock_page(page);
+		}
+	}
+	z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
+	bio_put(bio);
+}
+
 static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 					       unsigned int nr,
 					       struct list_head *pagepool,
@@ -1333,7 +1342,8 @@ static void z_erofs_readahead(struct readahead_control *rac)
 	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
 
 	unsigned int nr_pages = readahead_count(rac);
-	bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages);
+	bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages) &
+			sbi->sync_decompress;
 	struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
 	struct page *page, *head = NULL;
 	LIST_HEAD(pagepool);
-- 
2.25.1

Re: [PATCH 2/2] erofs: decompress in endio if possible

[Gao Xiang]

On Fri, Mar 05, 2021 at 02:22:19PM +0800, Huang Jianan via Linux-erofs wrote:
> z_erofs_decompressqueue_endio may not be executed in the interrupt
> context, for example, when dm-verity is turned on. In this scenario,
> io should be decompressed directly to avoid additional scheduling
> overhead. Also there is no need to wait for endio to execute
> synchronous decompression.

z_erofs_decompressqueue_endio may not be executed in the atomic
context, for example, when dm-verity is turned on. In this scenario,
data can be decompressed directly to get rid of additional kworker
scheduling overhead. Also, it makes no sense to apply synchronous
decompression for such case.

> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>  fs/erofs/internal.h |   3 ++
>  fs/erofs/super.c    |   1 +
>  fs/erofs/zdata.c    | 102 ++++++++++++++++++++++++--------------------
>  3 files changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 77965490dced..a19bcbb681fc 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -71,6 +71,9 @@ struct erofs_sb_info {
>  	/* pseudo inode to manage cached pages */
>  	struct inode *managed_cache;
>  
> +	/* decide whether to decompress synchronously */
> +	bool sync_decompress;

bool readahead_sync_decompress;

> +
>  	/* # of pages needed for EROFS lz4 rolling decompression */
>  	u16 lz4_max_distance_pages;
>  #endif	/* CONFIG_EROFS_FS_ZIP */
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 37f1cc9d28cc..5b9a21d10a30 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -188,6 +188,7 @@ static int erofs_read_superblock(struct super_block *sb)
>  		goto out;
>  	}
>  
> +	sbi->sync_decompress = false;

Ah, could you rebase the patch on the top of 5.12-rc1
rather than dev-test? since I've fold your
"erofs: support adjust lz4 history window size"
into a new patchset, see:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=erofs/compr_cfgs

>  	/* parse on-disk compression configurations */
>  	z_erofs_load_lz4_config(sbi, dsb);
>  	ret = 0;
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 6cb356c4217b..727dd01f55c1 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -706,56 +706,11 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>  	goto out;
>  }
>  
> -static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
> -				       bool sync, int bios)
> -{
> -	/* wake up the caller thread for sync decompression */
> -	if (sync) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&io->u.wait.lock, flags);
> -		if (!atomic_add_return(bios, &io->pending_bios))
> -			wake_up_locked(&io->u.wait);
> -		spin_unlock_irqrestore(&io->u.wait.lock, flags);
> -		return;
> -	}
> -
> -	if (!atomic_add_return(bios, &io->pending_bios))
> -		queue_work(z_erofs_workqueue, &io->u.work);
> -}

Is it necessary to move the code snippet?

> -
>  static bool z_erofs_page_is_invalidated(struct page *page)
>  {
>  	return !page->mapping && !z_erofs_is_shortlived_page(page);
>  }
>  
> -static void z_erofs_decompressqueue_endio(struct bio *bio)
> -{
> -	tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
> -	struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
> -	blk_status_t err = bio->bi_status;
> -	struct bio_vec *bvec;
> -	struct bvec_iter_all iter_all;
> -
> -	bio_for_each_segment_all(bvec, bio, iter_all) {
> -		struct page *page = bvec->bv_page;
> -
> -		DBG_BUGON(PageUptodate(page));
> -		DBG_BUGON(z_erofs_page_is_invalidated(page));
> -
> -		if (err)
> -			SetPageError(page);
> -
> -		if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
> -			if (!err)
> -				SetPageUptodate(page);
> -			unlock_page(page);
> -		}
> -	}
> -	z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
> -	bio_put(bio);
> -}
> -
>  static int z_erofs_decompress_pcluster(struct super_block *sb,
>  				       struct z_erofs_pcluster *pcl,
>  				       struct list_head *pagepool)
> @@ -991,6 +946,60 @@ static void z_erofs_decompressqueue_work(struct work_struct *work)
>  	kvfree(bgq);
>  }
>  
> +static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
> +				       bool sync, int bios)
> +{
> +	struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
> +
> +	/* wake up the caller thread for sync decompression */
> +	if (sync) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&io->u.wait.lock, flags);
> +		if (!atomic_add_return(bios, &io->pending_bios))
> +			wake_up_locked(&io->u.wait);
> +		spin_unlock_irqrestore(&io->u.wait.lock, flags);
> +		return;
> +	}
> +
> +	if (!atomic_add_return(bios, &io->pending_bios)) {
> +		if (in_atomic() || irqs_disabled()) {
> +			queue_work(z_erofs_workqueue, &io->u.work);
> +			if (unlikely(!sbi->sync_decompress))
> +				sbi->sync_decompress = true;
> +		}
> +		else
> +			z_erofs_decompressqueue_work(&io->u.work);

Nit: coding style:

if () {
	...
} else {	this arm is needed.
	...
}

> +	}
> +}
> +
> +static void z_erofs_decompressqueue_endio(struct bio *bio)
> +{
> +	tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
> +	struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
> +	blk_status_t err = bio->bi_status;
> +	struct bio_vec *bvec;
> +	struct bvec_iter_all iter_all;
> +
> +	bio_for_each_segment_all(bvec, bio, iter_all) {
> +		struct page *page = bvec->bv_page;
> +
> +		DBG_BUGON(PageUptodate(page));
> +		DBG_BUGON(z_erofs_page_is_invalidated(page));
> +
> +		if (err)
> +			SetPageError(page);
> +
> +		if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
> +			if (!err)
> +				SetPageUptodate(page);
> +			unlock_page(page);
> +		}
> +	}
> +	z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
> +	bio_put(bio);
> +}
> +
>  static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
>  					       unsigned int nr,
>  					       struct list_head *pagepool,
> @@ -1333,7 +1342,8 @@ static void z_erofs_readahead(struct readahead_control *rac)
>  	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
>  
>  	unsigned int nr_pages = readahead_count(rac);
> -	bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages);
> +	bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages) &
> +			sbi->sync_decompress;

it would be better written as:

bool sync = (sbi->readahead_sync_decompress &&
	     nr_pages <= sbi->ctx.max_sync_decompress_pages);

Thanks,
Gao Xiang

>  	struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
>  	struct page *page, *head = NULL;
>  	LIST_HEAD(pagepool);
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] erofs: decompress in endio if possible

[kernel test robot]

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

Hi Huang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on xiang-erofs/dev-test]
[cannot apply to v5.12-rc1 next-20210305]
[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/0day-ci/linux/commits/Huang-Jianan/erofs-avoid-memory-allocation-failure-during-rolling-decompression/20210305-142329
base:   https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git dev-test
config: parisc-randconfig-r012-20210305 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.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/0day-ci/linux/commit/ecb1173ba28392f322cbb4c9cb1a66524b10dd78
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Huang-Jianan/erofs-avoid-memory-allocation-failure-during-rolling-decompression/20210305-142329
        git checkout ecb1173ba28392f322cbb4c9cb1a66524b10dd78
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

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

All errors (new ones prefixed by >>):

   fs/erofs/super.c: In function 'erofs_read_superblock':
>> fs/erofs/super.c:191:5: error: 'struct erofs_sb_info' has no member named 'sync_decompress'
     191 |  sbi->sync_decompress = false;
         |     ^~


vim +191 fs/erofs/super.c

   124	
   125	static int erofs_read_superblock(struct super_block *sb)
   126	{
   127		struct erofs_sb_info *sbi;
   128		struct page *page;
   129		struct erofs_super_block *dsb;
   130		unsigned int blkszbits;
   131		void *data;
   132		int ret;
   133	
   134		page = read_mapping_page(sb->s_bdev->bd_inode->i_mapping, 0, NULL);
   135		if (IS_ERR(page)) {
   136			erofs_err(sb, "cannot read erofs superblock");
   137			return PTR_ERR(page);
   138		}
   139	
   140		sbi = EROFS_SB(sb);
   141	
   142		data = kmap(page);
   143		dsb = (struct erofs_super_block *)(data + EROFS_SUPER_OFFSET);
   144	
   145		ret = -EINVAL;
   146		if (le32_to_cpu(dsb->magic) != EROFS_SUPER_MAGIC_V1) {
   147			erofs_err(sb, "cannot find valid erofs superblock");
   148			goto out;
   149		}
   150	
   151		sbi->feature_compat = le32_to_cpu(dsb->feature_compat);
   152		if (sbi->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
   153			ret = erofs_superblock_csum_verify(sb, data);
   154			if (ret)
   155				goto out;
   156		}
   157	
   158		blkszbits = dsb->blkszbits;
   159		/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
   160		if (blkszbits != LOG_BLOCK_SIZE) {
   161			erofs_err(sb, "blkszbits %u isn't supported on this platform",
   162				  blkszbits);
   163			goto out;
   164		}
   165	
   166		if (!check_layout_compatibility(sb, dsb))
   167			goto out;
   168	
   169		sbi->blocks = le32_to_cpu(dsb->blocks);
   170		sbi->meta_blkaddr = le32_to_cpu(dsb->meta_blkaddr);
   171	#ifdef CONFIG_EROFS_FS_XATTR
   172		sbi->xattr_blkaddr = le32_to_cpu(dsb->xattr_blkaddr);
   173	#endif
   174		sbi->islotbits = ilog2(sizeof(struct erofs_inode_compact));
   175		sbi->root_nid = le16_to_cpu(dsb->root_nid);
   176		sbi->inos = le64_to_cpu(dsb->inos);
   177	
   178		sbi->build_time = le64_to_cpu(dsb->build_time);
   179		sbi->build_time_nsec = le32_to_cpu(dsb->build_time_nsec);
   180	
   181		memcpy(&sb->s_uuid, dsb->uuid, sizeof(dsb->uuid));
   182	
   183		ret = strscpy(sbi->volume_name, dsb->volume_name,
   184			      sizeof(dsb->volume_name));
   185		if (ret < 0) {	/* -E2BIG */
   186			erofs_err(sb, "bad volume name without NIL terminator");
   187			ret = -EFSCORRUPTED;
   188			goto out;
   189		}
   190	
 > 191		sbi->sync_decompress = false;
   192		/* parse on-disk compression configurations */
   193		z_erofs_load_lz4_config(sbi, dsb);
   194		ret = 0;
   195	out:
   196		kunmap(page);
   197		put_page(page);
   198		return ret;
   199	}
   200	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20631 bytes --]

Re: [PATCH 1/2] erofs: avoid memory allocation failure during rolling decompression

[Gao Xiang]

On Fri, Mar 05, 2021 at 02:22:18PM +0800, Huang Jianan via Linux-erofs wrote:
> It should be better to ensure memory allocation during rolling
> decompression to avoid io error.

Currently, err would be treated as io error. Therefore, it'd be
better to ensure memory allocation during rolling decompression
to avoid such io error.

In the long term, we might consider adding another !Uptodate case
for such case.

> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>  fs/erofs/decompressor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 49347e681a53..fb0fa4e5b9ea 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -86,7 +86,7 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>  			victim = availables[--top];
>  			get_page(victim);
>  		} else {
> -			victim = erofs_allocpage(pagepool, GFP_KERNEL);
> +			victim = erofs_allocpage(pagepool, GFP_KERNEL | __GFP_NOFAIL);

80 char limitation.

Thanks,
Gao Xiang

>  			if (!victim)
>  				return -ENOMEM;
>  			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
> -- 
> 2.25.1
> 

[PATCH v5 1/2] erofs: avoid memory allocation failure during rolling decompression

[Huang Jianan via Linux-erofs]

Currently, err would be treated as io error. Therefore, it'd be
better to ensure memory allocation during rolling decompression
to avoid such io error.

In the long term, we might consider adding another !Uptodate case
for such case.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/erofs/decompressor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 1cb1ffd10569..3d276a8aad86 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -73,7 +73,8 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
 			victim = availables[--top];
 			get_page(victim);
 		} else {
-			victim = erofs_allocpage(pagepool, GFP_KERNEL);
+			victim = erofs_allocpage(pagepool,
+						 GFP_KERNEL | __GFP_NOFAIL);
 			if (!victim)
 				return -ENOMEM;
 			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
-- 
2.25.1

[PATCH v5 2/2] erofs: decompress in endio if possible

[Huang Jianan via Linux-erofs]

z_erofs_decompressqueue_endio may not be executed in the atomic
context, for example, when dm-verity is turned on. In this scenario,
data can be decompressed directly to get rid of additional kworker
scheduling overhead. Also, it makes no sense to apply synchronous
decompression for such case.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/erofs/internal.h |  2 ++
 fs/erofs/super.c    |  1 +
 fs/erofs/zdata.c    | 16 +++++++++++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 67a7ec945686..e325da7be237 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -53,6 +53,8 @@ struct erofs_fs_context {
 
 	/* threshold for decompression synchronously */
 	unsigned int max_sync_decompress_pages;
+	/* decide whether to decompress synchronously */
+	bool readahead_sync_decompress;
 #endif
 	unsigned int mount_opt;
 };
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index d5a6b9b888a5..0445d09b6331 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -200,6 +200,7 @@ static void erofs_default_options(struct erofs_fs_context *ctx)
 #ifdef CONFIG_EROFS_FS_ZIP
 	ctx->cache_strategy = EROFS_ZIP_CACHE_READAROUND;
 	ctx->max_sync_decompress_pages = 3;
+	ctx->readahead_sync_decompress = false;
 #endif
 #ifdef CONFIG_EROFS_FS_XATTR
 	set_opt(ctx, XATTR_USER);
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 6cb356c4217b..b22cea78a9fd 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -706,9 +706,12 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 	goto out;
 }
 
+static void z_erofs_decompressqueue_work(struct work_struct *work);
 static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
 				       bool sync, int bios)
 {
+	struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
+
 	/* wake up the caller thread for sync decompression */
 	if (sync) {
 		unsigned long flags;
@@ -720,8 +723,14 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
 		return;
 	}
 
-	if (!atomic_add_return(bios, &io->pending_bios))
-		queue_work(z_erofs_workqueue, &io->u.work);
+	if (!atomic_add_return(bios, &io->pending_bios)) {
+		if (in_atomic() || irqs_disabled()) {
+			queue_work(z_erofs_workqueue, &io->u.work);
+			sbi->ctx.readahead_sync_decompress = true;
+		} else {
+			z_erofs_decompressqueue_work(&io->u.work);
+		}
+	}
 }
 
 static bool z_erofs_page_is_invalidated(struct page *page)
@@ -1333,7 +1342,8 @@ static void z_erofs_readahead(struct readahead_control *rac)
 	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
 
 	unsigned int nr_pages = readahead_count(rac);
-	bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages);
+	bool sync = (sbi->ctx.readahead_sync_decompress &&
+			nr_pages <= sbi->ctx.max_sync_decompress_pages);
 	struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
 	struct page *page, *head = NULL;
 	LIST_HEAD(pagepool);
-- 
2.25.1

Re: [PATCH v5 2/2] erofs: decompress in endio if possible

[Gao Xiang]

On Fri, Mar 05, 2021 at 05:58:40PM +0800, Huang Jianan via Linux-erofs wrote:
> z_erofs_decompressqueue_endio may not be executed in the atomic
> context, for example, when dm-verity is turned on. In this scenario,
> data can be decompressed directly to get rid of additional kworker
> scheduling overhead. Also, it makes no sense to apply synchronous
> decompression for such case.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>  fs/erofs/internal.h |  2 ++
>  fs/erofs/super.c    |  1 +
>  fs/erofs/zdata.c    | 16 +++++++++++++---
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 67a7ec945686..e325da7be237 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -53,6 +53,8 @@ struct erofs_fs_context {
>  
>  	/* threshold for decompression synchronously */
>  	unsigned int max_sync_decompress_pages;
> +	/* decide whether to decompress synchronously */
> +	bool readahead_sync_decompress;

I updated this as below:

 	/* current strategy of how to use managed cache */
 	unsigned char cache_strategy;
+	/* strategy of sync decompression (false - auto, true - force on) */
+	bool readahead_sync_decompress;
 
 	/* threshold for decompression synchronously */
 	unsigned int max_sync_decompress_pages;

>  #endif
>  	unsigned int mount_opt;
>  };

...

> @@ -720,8 +723,14 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
>  		return;
>  	}
>  
> -	if (!atomic_add_return(bios, &io->pending_bios))
> -		queue_work(z_erofs_workqueue, &io->u.work);
> +	if (!atomic_add_return(bios, &io->pending_bios)) {
> +		if (in_atomic() || irqs_disabled()) {
> +			queue_work(z_erofs_workqueue, &io->u.work);
> +			sbi->ctx.readahead_sync_decompress = true;
> +		} else {
> +			z_erofs_decompressqueue_work(&io->u.work);
> +		}
> +	}

Also updated this as below to return as early as possible:

-	if (!atomic_add_return(bios, &io->pending_bios))
+	if (atomic_add_return(bios, &io->pending_bios))
+		return;
+	/* Use workqueue and sync decompression for atomic contexts only */
+	if (in_atomic() || irqs_disabled()) {
 		queue_work(z_erofs_workqueue, &io->u.work);
+		sbi->ctx.readahead_sync_decompress = true;
+		return;
+	}
+	z_erofs_decompressqueue_work(&io->u.work);
 }

Otherwise, it looks good to me. I've applied to dev-test
for preliminary testing.

Reviewed-by: Gao Xiang <hsiangkao@redhat.com>

Thanks,
Gao Xiang

Re: [PATCH v5 1/2] erofs: avoid memory allocation failure during rolling decompression

[Gao Xiang]

On Fri, Mar 05, 2021 at 05:58:39PM +0800, Huang Jianan via Linux-erofs wrote:
> Currently, err would be treated as io error. Therefore, it'd be
> better to ensure memory allocation during rolling decompression
> to avoid such io error.
> 
> In the long term, we might consider adding another !Uptodate case
> for such case.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>

Reviewed-by: Gao Xiang <hsiangkao@redhat.com>

Thanks,
Gao Xiang

Re: [PATCH v5 1/2] erofs: avoid memory allocation failure during rolling decompression

[Chao Yu]

On 2021/3/5 17:58, Huang Jianan via Linux-erofs wrote:
> Currently, err would be treated as io error. Therefore, it'd be
> better to ensure memory allocation during rolling decompression
> to avoid such io error.
> 
> In the long term, we might consider adding another !Uptodate case
> for such case.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>   fs/erofs/decompressor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 1cb1ffd10569..3d276a8aad86 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -73,7 +73,8 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>   			victim = availables[--top];
>   			get_page(victim);
>   		} else {
> -			victim = erofs_allocpage(pagepool, GFP_KERNEL);
> +			victim = erofs_allocpage(pagepool,
> +						 GFP_KERNEL | __GFP_NOFAIL);
>   			if (!victim)
>   				return -ENOMEM;

A little bit weird that we still need to check return value of erofs_allocpage()
after we pass __GFP_NOFAIL parameter.

Thanks,

>   			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
> 

Re: [PATCH v5 1/2] erofs: avoid memory allocation failure during rolling decompression

[Gao Xiang]

On Tue, Mar 16, 2021 at 09:11:02AM +0800, Chao Yu wrote:
> On 2021/3/5 17:58, Huang Jianan via Linux-erofs wrote:
> > Currently, err would be treated as io error. Therefore, it'd be
> > better to ensure memory allocation during rolling decompression
> > to avoid such io error.
> > 
> > In the long term, we might consider adding another !Uptodate case
> > for such case.
> > 
> > Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> > Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> > ---
> >   fs/erofs/decompressor.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> > index 1cb1ffd10569..3d276a8aad86 100644
> > --- a/fs/erofs/decompressor.c
> > +++ b/fs/erofs/decompressor.c
> > @@ -73,7 +73,8 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
> >   			victim = availables[--top];
> >   			get_page(victim);
> >   		} else {
> > -			victim = erofs_allocpage(pagepool, GFP_KERNEL);
> > +			victim = erofs_allocpage(pagepool,
> > +						 GFP_KERNEL | __GFP_NOFAIL);
> >   			if (!victim)
> >   				return -ENOMEM;
> 
> A little bit weird that we still need to check return value of erofs_allocpage()
> after we pass __GFP_NOFAIL parameter.

Yeah, good point! sorry I forgot that.

Jianan,
Could you take some time resending the next version with all new things
updated?... thus Chao could review easily, Thanks!

Thanks,
Gao Xiang

> 
> Thanks,
> 
> >   			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
> > 
>