Re: [PATCH] erofs-utils: lib: fix ztailpacking fallback across lclusters

["zhaoyifan (H)" <zhaoyifan28@huawei.com>]

On 2026/7/20 19:42, Zhiguo Niu wrote:
> Yifan Zhao <zhaoyifan28@huawei.com> 于2026年7月9日周四 20:36写道：
>> With ztailpacking, the final compressed pcluster is first stored as
>> inline data.  If the inode metadata area cannot hold it, mkfs falls back
>> to a normal tail block and drops the inline pcluster marker.
>>
>> The current fallback path assumes that the inline tail pcluster belongs
>> to the EOF lcluster.  That is not always true: the tail pcluster can
>> start in the previous lcluster and end at EOF, while its raw size still
>> fits in one block.  In that case, patching the EOF lcluster is
>> semantically wrong.
>>
>> Let's keep raw tail data whenever it fits in one block, and convert the
>> corresponding lcluster index to PLAIN during fallback.
>>
>> Reported-by: Alberto Salvia Novella <es20490446e@gmail.com>
>> Closes: https://github.com/erofs/erofs-utils/issues/51
>> Assisted-by: Codex:GPT-5.5
>> Signed-off-by: Yifan Zhao <zhaoyifan28@huawei.com>
>> ---
> Hi Yifan,
> I tested this patch to focus on the issue fixed by commit
> 277a42502a7a, and it passed.
> But I have some questions:
>>   include/erofs/internal.h |   5 +-
>>   lib/compress.c           | 109 ++++++++++++++++++++++++++++-----------
>>   2 files changed, 83 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
>> index 2cc9cc8..bdde41f 100644
>> --- a/include/erofs/internal.h
>> +++ b/include/erofs/internal.h
>> @@ -212,8 +212,11 @@ struct erofs_diskbuf;
>>
>>   enum erofs_idata_type {
>>          EROFS_IDATA_TYPE_RAW,
>> -       EROFS_IDATA_TYPE_COMPRESSED_DEFAULT,
>> +       EROFS_IDATA_TYPE_COMPRESSED,
>> +       /* compressed idata follows a final 2B compacted index pack */
>>          EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B,
>> +       /* compressed idata follows a final single-entry 4B pack after a 2B pack */
>> +       EROFS_IDATA_TYPE_COMPRESSED_4B1_PREV2B,
>>   };
>>
>>   #define EROFS_I_BLKADDR_DEV_ID_BIT             48
>> diff --git a/lib/compress.c b/lib/compress.c
>> index f7ad5a1..ec90f65 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->idata_type = EROFS_IDATA_TYPE_COMPRESSED_DEFAULT;
>> +       inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED;
>>
>>          inode->idata = malloc(inode->idata_size);
>>          if (!inode->idata)
>> @@ -664,7 +664,7 @@ frag_packing:
>>                  ictx->fragemitted = true;
>>          /* tailpcluster should be less than 1 block */
>>          } else if (may_inline && len == e->length && compressedsize < blksz) {
>> -               if (ctx->clusterofs + len <= blksz) {
> Shouldn't this condition restrict the `tail pcluster` so that it
> corresponds to only  eof`lcluster`?
> and we drop the inline pcluster just when  eof_tailraw is not null.
> Thanks!

Hi Zhiguo,


A tail pcluster may start in the previous lcluster and end in the EOF 
lcluster while its whole raw payload still fits in one physical block. 
In that case `ctx->clusterofs + len > blksz`, but `len <= blksz`, but it 
should still be converted into a single PLAIN pcluster.

This patch targets this edge case so I think change the if statement 
here is necessary?


Thanks,

Yifan

>> +               if (len <= blksz) {
>>                          inode->eof_tailraw = malloc(len);
>>                          if (!inode->eof_tailraw)
>>                                  return -ENOMEM;
>> @@ -962,6 +962,14 @@ int z_erofs_convert_to_compacted_format(struct erofs_inode *inode,
>>                  dummy_head = true;
>>          }
>>
>> +       if (inode->idata_size) {
>> +               if (compacted_2b && !compacted_4b_end)
>> +                       inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B;
>> +               else if (compacted_2b && compacted_4b_end == 1)
>> +                       inode->idata_type =
>> +                               EROFS_IDATA_TYPE_COMPRESSED_4B1_PREV2B;
>> +       }
>> +
>>          /* generate compacted_4b_initial */
>>          while (compacted_4b_initial) {
>>                  in = parse_legacy_indexes(cv, 2, in);
>> @@ -974,8 +982,6 @@ int z_erofs_convert_to_compacted_format(struct erofs_inode *inode,
>>
>>          /* generate compacted_2b */
>>          if (compacted_2b) {
>> -               if (!compacted_4b_end && inode->idata_size)
>> -                       inode->idata_type = EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B;
>>                  do {
>>                          in = parse_legacy_indexes(cv, 16, in);
>>                          out = write_compacted_indexes(out, cv, &blkaddr,
>> @@ -1205,11 +1211,65 @@ out:
>>          return metabuf;
>>   }
>>
>> +static void z_erofs_patch_tail_compacted_index(struct erofs_inode *inode,
>> +                                              bool previous,
>> +                                              unsigned int type)
>> +{
>> +       const unsigned int totalidx = BLK_ROUND_UP(inode->sbi, inode->i_size);
>> +       const unsigned int lobits = max_t(unsigned int, inode->z_lclusterbits,
>> +                               ilog2(Z_EROFS_LI_D0_CBLKCNT) + 1U);
>> +       u8 *base = inode->compressmeta;
>> +       u8 *pack = base + inode->extent_isize;
>> +       unsigned int bitpos, bitoff;
>> +       u8 *out;
>> +       u32 v;
>> +
>> +       DBG_BUGON(!totalidx);
>> +       DBG_BUGON(inode->z_lclusterbits > 14);
>> +
>> +       if (inode->idata_type == EROFS_IDATA_TYPE_COMPRESSED_END_OF_2B) {
>> +               pack -= 32;
>> +               bitpos = 14 * (previous ? 14 : 15);
>> +               goto out;
>> +       }
>> +
>> +       /* Last compacted index pack is 4B */
>> +       pack -= 8;
>> +       if (!(totalidx & 1)) {
>> +               bitpos = previous ? 0 : 16;
>> +               goto out;
>> +       }
>> +
>> +       if (!previous) {
>> +               bitpos = 0;
>> +               goto out;
>> +       }
>> +
>> +       /* Second to last compacted index pack is 2B */
>> +       if (inode->idata_type == EROFS_IDATA_TYPE_COMPRESSED_4B1_PREV2B) {
>> +               pack -= 32;
>> +               bitpos = 14 * 15;
>> +       } else {
>> +               pack -= 8;
>> +               bitpos = 16;
>> +       }
>> +out:
>> +       DBG_BUGON(pack < base);
>> +       bitoff = bitpos & 7;
>> +       out = pack + bitpos / 8;
>> +       v = get_unaligned_le32(out);
>> +       v &= ~(Z_EROFS_LI_LCLUSTER_TYPE_MASK << (lobits + bitoff));
>> +       v |= type << (lobits + bitoff);
>> +       put_unaligned_le32(v, out);
>> +}
>> +
>>   void z_erofs_drop_inline_pcluster(struct erofs_inode *inode)
>>   {
>>          struct erofs_sb_info *sbi = inode->sbi;
>>          const unsigned int type = Z_EROFS_LCLUSTER_TYPE_PLAIN;
>>          struct z_erofs_map_header *h = inode->compressmeta;
>> +       erofs_off_t rawstart;
>> +       erofs_blk_t head_lcn, eof_lcn;
>>
>>          h->h_advise = cpu_to_le16(le16_to_cpu(h->h_advise) &
>>                                    ~Z_EROFS_ADVISE_INLINE_PCLUSTER);
>> @@ -1218,38 +1278,27 @@ void z_erofs_drop_inline_pcluster(struct erofs_inode *inode)
>>          if (!inode->eof_tailraw)
>>                  return;
>>          DBG_BUGON(inode->idata_type == EROFS_IDATA_TYPE_RAW);
>> +       DBG_BUGON(!inode->i_size);
>> +       DBG_BUGON(inode->eof_tailrawsize > erofs_blksiz(sbi));
>> +       DBG_BUGON(inode->eof_tailrawsize > inode->i_size);
>> +
>> +       rawstart = inode->i_size - inode->eof_tailrawsize;
>> +       head_lcn = rawstart >> sbi->blkszbits;
>> +       eof_lcn = (inode->i_size - 1) >> sbi->blkszbits;
>> +       DBG_BUGON(head_lcn != eof_lcn && head_lcn + 1 != eof_lcn);
>>
>> -       /* patch the EOF lcluster to uncompressed type first */
>> +       /* patch the tail pcluster head to uncompressed type first */
>>          if (inode->datalayout == EROFS_INODE_COMPRESSED_FULL) {
>>                  struct z_erofs_lcluster_index *di =
>> -                       (inode->compressmeta + inode->extent_isize) -
>> -                       sizeof(struct z_erofs_lcluster_index);
>> +                       (void *)((u8 *)inode->compressmeta +
>> +                                Z_EROFS_LEGACY_MAP_HEADER_SIZE +
>> +                                head_lcn *
>> +                                sizeof(struct z_erofs_lcluster_index));
>>
>>                  di->di_advise = cpu_to_le16(type);
>>          } else if (inode->datalayout == EROFS_INODE_COMPRESSED_COMPACT) {
>> -               /* handle the last compacted 4B/2B pack */
>> -               unsigned int lclusterbits = inode->z_lclusterbits;
>> -               unsigned int lobits, eofs, base, pos, v;
>> -               u8 *out;
>> -
>> -               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 -
>> -                               sizeof(__le32) - sizeof(__le16);
>> -                       lobits = 16 - 14 /* encodebits */ + lobits;
>> -               }
>> -
>> -               v = (get_unaligned_le16(out) & (BIT(lobits) - 1)) |
>> -                       (type << lobits);
>> -               *out = v & 0xff;
>> -               *(out + 1) = v >> 8;
>> +               z_erofs_patch_tail_compacted_index(inode, head_lcn != eof_lcn,
>> +                                                  type);
>>          } else {
>>                  DBG_BUGON(1);
>>                  return;
>> --
>> 2.47.3
>>
>>