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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2026/1/15 09:21, Hongbo Li wrote:
> 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.

yes, `int` return is good.

>  And can we mark it inline?

you could move this one to internal.h.

Thanks,
Gao Xiang

> 
> Thanks,
> Hongbo