fs/erofs/zmap.c | 63 +++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 39 deletions(-)
There's no need to enumerate each type. No logic changes.
Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
---
v5: Exit the loop and return EOPNOTSUPP when `!lookback_distance`.
v4: https://lore.kernel.org/linux-erofs/20250207094515.2589242-1-hongzhen@linux.alibaba.com/
v3: https://lore.kernel.org/all/20250207085056.2502010-1-hongzhen@linux.alibaba.com/
v2: https://lore.kernel.org/all/20250207080829.2405528-1-hongzhen@linux.alibaba.com/
v1: https://lore.kernel.org/all/20250207064135.2249529-1-hongzhen@linux.alibaba.com/
---
fs/erofs/zmap.c | 63 +++++++++++++++++++------------------------------
1 file changed, 24 insertions(+), 39 deletions(-)
diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 689437e99a5a..d278ebd60281 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -265,26 +265,22 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
if (err)
return err;
- switch (m->type) {
- case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
+ if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
+ erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
+ m->type, lcn, vi->nid);
+ DBG_BUGON(1);
+ return -EOPNOTSUPP;
+ } else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
lookback_distance = m->delta[0];
if (!lookback_distance)
- goto err_bogus;
+ break;
continue;
- case Z_EROFS_LCLUSTER_TYPE_PLAIN:
- case Z_EROFS_LCLUSTER_TYPE_HEAD1:
- case Z_EROFS_LCLUSTER_TYPE_HEAD2:
+ } else {
m->headtype = m->type;
m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
return 0;
- default:
- erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
- m->type, lcn, vi->nid);
- DBG_BUGON(1);
- return -EOPNOTSUPP;
}
}
-err_bogus:
erofs_err(sb, "bogus lookback distance %u @ lcn %lu of nid %llu",
lookback_distance, m->lcn, vi->nid);
DBG_BUGON(1);
@@ -329,35 +325,28 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
DBG_BUGON(lcn == initial_lcn &&
m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD);
- switch (m->type) {
- case Z_EROFS_LCLUSTER_TYPE_PLAIN:
- case Z_EROFS_LCLUSTER_TYPE_HEAD1:
- case Z_EROFS_LCLUSTER_TYPE_HEAD2:
+ 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;
+ } else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
/*
* if the 1st NONHEAD lcluster is actually PLAIN or HEAD type
* rather than CBLKCNT, it's a 1 block-sized pcluster.
*/
m->compressedblks = 1;
- break;
- case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
- if (m->delta[0] != 1)
- goto err_bonus_cblkcnt;
- if (m->compressedblks)
- break;
- fallthrough;
- default:
- erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn,
- vi->nid);
- DBG_BUGON(1);
- return -EFSCORRUPTED;
+ goto out;
}
+ erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
+ DBG_BUGON(1);
+ return -EFSCORRUPTED;
out:
m->map->m_plen = erofs_pos(sb, m->compressedblks);
return 0;
-err_bonus_cblkcnt:
- erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
- DBG_BUGON(1);
- return -EFSCORRUPTED;
}
static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
@@ -386,9 +375,7 @@ static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
m->delta[1] = 1;
DBG_BUGON(1);
}
- } else if (m->type == Z_EROFS_LCLUSTER_TYPE_PLAIN ||
- m->type == Z_EROFS_LCLUSTER_TYPE_HEAD1 ||
- m->type == Z_EROFS_LCLUSTER_TYPE_HEAD2) {
+ } else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
if (lcn != headlcn)
break; /* ends at the next HEAD lcluster */
m->delta[1] = 1;
@@ -452,8 +439,7 @@ static int z_erofs_do_map_blocks(struct inode *inode,
}
/* m.lcn should be >= 1 if endoff < m.clusterofs */
if (!m.lcn) {
- erofs_err(inode->i_sb,
- "invalid logical cluster 0 at nid %llu",
+ erofs_err(inode->i_sb, "invalid logical cluster 0 at nid %llu",
vi->nid);
err = -EFSCORRUPTED;
goto unmap_out;
@@ -469,8 +455,7 @@ static int z_erofs_do_map_blocks(struct inode *inode,
goto unmap_out;
break;
default:
- erofs_err(inode->i_sb,
- "unknown type %u @ offset %llu of nid %llu",
+ erofs_err(inode->i_sb, "unknown type %u @ offset %llu of nid %llu",
m.type, ofs, vi->nid);
err = -EOPNOTSUPP;
goto unmap_out;
--
2.43.5On 2025/2/10 11:29, Hongzhen Luo wrote:
> There's no need to enumerate each type. No logic changes.
>
> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
Looks good to me, feel free to add:
Reviewed-by: Chao Yu <chao@kernel.org>
And one minor comment below.
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index 689437e99a5a..d278ebd60281 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -265,26 +265,22 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
> if (err)
> return err;
>
> - switch (m->type) {
> - case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
> + if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
> + erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
> + m->type, lcn, vi->nid);
> + DBG_BUGON(1);
> + return -EOPNOTSUPP;
> + } else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {> lookback_distance = m->delta[0];
> if (!lookback_distance)
> - goto err_bogus;
> + break;
> continue;
> - case Z_EROFS_LCLUSTER_TYPE_PLAIN:
> - case Z_EROFS_LCLUSTER_TYPE_HEAD1:
> - case Z_EROFS_LCLUSTER_TYPE_HEAD2:
> + } else {
> m->headtype = m->type;
> m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
> return 0;
> - default:
> - erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
> - m->type, lcn, vi->nid);
> - DBG_BUGON(1);
> - return -EOPNOTSUPP;
Should return EFSCORRUPTED here? is m->type >= Z_EROFS_LCLUSTER_TYPE_MAX a possible
case?
Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
then we can treat m->type as reliable variable later.
advise = le16_to_cpu(di->di_advise);
m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
...
DBG_BUGON(1);
return -EFSCORRUPTED;
}
Thanks,
> }
> }
> -err_bogus:
> erofs_err(sb, "bogus lookback distance %u @ lcn %lu of nid %llu",
> lookback_distance, m->lcn, vi->nid);
> DBG_BUGON(1);
> @@ -329,35 +325,28 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
> DBG_BUGON(lcn == initial_lcn &&
> m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD);
>
> - switch (m->type) {
> - case Z_EROFS_LCLUSTER_TYPE_PLAIN:
> - case Z_EROFS_LCLUSTER_TYPE_HEAD1:
> - case Z_EROFS_LCLUSTER_TYPE_HEAD2:
> + 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;
> + } else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
> /*
> * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type
> * rather than CBLKCNT, it's a 1 block-sized pcluster.
> */
> m->compressedblks = 1;
> - break;
> - case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
> - if (m->delta[0] != 1)
> - goto err_bonus_cblkcnt;
> - if (m->compressedblks)
> - break;
> - fallthrough;
> - default:
> - erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn,
> - vi->nid);
> - DBG_BUGON(1);
> - return -EFSCORRUPTED;
> + goto out;
> }
> + erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> + DBG_BUGON(1);
> + return -EFSCORRUPTED;
> out:
> m->map->m_plen = erofs_pos(sb, m->compressedblks);
> return 0;
> -err_bonus_cblkcnt:
> - erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> - DBG_BUGON(1);
> - return -EFSCORRUPTED;
> }
>
> static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
> @@ -386,9 +375,7 @@ static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
> m->delta[1] = 1;
> DBG_BUGON(1);
> }
> - } else if (m->type == Z_EROFS_LCLUSTER_TYPE_PLAIN ||
> - m->type == Z_EROFS_LCLUSTER_TYPE_HEAD1 ||
> - m->type == Z_EROFS_LCLUSTER_TYPE_HEAD2) {
> + } else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
> if (lcn != headlcn)
> break; /* ends at the next HEAD lcluster */
> m->delta[1] = 1;
> @@ -452,8 +439,7 @@ static int z_erofs_do_map_blocks(struct inode *inode,
> }
> /* m.lcn should be >= 1 if endoff < m.clusterofs */
> if (!m.lcn) {
> - erofs_err(inode->i_sb,
> - "invalid logical cluster 0 at nid %llu",
> + erofs_err(inode->i_sb, "invalid logical cluster 0 at nid %llu",
> vi->nid);
> err = -EFSCORRUPTED;
> goto unmap_out;
> @@ -469,8 +455,7 @@ static int z_erofs_do_map_blocks(struct inode *inode,
> goto unmap_out;
> break;
> default:
> - erofs_err(inode->i_sb,
> - "unknown type %u @ offset %llu of nid %llu",
> + erofs_err(inode->i_sb, "unknown type %u @ offset %llu of nid %llu",
> m.type, ofs, vi->nid);
> err = -EOPNOTSUPP;
> goto unmap_out;
Hi Chao,
On 2025/3/16 10:36, Chao Yu wrote:
> On 2025/2/10 11:29, Hongzhen Luo wrote:
>> There's no need to enumerate each type. No logic changes.
>>
>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>
> Looks good to me, feel free to add:
>
> Reviewed-by: Chao Yu <chao@kernel.org>
>
> And one minor comment below.
>
>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>> index 689437e99a5a..d278ebd60281 100644
>> --- a/fs/erofs/zmap.c
>> +++ b/fs/erofs/zmap.c
>> @@ -265,26 +265,22 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>> if (err)
>> return err;
>> - switch (m->type) {
>> - case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
>> + if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>> + erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
>> + m->type, lcn, vi->nid);
>> + DBG_BUGON(1);
>> + return -EOPNOTSUPP;
> > + } else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {> lookback_distance = m->delta[0];
>> if (!lookback_distance)
>> - goto err_bogus;
>> + break;
>> continue;
>> - case Z_EROFS_LCLUSTER_TYPE_PLAIN:
>> - case Z_EROFS_LCLUSTER_TYPE_HEAD1:
>> - case Z_EROFS_LCLUSTER_TYPE_HEAD2:
>> + } else {
>> m->headtype = m->type;
>> m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
>> return 0;
>> - default:
>> - erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
>> - m->type, lcn, vi->nid);
>> - DBG_BUGON(1);
>> - return -EOPNOTSUPP;
>
> Should return EFSCORRUPTED here? is m->type >= Z_EROFS_LCLUSTER_TYPE_MAX a possible
> case?
It's impossible by the latest on-disk definition, see:
#define Z_EROFS_LI_LCLUSTER_TYPE_MASK (Z_EROFS_LCLUSTER_TYPE_MAX - 1)
Previously, it was useful before Z_EROFS_LCLUSTER_TYPE_HEAD2 was
introduced, but the `default:` case is already deadcode now.
>
> Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
> then we can treat m->type as reliable variable later.
>
> advise = le16_to_cpu(di->di_advise);
> m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
> if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
It's always false here.
Thanks,
Gao Xiang
On 3/17/25 01:17, Gao Xiang wrote:
> Hi Chao,
>
> On 2025/3/16 10:36, Chao Yu wrote:
>> On 2025/2/10 11:29, Hongzhen Luo wrote:
>>> There's no need to enumerate each type. No logic changes.
>>>
>>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>>
>> Looks good to me, feel free to add:
>>
>> Reviewed-by: Chao Yu <chao@kernel.org>
>>
>> And one minor comment below.
>>
>>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>>> index 689437e99a5a..d278ebd60281 100644
>>> --- a/fs/erofs/zmap.c
>>> +++ b/fs/erofs/zmap.c
>>> @@ -265,26 +265,22 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>>> if (err)
>>> return err;
>>> - switch (m->type) {
>>> - case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
>>> + if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>> + erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
>>> + m->type, lcn, vi->nid);
>>> + DBG_BUGON(1);
>>> + return -EOPNOTSUPP;
>> > + } else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {> lookback_distance = m->delta[0];
>>> if (!lookback_distance)
>>> - goto err_bogus;
>>> + break;
>>> continue;
>>> - case Z_EROFS_LCLUSTER_TYPE_PLAIN:
>>> - case Z_EROFS_LCLUSTER_TYPE_HEAD1:
>>> - case Z_EROFS_LCLUSTER_TYPE_HEAD2:
>>> + } else {
>>> m->headtype = m->type;
>>> m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
>>> return 0;
>>> - default:
>>> - erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
>>> - m->type, lcn, vi->nid);
>>> - DBG_BUGON(1);
>>> - return -EOPNOTSUPP;
>>
>> Should return EFSCORRUPTED here? is m->type >= Z_EROFS_LCLUSTER_TYPE_MAX a possible
>> case?
>
> It's impossible by the latest on-disk definition, see:
> #define Z_EROFS_LI_LCLUSTER_TYPE_MASK (Z_EROFS_LCLUSTER_TYPE_MAX - 1)
>
> Previously, it was useful before Z_EROFS_LCLUSTER_TYPE_HEAD2 was
> introduced, but the `default:` case is already deadcode now.
Xiang, thanks for the explanation.
So seems it can happen when mounting last image w/ old kernel which can not
support newly introduced Z_EROFS_LCLUSTER_TYPE_* type, then it makes sense to
return EOPNOTSUPP.
>
>>
>> Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
>> then we can treat m->type as reliable variable later.
>>
>> advise = le16_to_cpu(di->di_advise);
>> m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
>> if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>
> It's always false here.
So, what do you think of this?
From af584b2eacd468f145e9ee31ccdeedb7355d5afd Mon Sep 17 00:00:00 2001
From: Chao Yu <chao@kernel.org>
Date: Mon, 17 Mar 2025 13:57:55 +0800
Subject: [PATCH] erofs: remove dead codes for cleanup
z_erofs_extent_lookback() and z_erofs_get_extent_decompressedlen() tries
to do sanity check on m->type, however their caller z_erofs_map_blocks_fo()
has already checked that, so let's remove those dead codes.
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/erofs/zmap.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 8de50df05dfe..4d883ec212d7 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -265,17 +265,12 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
if (err)
return err;
- if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
- erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
- m->type, lcn, vi->nid);
- DBG_BUGON(1);
- return -EOPNOTSUPP;
- } else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
+ if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
lookback_distance = m->delta[0];
if (!lookback_distance)
break;
continue;
- } else {
+ } else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
m->headtype = m->type;
m->map->m_la = (lcn << lclusterbits) | m->clusterofs;
return 0;
@@ -379,11 +374,6 @@ static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
if (lcn != headlcn)
break; /* ends at the next HEAD lcluster */
m->delta[1] = 1;
- } else {
- erofs_err(inode->i_sb, "unknown type %u @ lcn %llu of nid %llu",
- m->type, lcn, vi->nid);
- DBG_BUGON(1);
- return -EOPNOTSUPP;
}
lcn += m->delta[1];
}
--
2.48.1
>
> Thanks,
> Gao Xiang
>
Hi Chao,
On 2025/3/17 14:03, Chao Yu wrote:
> On 3/17/25 01:17, Gao Xiang wrote:
>> Hi Chao,
>>
...
>>
>> Previously, it was useful before Z_EROFS_LCLUSTER_TYPE_HEAD2 was
>> introduced, but the `default:` case is already deadcode now.
>
> Xiang, thanks for the explanation.
>
> So seems it can happen when mounting last image w/ old kernel which can not
> support newly introduced Z_EROFS_LCLUSTER_TYPE_* type, then it makes sense to
> return EOPNOTSUPP.
Yeah.
>
>>
>>>
>>> Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
>>> then we can treat m->type as reliable variable later.
>>>
>>> advise = le16_to_cpu(di->di_advise);
>>> m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
>>> if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>
>> It's always false here.
>
> So, what do you think of this?
>
> From af584b2eacd468f145e9ee31ccdeedb7355d5afd Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao@kernel.org>
> Date: Mon, 17 Mar 2025 13:57:55 +0800
> Subject: [PATCH] erofs: remove dead codes for cleanup
>
> z_erofs_extent_lookback() and z_erofs_get_extent_decompressedlen() tries
> to do sanity check on m->type, however their caller z_erofs_map_blocks_fo()
> has already checked that, so let's remove those dead codes.
z_erofs_extent_lookback() will (lookback) read new lcn in
z_erofs_load_lcluster_from_disk() so it won't be covered by
the original z_erofs_map_blocks_fo().
I think this check can be resolved in
z_erofs_load_lcluster_from_disk() instead but maybe address
for the next cycle? since there are already enough features
for this cycle and I have to make sure no major issues....
Thanks,
Gao Xiang
On 3/17/25 14:15, Gao Xiang wrote:
> Hi Chao,
>
> On 2025/3/17 14:03, Chao Yu wrote:
>> On 3/17/25 01:17, Gao Xiang wrote:
>>> Hi Chao,
>>>
>
> ...
>
>>>
>>> Previously, it was useful before Z_EROFS_LCLUSTER_TYPE_HEAD2 was
>>> introduced, but the `default:` case is already deadcode now.
>>
>> Xiang, thanks for the explanation.
>>
>> So seems it can happen when mounting last image w/ old kernel which can not
>> support newly introduced Z_EROFS_LCLUSTER_TYPE_* type, then it makes sense to
>> return EOPNOTSUPP.
>
> Yeah.
>
>>
>>>
>>>>
>>>> Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
>>>> then we can treat m->type as reliable variable later.
>>>>
>>>> advise = le16_to_cpu(di->di_advise);
>>>> m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
>>>> if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>>
>>> It's always false here.
>>
>> So, what do you think of this?
>>
>> From af584b2eacd468f145e9ee31ccdeedb7355d5afd Mon Sep 17 00:00:00 2001
>> From: Chao Yu <chao@kernel.org>
>> Date: Mon, 17 Mar 2025 13:57:55 +0800
>> Subject: [PATCH] erofs: remove dead codes for cleanup
>>
>> z_erofs_extent_lookback() and z_erofs_get_extent_decompressedlen() tries
>> to do sanity check on m->type, however their caller z_erofs_map_blocks_fo()
>> has already checked that, so let's remove those dead codes.
>
> z_erofs_extent_lookback() will (lookback) read new lcn in
> z_erofs_load_lcluster_from_disk() so it won't be covered by
> the original z_erofs_map_blocks_fo().
Xiang,
Oh, I see, changed here:
- z_erofs_extent_lookback
- z_erofs_load_lcluster_from_disk
- z_erofs_load_full_lcluster
: m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
- z_erofs_load_compact_lcluster
: m->type = type;
>
> I think this check can be resolved in
> z_erofs_load_lcluster_from_disk() instead but maybe address
> for the next cycle? since there are already enough features
> for this cycle and I have to make sure no major issues....
Yeah, it's fine to check the cleanup later, let's keep focusing
on improving patches in dev now.
Thanks,
>
> Thanks,
> Gao Xiang
>
On 2025/3/17 14:42, Chao Yu wrote:
> On 3/17/25 14:15, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2025/3/17 14:03, Chao Yu wrote:
>>> On 3/17/25 01:17, Gao Xiang wrote:
>>>> Hi Chao,
>>>>
>>
>> ...
>>
>>>>
>>>> Previously, it was useful before Z_EROFS_LCLUSTER_TYPE_HEAD2 was
>>>> introduced, but the `default:` case is already deadcode now.
>>>
>>> Xiang, thanks for the explanation.
>>>
>>> So seems it can happen when mounting last image w/ old kernel which can not
>>> support newly introduced Z_EROFS_LCLUSTER_TYPE_* type, then it makes sense to
>>> return EOPNOTSUPP.
>>
>> Yeah.
>>
>>>
>>>>
>>>>>
>>>>> Btw, we'd better to do sanity check for m->type in z_erofs_load_full_lcluster(),
>>>>> then we can treat m->type as reliable variable later.
>>>>>
>>>>> advise = le16_to_cpu(di->di_advise);
>>>>> m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
>>>>> if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>>>
>>>> It's always false here.
>>>
>>> So, what do you think of this?
>>>
>>> From af584b2eacd468f145e9ee31ccdeedb7355d5afd Mon Sep 17 00:00:00 2001
>>> From: Chao Yu <chao@kernel.org>
>>> Date: Mon, 17 Mar 2025 13:57:55 +0800
>>> Subject: [PATCH] erofs: remove dead codes for cleanup
>>>
>>> z_erofs_extent_lookback() and z_erofs_get_extent_decompressedlen() tries
>>> to do sanity check on m->type, however their caller z_erofs_map_blocks_fo()
>>> has already checked that, so let's remove those dead codes.
>>
>> z_erofs_extent_lookback() will (lookback) read new lcn in
>> z_erofs_load_lcluster_from_disk() so it won't be covered by
>> the original z_erofs_map_blocks_fo().
>
> Xiang,
>
> Oh, I see, changed here:
>
> - z_erofs_extent_lookback
> - z_erofs_load_lcluster_from_disk
> - z_erofs_load_full_lcluster
> : m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
> - z_erofs_load_compact_lcluster
> : m->type = type;
Yeah, we'd better to move all checks into
z_erofs_load_lcluster_from_disk() later.
>
>>
>> I think this check can be resolved in
>> z_erofs_load_lcluster_from_disk() instead but maybe address
>> for the next cycle? since there are already enough features
>> for this cycle and I have to make sure no major issues....
>
> Yeah, it's fine to check the cleanup later, let's keep focusing
> on improving patches in dev now.
Yes.
Thanks,
Gao Xiang
>
> Thanks,
>
>>
>> Thanks,
>> Gao Xiang
>>
On 2025/3/17 01:17, Gao Xiang wrote:
> Hi Chao,
>
> On 2025/3/16 10:36, Chao Yu wrote:
>> On 2025/2/10 11:29, Hongzhen Luo wrote:
>>> There's no need to enumerate each type. No logic changes.
>>>
>>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>>
>> Looks good to me, feel free to add:
>>
>> Reviewed-by: Chao Yu <chao@kernel.org>
>>
>> And one minor comment below.
>>
>>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>>> index 689437e99a5a..d278ebd60281 100644
>>> --- a/fs/erofs/zmap.c
>>> +++ b/fs/erofs/zmap.c
>>> @@ -265,26 +265,22 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>>> if (err)
>>> return err;
>>> - switch (m->type) {
>>> - case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
>>> + if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>> + erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
>>> + m->type, lcn, vi->nid);
>>> + DBG_BUGON(1);
>>> + return -EOPNOTSUPP;
`m->type >= Z_EROFS_LCLUSTER_TYPE_MAX` is checked here BTW,
I think the patch is good.
Thanks,
Gao Xiang
On 2025/2/10 11:29, Hongzhen Luo wrote: > There's no need to enumerate each type. No logic changes. > > Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> Thanks, Gao Xiang
© 2016 - 2026 Red Hat, Inc.