Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()

[Chao Yu <chao@kernel.org>]

On 7/8/25 10:58, Gao Xiang wrote:
> Hi Chao,
> 
> On 2025/7/7 16:47, Chao Yu wrote:
>> All below functions will do sanity check on m->type, let's move sanity
>> check to z_erofs_load_compact_lcluster() for cleanup.
>> - z_erofs_map_blocks_fo
>> - z_erofs_get_extent_compressedlen
>> - z_erofs_get_extent_decompressedlen
>> - z_erofs_extent_lookback
>>
>> Signed-off-by: Chao Yu <chao@kernel.org>
> How about appending the following diff to tidy up the code
> a bit further (avoid `goto map_block` and `goto out`)?

Xiang,

Looks good to me, will append the diff in the patch.

Thanks,

> 
> 
>  fs/erofs/zmap.c | 67 +++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index e530b152e14e..431199452542 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -327,21 +327,18 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
>      DBG_BUGON(lcn == initial_lcn &&
>            m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD);
>  
> -    if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> -        if (m->delta[0] != 1) {
> -            erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> -            DBG_BUGON(1);
> -            return -EFSCORRUPTED;
> -        }
> -        if (m->compressedblks)
> -            goto out;
> +    if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD && m->delta[0] != 1) {
> +        erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> +        DBG_BUGON(1);
> +        return -EFSCORRUPTED;
>      }
>  
>      /*
>       * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type rather
>       * than CBLKCNT, it's a 1 block-sized pcluster.
>       */
> -    m->compressedblks = 1;
> +    if (m->type != Z_EROFS_LCLUSTER_TYPE_NONHEAD || !m->compressedblks)
> +        m->compressedblks = 1;
>  out:
>      m->map->m_plen = erofs_pos(sb, m->compressedblks);
>      return 0;
> @@ -422,36 +419,34 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
>      map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED;
>      end = (m.lcn + 1ULL) << lclusterbits;
>  
> -    if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> -        if (endoff >= m.clusterofs) {
> -            m.headtype = m.type;
> -            map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
> -            /*
> -             * For ztailpacking files, in order to inline data more
> -             * effectively, special EOF lclusters are now supported
> -             * which can have three parts at most.
> -             */
> -            if (ztailpacking && end > inode->i_size)
> -                end = inode->i_size;
> -            goto map_block;
> +    if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD && endoff >= m.clusterofs) {
> +        m.headtype = m.type;
> +        map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
> +        /*
> +         * For ztailpacking files, in order to inline data more
> +         * effectively, special EOF lclusters are now supported
> +         * which can have three parts at most.
> +         */
> +        if (ztailpacking && end > inode->i_size)
> +            end = inode->i_size;
> +    } else {
> +        if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> +            /* m.lcn should be >= 1 if endoff < m.clusterofs */
> +            if (!m.lcn) {
> +                erofs_err(sb, "invalid logical cluster 0 at nid %llu",
> +                      vi->nid);
> +                err = -EFSCORRUPTED;
> +                goto unmap_out;
> +            }
> +            end = (m.lcn << lclusterbits) | m.clusterofs;
> +            map->m_flags |= EROFS_MAP_FULL_MAPPED;
> +            m.delta[0] = 1;
>          }
> -        /* m.lcn should be >= 1 if endoff < m.clusterofs */
> -        if (!m.lcn) {
> -            erofs_err(sb, "invalid logical cluster 0 at nid %llu",
> -                  vi->nid);
> -            err = -EFSCORRUPTED;
> +        /* get the corresponding first chunk */
> +        err = z_erofs_extent_lookback(&m, m.delta[0]);
> +        if (err)
>              goto unmap_out;
> -        }
> -        end = (m.lcn << lclusterbits) | m.clusterofs;
> -        map->m_flags |= EROFS_MAP_FULL_MAPPED;
> -        m.delta[0] = 1;
>      }
> -
> -    /* get the corresponding first chunk */
> -    err = z_erofs_extent_lookback(&m, m.delta[0]);
> -    if (err)
> -        goto unmap_out;
> -map_block:
>      if (m.partialref)
>          map->m_flags |= EROFS_MAP_PARTIAL_REF;
>      map->m_llen = end - map->m_la;