[PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

Avoid the unnecessary tail recursion since it can be converted into
a loop directly in order to prevent potential stack overflow.

It's a pretty straightforward conversion.

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index b4059b9c3bac..572f0b8151ba 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
 				   unsigned int lookback_distance)
 {
 	struct erofs_inode *const vi = EROFS_I(m->inode);
-	struct erofs_map_blocks *const map = m->map;
 	const unsigned int lclusterbits = vi->z_logical_clusterbits;
-	unsigned long lcn = m->lcn;
-	int err;
 
-	if (lcn < lookback_distance) {
-		erofs_err(m->inode->i_sb,
-			  "bogus lookback distance @ nid %llu", vi->nid);
-		DBG_BUGON(1);
-		return -EFSCORRUPTED;
-	}
+	while (m->lcn >= lookback_distance) {
+		unsigned long lcn = m->lcn - lookback_distance;
+		int err;
 
-	/* load extent head logical cluster if needed */
-	lcn -= lookback_distance;
-	err = z_erofs_load_cluster_from_disk(m, lcn, false);
-	if (err)
-		return err;
+		/* load extent head logical cluster if needed */
+		err = z_erofs_load_cluster_from_disk(m, lcn, false);
+		if (err)
+			return err;
 
-	switch (m->type) {
-	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
-		if (!m->delta[0]) {
+		switch (m->type) {
+		case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
+			if (!m->delta[0]) {
+				erofs_err(m->inode->i_sb,
+					  "invalid lookback distance 0 @ nid %llu",
+					  vi->nid);
+				DBG_BUGON(1);
+				return -EFSCORRUPTED;
+			}
+			lookback_distance = m->delta[0];
+			continue;
+		case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
+		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
+		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
+			m->headtype = m->type;
+			m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
+			return 0;
+		default:
 			erofs_err(m->inode->i_sb,
-				  "invalid lookback distance 0 @ nid %llu",
-				  vi->nid);
+				  "unknown type %u @ lcn %lu of nid %llu",
+				  m->type, lcn, vi->nid);
 			DBG_BUGON(1);
-			return -EFSCORRUPTED;
+			return -EOPNOTSUPP;
 		}
-		return z_erofs_extent_lookback(m, m->delta[0]);
-	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
-	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
-	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
-		m->headtype = m->type;
-		map->m_la = (lcn << lclusterbits) | m->clusterofs;
-		break;
-	default:
-		erofs_err(m->inode->i_sb,
-			  "unknown type %u @ lcn %lu of nid %llu",
-			  m->type, lcn, vi->nid);
-		DBG_BUGON(1);
-		return -EOPNOTSUPP;
 	}
-	return 0;
+
+	erofs_err(m->inode->i_sb, "bogus lookback distance @ nid %llu",
+		  vi->nid);
+	DBG_BUGON(1);
+	return -EFSCORRUPTED;
 }
 
 static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
-- 
2.24.4

[PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

Avoid the unnecessary tail recursion since it can be converted into
a loop directly in order to prevent potential stack overflow.

It's a pretty straightforward conversion.

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index b4059b9c3bac..572f0b8151ba 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
 				   unsigned int lookback_distance)
 {
 	struct erofs_inode *const vi = EROFS_I(m->inode);
-	struct erofs_map_blocks *const map = m->map;
 	const unsigned int lclusterbits = vi->z_logical_clusterbits;
-	unsigned long lcn = m->lcn;
-	int err;
 
-	if (lcn < lookback_distance) {
-		erofs_err(m->inode->i_sb,
-			  "bogus lookback distance @ nid %llu", vi->nid);
-		DBG_BUGON(1);
-		return -EFSCORRUPTED;
-	}
+	while (m->lcn >= lookback_distance) {
+		unsigned long lcn = m->lcn - lookback_distance;
+		int err;
 
-	/* load extent head logical cluster if needed */
-	lcn -= lookback_distance;
-	err = z_erofs_load_cluster_from_disk(m, lcn, false);
-	if (err)
-		return err;
+		/* load extent head logical cluster if needed */
+		err = z_erofs_load_cluster_from_disk(m, lcn, false);
+		if (err)
+			return err;
 
-	switch (m->type) {
-	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
-		if (!m->delta[0]) {
+		switch (m->type) {
+		case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
+			if (!m->delta[0]) {
+				erofs_err(m->inode->i_sb,
+					  "invalid lookback distance 0 @ nid %llu",
+					  vi->nid);
+				DBG_BUGON(1);
+				return -EFSCORRUPTED;
+			}
+			lookback_distance = m->delta[0];
+			continue;
+		case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
+		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
+		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
+			m->headtype = m->type;
+			m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
+			return 0;
+		default:
 			erofs_err(m->inode->i_sb,
-				  "invalid lookback distance 0 @ nid %llu",
-				  vi->nid);
+				  "unknown type %u @ lcn %lu of nid %llu",
+				  m->type, lcn, vi->nid);
 			DBG_BUGON(1);
-			return -EFSCORRUPTED;
+			return -EOPNOTSUPP;
 		}
-		return z_erofs_extent_lookback(m, m->delta[0]);
-	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
-	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
-	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
-		m->headtype = m->type;
-		map->m_la = (lcn << lclusterbits) | m->clusterofs;
-		break;
-	default:
-		erofs_err(m->inode->i_sb,
-			  "unknown type %u @ lcn %lu of nid %llu",
-			  m->type, lcn, vi->nid);
-		DBG_BUGON(1);
-		return -EOPNOTSUPP;
 	}
-	return 0;
+
+	erofs_err(m->inode->i_sb, "bogus lookback distance @ nid %llu",
+		  vi->nid);
+	DBG_BUGON(1);
+	return -EFSCORRUPTED;
 }
 
 static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
-- 
2.24.4

[PATCH 2/2] erofs: refine managed inode stuffs

[Gao Xiang]

Set up the correct gfp mask and use it instead of hard coding.
Also add comments about .invalidatepage() to show more details.

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/super.c | 8 ++++++--
 fs/erofs/zdata.c | 7 +++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 12755217631f..e178100c162a 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -532,6 +532,11 @@ static int erofs_managed_cache_releasepage(struct page *page, gfp_t gfp_mask)
 	return ret;
 }
 
+/*
+ * It will be called only on inode eviction. In case that there are still some
+ * decompression requests in progress, wait with rescheduling for a bit here.
+ * We could introduce an extra locking instead but it seems unnecessary.
+ */
 static void erofs_managed_cache_invalidatepage(struct page *page,
 					       unsigned int offset,
 					       unsigned int length)
@@ -565,8 +570,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
 	inode->i_size = OFFSET_MAX;
 
 	inode->i_mapping->a_ops = &managed_cache_aops;
-	mapping_set_gfp_mask(inode->i_mapping,
-			     GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE);
+	mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	sbi->managed_cache = inode;
 	return 0;
 }
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 59aecf42e45c..f5e17c03b2fb 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1091,10 +1091,10 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
 static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 					       unsigned int nr,
 					       struct page **pagepool,
