Re: [PATCH v14 07/10] erofs: introduce the page cache share feature

[Hongbo Li <lihongbo22@huawei.com>]

Hi,Xiang

On 2026/1/14 18:18, Gao Xiang wrote:
> 
> 
> On 2026/1/9 18:28, Hongbo Li wrote:
>> From: Hongzhen Luo <hongzhen@linux.alibaba.com>
>>
...

>> +
>> +static int erofs_ishare_iget5_set(struct inode *inode, void *data)
>> +{
>> +    struct erofs_inode *vi = EROFS_I(inode);
>> +
>> +    vi->fingerprint = *(struct erofs_inode_fingerprint *)data;
>> +    INIT_LIST_HEAD(&vi->ishare_list);
>> +    spin_lock_init(&vi->ishare_lock);
>> +    return 0;
>> +}
>> +
>> +bool erofs_ishare_fill_inode(struct inode *inode)
>> +{
>> +    struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
>> +    struct erofs_inode *vi = EROFS_I(inode);
>> +    struct erofs_inode_fingerprint fp;
>> +    struct inode *sharedinode;
>> +    unsigned long hash;
>> +
>> +    if (erofs_xattr_fill_inode_fingerprint(&fp, inode, sbi->domain_id))
>> +        return false;
>> +    hash = xxh32(fp.opaque, fp.size, 0);
>> +    sharedinode = iget5_locked(erofs_ishare_mnt->mnt_sb, hash,
>> +                   erofs_ishare_iget5_eq, erofs_ishare_iget5_set,
>> +                   &fp);
>> +    if (!sharedinode) {
>> +        kfree(fp.opaque);
>> +        return false;
>> +    }
>> +
>> +    vi->sharedinode = sharedinode;
>> +    if (inode_state_read_once(sharedinode) & I_NEW) {
>> +        if (erofs_inode_is_data_compressed(vi->datalayout)) {
>> +            sharedinode->i_mapping->a_ops = &z_erofs_aops;
> 
> It seems that it caused a build warning:
> https://lore.kernel.org/r/202601130827.dHbGXL3Y-lkp@intel.com
> 
>> +        } else {
>> +            sharedinode->i_mapping->a_ops = &erofs_aops;
>> +#ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
>> +            if (erofs_is_fileio_mode(sbi))
>> +                sharedinode->i_mapping->a_ops = &erofs_fileio_aops;
>> +#endif
>> +        }
> 
> Can we introduce a new helper for those aops setting? such as:
> 
> void erofs_inode_set_aops(struct erofs_inode *inode,
>                struct erofs_inode *realinode, bool no_fscache)

Yeah, good idea. So it also can be reuse in erofs_fill_inode.

And how about declearing it as "int erofs_iode_set_aops(struct 
erofs_inode *inode, struct erofs_inode *realinode, bool no_fscache)"; 
because the compressed case may return -EOPNOTSUPP and it seems we 
cannot break this in advance. And can we mark it inline?

Thanks,
Hongbo

> {
>      if (erofs_inode_is_data_compressed(EROFS_I(inode)->datalayout)) {
> #ifdef CONFIG_EROFS_FS_ZIP
>          DO_ONCE_LITE_IF(realinode->i_blkbits != PAGE_SHIFT,
>                  erofs_info, realinode->i_sb,
>                                "EXPERIMENTAL EROFS subpage compressed 
> block support in use. Use at your own risk!");
>          inode->i_mapping->a_ops = &z_erofs_aops;
> #else
>          err = -EOPNOTSUPP;
> #endif
>          } else {
>                  inode->i_mapping->a_ops = &erofs_aops;
> #ifdef CONFIG_EROFS_FS_ONDEMAND
>                  if (!nofscache && erofs_is_fscache_mode(realinode->i_sb))
>                          inode->i_mapping->a_ops = 
> &erofs_fscache_access_aops;
> #endif
> #ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
>                  if (erofs_is_fileio_mode(EROFS_SB(realinode->i_sb)))
>                          inode->i_mapping->a_ops = &erofs_fileio_aops;
> #endif
>          }
> }
> 
> 
> 
>> +        sharedinode->i_mode = vi->vfs_inode.i_mode;
>> +        sharedinode->i_size = vi->vfs_inode.i_size;
>> +        unlock_new_inode(sharedinode);
>> +    } else {
>> +        kfree(fp.opaque);
>> +    }
>> +    INIT_LIST_HEAD(&vi->ishare_list);
>> +    spin_lock(&EROFS_I(sharedinode)->ishare_lock);
>> +    list_add(&vi->ishare_list, &EROFS_I(sharedinode)->ishare_list);
>> +    spin_unlock(&EROFS_I(sharedinode)->ishare_lock);
>> +    return true;
>> +}
>> +
>> +void erofs_ishare_free_inode(struct inode *inode)
>> +{
>> +    struct erofs_inode *vi = EROFS_I(inode);
>> +    struct inode *sharedinode = vi->sharedinode;
>> +
>> +    if (!sharedinode)
>> +        return;
>> +    spin_lock(&EROFS_I(sharedinode)->ishare_lock);
>> +    list_del(&vi->ishare_list);
>> +    spin_unlock(&EROFS_I(sharedinode)->ishare_lock);
>> +    iput(sharedinode);
>> +    vi->sharedinode = NULL;
>> +}
>> +

...
>>           /*
>> @@ -719,6 +733,10 @@ static int erofs_fc_fill_super(struct super_block 
>> *sb, struct fs_context *fc)
>>           erofs_info(sb, "unsupported blocksize for DAX");
>>           clear_opt(&sbi->opt, DAX_ALWAYS);
>>       }
>> +    if (test_opt(&sbi->opt, INODE_SHARE) && 
>> !erofs_sb_has_ishare_xattrs(sbi)) {
>> +        erofs_info(sb, "on-disk ishare xattrs not found. Turning off 
>> inode_share.");
>> +        clear_opt(&sbi->opt, INODE_SHARE);
>> +    }
> 
> It would be better to add a message like:
> 
>      if (test_opt(&sbi->opt, INODE_SHARE))
>          erofs_info(sb, "EXPERIMENTAL EROFS page cache share support in 
> use. Use at your own risk!");
> 
> At the end of erofs_read_superblock().
> 

Ok, I will add in next version.

Thanks,
Hongbo

> Thanks,
> Gao Xiang