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 <[email protected]>
---
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
On Fri, 11 Mar 2022 15:39:00 +0800
Gao Xiang <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>
> Thanks,
> Gao Xiang
>
> >
> > Thanks,
> > 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
---
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
On Fri, 11 Mar 2022 02:27:42 +0800
Gao Xiang <[email protected]> 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 <[email protected]>
> ---
> 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,
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 <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,
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 <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,