-					       struct address_space *mc,
-					       gfp_t gfp)
+					       struct address_space *mc)
 {
 	const pgoff_t index = pcl->obj.index;
+	gfp_t gfp = mapping_gfp_mask(mc);
 	bool tocache = false;
 
 	struct address_space *mapping;
@@ -1350,8 +1350,7 @@ static void z_erofs_submit_queue(struct super_block *sb,
 			struct page *page;
 
 			page = pickup_page_for_submission(pcl, i++, pagepool,
-							  MNGD_MAPPING(sbi),
-							  GFP_NOFS);
+							  MNGD_MAPPING(sbi));
 			if (!page)
 				continue;
 
-- 
2.24.4

[PATCH 2/2] erofs: refine managed inode stuffs

[Gao Xiang]

Set up the correct gfp mask and use it instead of hard coding.
Also add comments about .invalidatepage() to show more details.

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/super.c | 8 ++++++--
 fs/erofs/zdata.c | 7 +++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 12755217631f..e178100c162a 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -532,6 +532,11 @@ static int erofs_managed_cache_releasepage(struct page *page, gfp_t gfp_mask)
 	return ret;
 }
 
+/*
+ * It will be called only on inode eviction. In case that there are still some
+ * decompression requests in progress, wait with rescheduling for a bit here.
+ * We could introduce an extra locking instead but it seems unnecessary.
+ */
 static void erofs_managed_cache_invalidatepage(struct page *page,
 					       unsigned int offset,
 					       unsigned int length)
@@ -565,8 +570,7 @@ static int erofs_init_managed_cache(struct super_block *sb)
 	inode->i_size = OFFSET_MAX;
 
 	inode->i_mapping->a_ops = &managed_cache_aops;
