Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 11 Mar 2022 15:12:32 +0800
From:   Yue Hu <zbestahu@gmail.com>
To:     Gao Xiang <hsiangkao@linux.alibaba.com>
Cc:     linux-erofs@lists.ozlabs.org, Chao Yu <chao@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, zhangwen@coolpad.com,
        huyue2@coolpad.com
Subject: Re: [PATCH 1/2] erofs: clean up z_erofs_extent_lookback
Message-ID: <20220311151232.00003619.zbestahu@gmail.com>
In-Reply-To: <20220310182743.102365-1-hsiangkao@linux.alibaba.com>
References: <20220310182743.102365-1-hsiangkao@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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,