Re: [PATCH] erofs-utils: mkfs: also handle last compacted 2B pack in z_erofs_drop_inline_pcluster

On 2026/5/18 15:28, Zhiguo Niu wrote:
> Gao Xiang <[email protected]> 于2026年5月18日周一 12:18写道：
>>
>> Hi Zhiguo,
>>
>> On 2026/5/11 15:54, Zhiguo Niu wrote:
>>> Gao Xiang <[email protected]> 于2026年5月11日周一 12:01写道：
>>>>
>>>> Hi Zhiguo,
>>>>
>>>> On 2026/4/29 15:59, Zhiguo Niu wrote:
>>>>> With ztailpacking enabled, the current process assumes that a compacted_4b_end
>>>>> always exists in the compacted pack. However, in some specific files, the
>>>>> compacted pack may not have a compacted_4b_end. This leads to an incorrect
>>>>> modification of the last compacted_2B entry, resulting in incorrect modification
>>>>> of its clusterofs. In subsequent fsck operations, incorrect parameters will
>>>>> affect the decompression of the penultimate pcluster.
>>>>>
>>>>> This patch determines whether the last entry of the current compacted pack
>>>>> belongs to compacted 2B or 4B and then updates the correct bits accordingly.
>>>>>
>>>>> Fixes: a7c1f0575ef8 ("erofs-utils: lib: refine tailpcluster compression approach")
>>>>> Signed-off-by: Zhiguo Niu <[email protected]>
>>>>
>>>> Sorry for late response.
>>>>
>>>> I do think the issue is valid, but either the previous
>>>> solution or the proposed one is ugly.
>>>
>>> Hi Xiang,
>>> Yes it would be ideal if the same piece of common code could cover
>>> both scenarios.
>>> But I haven't figured it out yet, so I'll distinguish them like this for now. ^^
>>> thanks!
>>>>
>> Could you confirm if the following diff fixes the issue?
> Hi Xiang,
> Just confirming a few minor issues：
>>
>>
>> diff --git a/include/erofs/defs.h b/include/erofs/defs.h
>> index 9f3d0f9c35bc..0e4c2a9b53c7 100644
>> --- a/include/erofs/defs.h
>> +++ b/include/erofs/defs.h
>> @@ -218,6 +218,11 @@ typedef int64_t         s64;
>>    #define get_unaligned(ptr)    __get_unaligned_t(typeof(*(ptr)), (ptr))
>>    #define put_unaligned(val, ptr) __put_unaligned_t(typeof(*(ptr)), (val), (ptr))
>>
>> +static inline u32 get_unaligned_le16(const void *p)
>> +{
>> +       return le32_to_cpu(__get_unaligned_t(__le16, p));
>> +}
>> +
>>    static inline u32 get_unaligned_le32(const void *p)
>>    {
>>          return le32_to_cpu(__get_unaligned_t(__le32, p));
>> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
>> index 450e2647cca7..2cc9cc8009aa 100644
>> --- a/include/erofs/internal.h
>> +++ b/include/erofs/internal.h
>> @@ -210,6 +210,12 @@ struct erofs_diskbuf;
>>    #define EROFS_INODE_DATA_SOURCE_RESVSP                3
>>    #define EROFS_INODE_DATA_SOURCE_REBUILD_BLOB  4
>>
>> +enum erofs_idata_type {
>> +       EROFS_IDATA_TYPE_RAW,
>> +       EROFS_IDATA_TYPE_COMPRESSED_DEFAULT,
>> +       EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B,
>> +};
>> +
>>    #define EROFS_I_BLKADDR_DEV_ID_BIT            48
>>
>>    struct erofs_inode {
>> @@ -262,7 +268,7 @@ struct erofs_inode {
>>          unsigned short idata_size;
>>          char datasource;
>>          bool in_metabox;
>> -       bool compressed_idata;
>> +       char idata_type;
>>          bool lazy_tailblock;
>>          bool opaque;
>>          /* OVL: non-merge dir that may contain whiteout entries */
>> diff --git a/lib/compress.c b/lib/compress.c
>> index 62d2672cb665..e171aee48c0b 100644
>> --- a/lib/compress.c
>> +++ b/lib/compress.c
>> @@ -483,7 +483,7 @@ static int z_erofs_fill_inline_data(struct erofs_inode *inode, void *data,
>>    {
>>          inode->z_advise |= Z_EROFS_ADVISE_INLINE_PCLUSTER;
>>          inode->idata_size = len;
>> -       inode->compressed_idata = !raw;
>> +       inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED_DEFAULT;
>>
>>          inode->idata = malloc(inode->idata_size);
>>          if (!inode->idata)
>> @@ -980,7 +980,8 @@ int z_erofs_convert_to_compacted_format(struct erofs_inode *inode,
>>                                                &dummy_head, big_pcluster);
>>                  compacted_2b -= 16;
>>          }
>> -       DBG_BUGON(compacted_2b);
>> +       if (!compacted_4b_end && inode->idata_size)
>> +               inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B;
> if  compacted_2b && compacted_4b_end both are zero,  should not set this???

Yes, maybe it should be ahead of
`while (compacted_2b)`, and

if (compacted_2b) {
	if (!compacted_4b_end && inode->idata_size)
		inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B;
	do {
		...
	} while(compacted_2b);
}


>>
>>          /* generate compacted_4b_end */
>>          while (compacted_4b_end > 1) {
>> @@ -1210,10 +1211,12 @@ void z_erofs_drop_inline_pcluster(struct erofs_inode *inode)
>>
>>          h->h_advise = cpu_to_le16(le16_to_cpu(h->h_advise) &
>>                                    ~Z_EROFS_ADVISE_INLINE_PCLUSTER);
>> +       DBG_BUGON(inode->idata_size != le16_to_cpu(h->h_idata_size));
>>          h->h_idata_size = 0;
>> +
>>          if (!inode->eof_tailraw)
>>                  return;
>> -       DBG_BUGON(inode->compressed_idata != true);
>> +       DBG_BUGON(inode->idata_type != EROFS_IDATA_TYPE_RAW);
>   DBG_BUGON(inode->idata_type != EROFS_IDATA_TYPE_COMPRESSED_DEFAULT); ???

It should be

DBG_BUGON(inode->idata_type == EROFS_IDATA_TYPE_RAW);


>>
>>          /* patch the EOF lcluster to uncompressed type first */
>>          if (inode->datalayout == EROFS_INODE_COMPRESSED_FULL) {
>> @@ -1224,18 +1227,26 @@ void z_erofs_drop_inline_pcluster(struct erofs_inode *inode)
>>                  di->di_advise = cpu_to_le16(type);
>>          } else if (inode->datalayout == EROFS_INODE_COMPRESSED_COMPACT) {
>>                  /* handle the last compacted 4B pack */
>> -               unsigned int eofs, base, pos, v, lo;
>> +               unsigned int lclusterbits = inode->z_lclusterbits;
>> +               unsigned int lobits, eofs, base, pos, v;
>>                  u8 *out;
>>
>> -               eofs = inode->extent_isize -
>> -                       (4 << (BLK_ROUND_UP(sbi, inode->i_size) & 1));
>> -               base = round_down(eofs, 8);
>> -               pos = 16 /* encodebits */ * ((eofs - base) / 4);
>> -               out = inode->compressmeta + base;
>> -               lo = erofs_blkoff(sbi, get_unaligned_le32(out + pos / 8));
>> -               v = (type << sbi->blkszbits) | lo;
>> -               out[pos / 8] = v & 0xff;
>> -               out[pos / 8 + 1] = v >> 8;
>> +               lobits = max(lclusterbits, ilog2(Z_EROFS_LI_D0_CBLKCNT) + 1U);
>> +
>> +               if (inode->idata_type == EROFS_IDATA_TYPE_COMPRESSED_DEFAULT) {
>> +                       eofs = inode->extent_isize -
>> +                               (4 << (BLK_ROUND_UP(sbi, inode->i_size) & 1));
>> +                       base = round_down(eofs, 8);
>> +                       pos = 16 /* encodebits */ * ((eofs - base) / 4);
>> +                       out = inode->compressmeta + base + pos / 8;
>> +               } else {
>> +                       out = inode->compressmeta + inode->extent_isize - 4 - 2;
>> +                       lobits = 16 - 14 /* encodebits */ + lobits;
> if  compacted 2B, lobits=2+12=14, but encodebis =14, clusterofbits
> should be 12??
> Or is there something I'm misunderstanding?

Not sure what you say, basically compacted 2B
encodebits == 14 all the time,

and the last item should be the highest 14 bits,

so

|                    16 bits.             |
| <----  14 bits ---->|
|  2bits |  12 or 11  | 2 bits
| type   |    lobits  | the previous item |