-	mapping_set_gfp_mask(inode->i_mapping,
-			     GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE);
+	mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	sbi->managed_cache = inode;
 	return 0;
 }
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 59aecf42e45c..f5e17c03b2fb 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1091,10 +1091,10 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
 static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 					       unsigned int nr,
 					       struct page **pagepool,
-					       struct address_space *mc,
-					       gfp_t gfp)
+					       struct address_space *mc)
 {
 	const pgoff_t index = pcl->obj.index;
+	gfp_t gfp = mapping_gfp_mask(mc);
 	bool tocache = false;
 
 	struct address_space *mapping;
@@ -1350,8 +1350,7 @@ static void z_erofs_submit_queue(struct super_block *sb,
 			struct page *page;
 
 			page = pickup_page_for_submission(pcl, i++, pagepool,
-							  MNGD_MAPPING(sbi),
-							  GFP_NOFS);
+							  MNGD_MAPPING(sbi));
 			if (!page)
 				continue;
 
-- 
2.24.4

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Yue Hu]

On Fri, 11 Mar 2022 02:27:42 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> Avoid the unnecessary tail recursion since it can be converted into
> a loop directly in order to prevent potential stack overflow.
> 
> It's a pretty straightforward conversion.
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
>  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index b4059b9c3bac..572f0b8151ba 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>  				   unsigned int lookback_distance)
>  {
>  	struct erofs_inode *const vi = EROFS_I(m->inode);
> -	struct erofs_map_blocks *const map = m->map;
>  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> -	unsigned long lcn = m->lcn;
> -	int err;
>  
> -	if (lcn < lookback_distance) {
> -		erofs_err(m->inode->i_sb,
> -			  "bogus lookback distance @ nid %llu", vi->nid);
> -		DBG_BUGON(1);
> -		return -EFSCORRUPTED;
> -	}
> +	while (m->lcn >= lookback_distance) {
> +		unsigned long lcn = m->lcn - lookback_distance;
> +		int err;

may better to declare variable 'lclusterbits' in loop just like 'err' usage?

>  
> -	/* load extent head logical cluster if needed */
> -	lcn -= lookback_distance;
> -	err = z_erofs_load_cluster_from_disk(m, lcn, false);
> -	if (err)
> -		return err;
> +		/* load extent head logical cluster if needed */
> +		err = z_erofs_load_cluster_from_disk(m, lcn, false);
> +		if (err)
> +			return err;
>  
> -	switch (m->type) {
> -	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> -		if (!m->delta[0]) {
> +		switch (m->type) {
> +		case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> +			if (!m->delta[0]) {
> +				erofs_err(m->inode->i_sb,
> +					  "invalid lookback distance 0 @ nid %llu",
> +					  vi->nid);
> +				DBG_BUGON(1);
> +				return -EFSCORRUPTED;
> +			}
> +			lookback_distance = m->delta[0];
> +			continue;
> +		case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
> +		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
> +		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
> +			m->headtype = m->type;
> +			m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
> +			return 0;
> +		default:
>  			erofs_err(m->inode->i_sb,
> -				  "invalid lookback distance 0 @ nid %llu",
> -				  vi->nid);
> +				  "unknown type %u @ lcn %lu of nid %llu",
> +				  m->type, lcn, vi->nid);
>  			DBG_BUGON(1);
> -			return -EFSCORRUPTED;
> +			return -EOPNOTSUPP;
>  		}
> -		return z_erofs_extent_lookback(m, m->delta[0]);
> -	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
> -	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
> -	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
> -		m->headtype = m->type;
> -		map->m_la = (lcn << lclusterbits) | m->clusterofs;
> -		break;
> -	default:
> -		erofs_err(m->inode->i_sb,
> -			  "unknown type %u @ lcn %lu of nid %llu",
> -			  m->type, lcn, vi->nid);
> -		DBG_BUGON(1);
> -		return -EOPNOTSUPP;
>  	}
> -	return 0;
> +
> +	erofs_err(m->inode->i_sb, "bogus lookback distance @ nid %llu",
> +		  vi->nid);
> +	DBG_BUGON(1);
> +	return -EFSCORRUPTED;
>  }
>  
>  static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Yue Hu]

On Fri, 11 Mar 2022 02:27:42 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> Avoid the unnecessary tail recursion since it can be converted into
> a loop directly in order to prevent potential stack overflow.
> 
> It's a pretty straightforward conversion.
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
>  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index b4059b9c3bac..572f0b8151ba 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>  				   unsigned int lookback_distance)
>  {
>  	struct erofs_inode *const vi = EROFS_I(m->inode);
> -	struct erofs_map_blocks *const map = m->map;
>  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> -	unsigned long lcn = m->lcn;
> -	int err;
>  
> -	if (lcn < lookback_distance) {
> -		erofs_err(m->inode->i_sb,
> -			  "bogus lookback distance @ nid %llu", vi->nid);
> -		DBG_BUGON(1);
> -		return -EFSCORRUPTED;
> -	}
> +	while (m->lcn >= lookback_distance) {
> +		unsigned long lcn = m->lcn - lookback_distance;
> +		int err;

may better to declare variable 'lclusterbits' in loop just like 'err' usage?

>  
> -	/* load extent head logical cluster if needed */
> -	lcn -= lookback_distance;
> -	err = z_erofs_load_cluster_from_disk(m, lcn, false);
> -	if (err)
> -		return err;
> +		/* load extent head logical cluster if needed */
> +		err = z_erofs_load_cluster_from_disk(m, lcn, false);
> +		if (err)
> +			return err;
>  
> -	switch (m->type) {
> -	case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> -		if (!m->delta[0]) {
> +		switch (m->type) {
> +		case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
> +			if (!m->delta[0]) {
> +				erofs_err(m->inode->i_sb,
> +					  "invalid lookback distance 0 @ nid %llu",
> +					  vi->nid);
> +				DBG_BUGON(1);
> +				return -EFSCORRUPTED;
> +			}
> +			lookback_distance = m->delta[0];
> +			continue;
> +		case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
> +		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
> +		case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
> +			m->headtype = m->type;
> +			m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
> +			return 0;
> +		default:
>  			erofs_err(m->inode->i_sb,
> -				  "invalid lookback distance 0 @ nid %llu",
> -				  vi->nid);
> +				  "unknown type %u @ lcn %lu of nid %llu",
> +				  m->type, lcn, vi->nid);
>  			DBG_BUGON(1);
> -			return -EFSCORRUPTED;
> +			return -EOPNOTSUPP;
>  		}
> -		return z_erofs_extent_lookback(m, m->delta[0]);
> -	case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
> -	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD1:
> -	case Z_EROFS_VLE_CLUSTER_TYPE_HEAD2:
> -		m->headtype = m->type;
> -		map->m_la = (lcn << lclusterbits) | m->clusterofs;
> -		break;
> -	default:
> -		erofs_err(m->inode->i_sb,
> -			  "unknown type %u @ lcn %lu of nid %llu",
> -			  m->type, lcn, vi->nid);
> -		DBG_BUGON(1);
> -		return -EOPNOTSUPP;
>  	}
> -	return 0;
> +
> +	erofs_err(m->inode->i_sb, "bogus lookback distance @ nid %llu",
> +		  vi->nid);
> +	DBG_BUGON(1);
> +	return -EFSCORRUPTED;
>  }
>  
>  static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:
> On Fri, 11 Mar 2022 02:27:42 +0800
> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> 
> > Avoid the unnecessary tail recursion since it can be converted into
> > a loop directly in order to prevent potential stack overflow.
> > 
> > It's a pretty straightforward conversion.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > ---
> >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> >  1 file changed, 33 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > index b4059b9c3bac..572f0b8151ba 100644
> > --- a/fs/erofs/zmap.c
> > +++ b/fs/erofs/zmap.c
> > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> >  				   unsigned int lookback_distance)
> >  {
> >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > -	struct erofs_map_blocks *const map = m->map;
> >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > -	unsigned long lcn = m->lcn;
> > -	int err;
> >  
> > -	if (lcn < lookback_distance) {
> > -		erofs_err(m->inode->i_sb,
> > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > -		DBG_BUGON(1);
> > -		return -EFSCORRUPTED;
> > -	}
> > +	while (m->lcn >= lookback_distance) {
> > +		unsigned long lcn = m->lcn - lookback_distance;
> > +		int err;
> 
> may better to declare variable 'lclusterbits' in loop just like 'err' usage?

I'm fine with either way. Ok, will post the next version later.

Thanks,
Gao Xiang

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:
> On Fri, 11 Mar 2022 02:27:42 +0800
> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> 
> > Avoid the unnecessary tail recursion since it can be converted into
> > a loop directly in order to prevent potential stack overflow.
> > 
> > It's a pretty straightforward conversion.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > ---
> >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> >  1 file changed, 33 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > index b4059b9c3bac..572f0b8151ba 100644
> > --- a/fs/erofs/zmap.c
> > +++ b/fs/erofs/zmap.c
> > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> >  				   unsigned int lookback_distance)
> >  {
> >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > -	struct erofs_map_blocks *const map = m->map;
> >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > -	unsigned long lcn = m->lcn;
> > -	int err;
> >  
> > -	if (lcn < lookback_distance) {
> > -		erofs_err(m->inode->i_sb,
> > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > -		DBG_BUGON(1);
> > -		return -EFSCORRUPTED;
> > -	}
> > +	while (m->lcn >= lookback_distance) {
> > +		unsigned long lcn = m->lcn - lookback_distance;
> > +		int err;
> 
> may better to declare variable 'lclusterbits' in loop just like 'err' usage?

I'm fine with either way. Ok, will post the next version later.

Thanks,
Gao Xiang

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

On Fri, Mar 11, 2022 at 03:28:28PM +0800, Gao Xiang wrote:
> On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:
> > On Fri, 11 Mar 2022 02:27:42 +0800
> > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > 
> > > Avoid the unnecessary tail recursion since it can be converted into
> > > a loop directly in order to prevent potential stack overflow.
> > > 
> > > It's a pretty straightforward conversion.
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > > ---
> > >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> > >  1 file changed, 33 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > > index b4059b9c3bac..572f0b8151ba 100644
> > > --- a/fs/erofs/zmap.c
> > > +++ b/fs/erofs/zmap.c
> > > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> > >  				   unsigned int lookback_distance)
> > >  {
> > >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > > -	struct erofs_map_blocks *const map = m->map;
> > >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > > -	unsigned long lcn = m->lcn;
> > > -	int err;
> > >  
> > > -	if (lcn < lookback_distance) {
> > > -		erofs_err(m->inode->i_sb,
> > > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > > -		DBG_BUGON(1);
> > > -		return -EFSCORRUPTED;
> > > -	}
> > > +	while (m->lcn >= lookback_distance) {
> > > +		unsigned long lcn = m->lcn - lookback_distance;
> > > +		int err;
> > 
> > may better to declare variable 'lclusterbits' in loop just like 'err' usage?
> 
> I'm fine with either way. Ok, will post the next version later.

Oh, I just noticed that you mean `lclusterbits', I think it won't
change in this function, so I don't tend to move it into the inner
loop.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Gao Xiang]

On Fri, Mar 11, 2022 at 03:28:28PM +0800, Gao Xiang wrote:
> On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:
> > On Fri, 11 Mar 2022 02:27:42 +0800
> > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > 
> > > Avoid the unnecessary tail recursion since it can be converted into
> > > a loop directly in order to prevent potential stack overflow.
> > > 
> > > It's a pretty straightforward conversion.
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > > ---
> > >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> > >  1 file changed, 33 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > > index b4059b9c3bac..572f0b8151ba 100644
> > > --- a/fs/erofs/zmap.c
> > > +++ b/fs/erofs/zmap.c
> > > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> > >  				   unsigned int lookback_distance)
> > >  {
> > >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > > -	struct erofs_map_blocks *const map = m->map;
> > >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > > -	unsigned long lcn = m->lcn;
> > > -	int err;
> > >  
> > > -	if (lcn < lookback_distance) {
> > > -		erofs_err(m->inode->i_sb,
> > > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > > -		DBG_BUGON(1);
> > > -		return -EFSCORRUPTED;
> > > -	}
> > > +	while (m->lcn >= lookback_distance) {
> > > +		unsigned long lcn = m->lcn - lookback_distance;
> > > +		int err;
> > 
> > may better to declare variable 'lclusterbits' in loop just like 'err' usage?
> 
> I'm fine with either way. Ok, will post the next version later.

Oh, I just noticed that you mean `lclusterbits', I think it won't
change in this function, so I don't tend to move it into the inner
loop.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Yue Hu]

On Fri, 11 Mar 2022 15:39:00 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> On Fri, Mar 11, 2022 at 03:28:28PM +0800, Gao Xiang wrote:
> > On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:  
> > > On Fri, 11 Mar 2022 02:27:42 +0800
> > > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > >   
> > > > Avoid the unnecessary tail recursion since it can be converted into
> > > > a loop directly in order to prevent potential stack overflow.
> > > > 
> > > > It's a pretty straightforward conversion.
> > > > 
> > > > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > > > ---
> > > >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 33 insertions(+), 34 deletions(-)
> > > > 
> > > > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > > > index b4059b9c3bac..572f0b8151ba 100644
> > > > --- a/fs/erofs/zmap.c
> > > > +++ b/fs/erofs/zmap.c
> > > > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> > > >  				   unsigned int lookback_distance)
> > > >  {
> > > >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > > > -	struct erofs_map_blocks *const map = m->map;
> > > >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > > > -	unsigned long lcn = m->lcn;
> > > > -	int err;
> > > >  
> > > > -	if (lcn < lookback_distance) {
> > > > -		erofs_err(m->inode->i_sb,
> > > > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > > > -		DBG_BUGON(1);
> > > > -		return -EFSCORRUPTED;
> > > > -	}
> > > > +	while (m->lcn >= lookback_distance) {
> > > > +		unsigned long lcn = m->lcn - lookback_distance;
> > > > +		int err;  
> > > 
> > > may better to declare variable 'lclusterbits' in loop just like 'err' usage?  
> > 
> > I'm fine with either way. Ok, will post the next version later.  
> 
> Oh, I just noticed that you mean `lclusterbits', I think it won't
> change in this function, so I don't tend to move it into the inner
> loop.

Ok, looks good to me.

Reviewed-by: Yue Hu <huyue2@coolpad.com>

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thanks,
> > Gao Xiang  

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Yue Hu]

On Fri, 11 Mar 2022 15:39:00 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> On Fri, Mar 11, 2022 at 03:28:28PM +0800, Gao Xiang wrote:
> > On Fri, Mar 11, 2022 at 03:12:32PM +0800, Yue Hu wrote:  
> > > On Fri, 11 Mar 2022 02:27:42 +0800
> > > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > >   
> > > > Avoid the unnecessary tail recursion since it can be converted into
> > > > a loop directly in order to prevent potential stack overflow.
> > > > 
> > > > It's a pretty straightforward conversion.
> > > > 
> > > > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > > > ---
> > > >  fs/erofs/zmap.c | 67 ++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 33 insertions(+), 34 deletions(-)
> > > > 
> > > > diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> > > > index b4059b9c3bac..572f0b8151ba 100644
> > > > --- a/fs/erofs/zmap.c
> > > > +++ b/fs/erofs/zmap.c
> > > > @@ -431,48 +431,47 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> > > >  				   unsigned int lookback_distance)
> > > >  {
> > > >  	struct erofs_inode *const vi = EROFS_I(m->inode);
> > > > -	struct erofs_map_blocks *const map = m->map;
> > > >  	const unsigned int lclusterbits = vi->z_logical_clusterbits;
> > > > -	unsigned long lcn = m->lcn;
> > > > -	int err;
> > > >  
> > > > -	if (lcn < lookback_distance) {
> > > > -		erofs_err(m->inode->i_sb,
> > > > -			  "bogus lookback distance @ nid %llu", vi->nid);
> > > > -		DBG_BUGON(1);
> > > > -		return -EFSCORRUPTED;
> > > > -	}
> > > > +	while (m->lcn >= lookback_distance) {
> > > > +		unsigned long lcn = m->lcn - lookback_distance;
> > > > +		int err;  
> > > 
> > > may better to declare variable 'lclusterbits' in loop just like 'err' usage?  
> > 
> > I'm fine with either way. Ok, will post the next version later.  
> 
> Oh, I just noticed that you mean `lclusterbits', I think it won't
> change in this function, so I don't tend to move it into the inner
> loop.

Ok, looks good to me.

Reviewed-by: Yue Hu <huyue2@coolpad.com>

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thanks,
> > Gao Xiang  

Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback

[Chao Yu]

On 2022/3/11 2:27, Gao Xiang wrote:
> Avoid the unnecessary tail recursion since it can be converted into
> a loop directly in order to prevent potential stack overflow.
> 
> It's a pretty straightforward conversion.
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [PATCH 2/2] erofs: refine managed inode stuffs

[Chao Yu]

On 2022/3/11 2:27, Gao Xiang wrote:
> Set up the correct gfp mask and use it instead of hard coding.
> Also add comments about .invalidatepage() to show more details.
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,