Re: [PATCH v8 4/9] erofs: support user-defined fingerprint name

[Hongbo Li <lihongbo22@huawei.com>]

Hi Xiang,

On 2025/11/17 10:54, Gao Xiang wrote:
> 
> 
> On 2025/11/14 17:55, Hongbo Li wrote:
>> From: Hongzhen Luo <hongzhen@linux.alibaba.com>
>>
>> When creating the EROFS image, users can specify the fingerprint name.
>> This is to prepare for the upcoming inode page cache share.
>>
>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>> ---
>>   fs/erofs/Kconfig    |  9 +++++++++
>>   fs/erofs/erofs_fs.h |  6 ++++--
>>   fs/erofs/internal.h |  6 ++++++
>>   fs/erofs/super.c    |  5 ++++-
>>   fs/erofs/xattr.c    | 26 ++++++++++++++++++++++++++
>>   fs/erofs/xattr.h    |  6 ++++++
>>   6 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/erofs/Kconfig b/fs/erofs/Kconfig
>> index d81f3318417d..1b5c0cd99203 100644
>> --- a/fs/erofs/Kconfig
>> +++ b/fs/erofs/Kconfig
>> @@ -194,3 +194,12 @@ config EROFS_FS_PCPU_KTHREAD_HIPRI
>>         at higher priority.
>>         If unsure, say N.
>> +
>> +config EROFS_FS_INODE_SHARE
>> +    bool "EROFS inode page cache share support (experimental)"
>> +    depends on EROFS_FS && EROFS_FS_XATTR && !EROFS_FS_ONDEMAND
>> +    help
>> +      This permits EROFS to share page cache for files with same
>> +      fingerprints.
> 
> I tend to use "EROFS_FS_PAGE_CACHE_SHARE" since it's closer to
> user impact definition (inode sharing is ambiguious), but we
> could leave "ishare.c" since it's closer to the implementation
> details.
> 
> And how about:
> 
> config EROFS_FS_PAGE_CACHE_SHARE
>      bool "EROFS page cache share support (experimental)"
>      depends on EROFS_FS && EROFS_FS_XATTR && !EROFS_FS_ONDEMAND
>      help
>        This enables page cache sharing among inodes with identical
>        content fingerprints on the same device.
> 
>        If unsure, say N.
> 
>> +
>> +      If unsure, say N.
>> \ No newline at end of file
> 
> "\ No newline at end of file" should be fixed.
> 
>> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
>> index 3d5738f80072..104518cd161d 100644
>> --- a/fs/erofs/erofs_fs.h
>> +++ b/fs/erofs/erofs_fs.h
>> @@ -35,8 +35,9 @@
>>   #define EROFS_FEATURE_INCOMPAT_XATTR_PREFIXES    0x00000040
>>   #define EROFS_FEATURE_INCOMPAT_48BIT        0x00000080
>>   #define EROFS_FEATURE_INCOMPAT_METABOX        0x00000100
>> +#define EROFS_FEATURE_INCOMPAT_ISHARE_KEY    0x00000200
> 
> I do think it should be a compatible feature since images can be
> mounted in the old kernels without any issue, and it should be
> renamed as
> 
> EROFS_FEATURE_COMPAT_ISHARE_XATTRS
> 
>>   #define EROFS_ALL_FEATURE_INCOMPAT        \
>> -    ((EROFS_FEATURE_INCOMPAT_METABOX << 1) - 1)
>> +    ((EROFS_FEATURE_INCOMPAT_ISHARE_KEY << 1) - 1)
>>   #define EROFS_SB_EXTSLOT_SIZE    16
>> @@ -83,7 +84,8 @@ struct erofs_super_block {
>>       __le32 xattr_prefix_start;    /* start of long xattr prefixes */
>>       __le64 packed_nid;    /* nid of the special packed inode */
>>       __u8 xattr_filter_reserved; /* reserved for xattr name filter */
>> -    __u8 reserved[3];
>> +    __u8 ishare_key_start;    /* start of ishare key */
> 
> ishare_xattr_prefix_id; ?
> 
>> +    __u8 reserved[2];
>>       __le32 build_time;    /* seconds added to epoch for mkfs time */
>>       __le64 rootnid_8b;    /* (48BIT on) nid of root directory */
>>       __le64 reserved2;
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index e80b35db18e4..3ebbb7c5d085 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -167,6 +167,11 @@ struct erofs_sb_info {
>>       struct erofs_domain *domain;
>>       char *fsid;
>>       char *domain_id;
>> +
>> +    /* inode page cache share support */
>> +    u8 ishare_key_start;
> 
>      u8 ishare_xattr_pfx;
> 
>> +    u8 ishare_key_idx;
> 
> why need this, considering we could just use
> 
> sbi->xattr_prefixes[sbi->ishare_xattr_pfx]
> 
> to get this.
> 
>> +    char *ishare_key;
>>   };
>>   #define EROFS_SB(sb) ((struct erofs_sb_info *)(sb)->s_fs_info)
>> @@ -236,6 +241,7 @@ EROFS_FEATURE_FUNCS(dedupe, incompat, 
>> INCOMPAT_DEDUPE)
>>   EROFS_FEATURE_FUNCS(xattr_prefixes, incompat, INCOMPAT_XATTR_PREFIXES)
>>   EROFS_FEATURE_FUNCS(48bit, incompat, INCOMPAT_48BIT)
>>   EROFS_FEATURE_FUNCS(metabox, incompat, INCOMPAT_METABOX)
>> +EROFS_FEATURE_FUNCS(ishare_key, incompat, INCOMPAT_ISHARE_KEY)
>>   EROFS_FEATURE_FUNCS(sb_chksum, compat, COMPAT_SB_CHKSUM)
>>   EROFS_FEATURE_FUNCS(xattr_filter, compat, COMPAT_XATTR_FILTER)
>>   EROFS_FEATURE_FUNCS(shared_ea_in_metabox, compat, 
>> COMPAT_SHARED_EA_IN_METABOX)
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 0d88c04684b9..3561473cb789 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -339,7 +339,7 @@ static int erofs_read_superblock(struct 
>> super_block *sb)
>>               return -EFSCORRUPTED;    /* self-loop detection */
>>       }
>>       sbi->inos = le64_to_cpu(dsb->inos);
>> -
>> +    sbi->ishare_key_start = dsb->ishare_key_start;
>>       sbi->epoch = (s64)le64_to_cpu(dsb->epoch);
>>       sbi->fixed_nsec = le32_to_cpu(dsb->fixed_nsec);
>>       super_set_uuid(sb, (void *)dsb->uuid, sizeof(dsb->uuid));
>> @@ -738,6 +738,9 @@ static int erofs_fc_fill_super(struct super_block 
>> *sb, struct fs_context *fc)
>>       if (err)
>>           return err;
>> +    err = erofs_xattr_set_ishare_key(sb);
> 
> I don't think it's necessary to duplicate the copy, just use
> "sbi->xattr_prefixes[sbi->ishare_xattr_pfx]" directly.
> 

Thanks for review, but here we should pass the char * to erofs_getxattr 
to obtain the xattr length and value. And xattr_prefixes packed all 
entries together so we cannot tranform 
sbi->xattr_prefixes[sbi->ishare_xattr_pfx] into char * directly.

Thanks,
Hongbo

> Thanks,
> Gao Xiang
> 
>> +    if (err)
>> +        return err;
>>       erofs_set_sysfs_name(sb);
>>       err = erofs_register_sysfs(sb);
>>       if (err)
>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>> index 396536d9a862..3c99091f39a5 100644
>> --- a/fs/erofs/xattr.c
>> +++ b/fs/erofs/xattr.c
>> @@ -564,3 +564,29 @@ struct posix_acl *erofs_get_acl(struct inode 
>> *inode, int type, bool rcu)
>>       return acl;
>>   }
>>   #endif
>> +
>> +#ifdef CONFIG_EROFS_FS_INODE_SHARE
>> +int erofs_xattr_set_ishare_key(struct super_block *sb)
>> +{
>> +    struct erofs_sb_info *sbi = EROFS_SB(sb);
>> +    struct erofs_xattr_prefix_item *pf;
>> +    char *ishare_key;
>> +
>> +    if (!sbi->xattr_prefixes ||
>> +        !(sbi->ishare_key_start & EROFS_XATTR_LONG_PREFIX))
>> +        return 0;
>> +
>> +    pf = sbi->xattr_prefixes +
>> +        (sbi->ishare_key_start & EROFS_XATTR_LONG_PREFIX_MASK);
>> +    if (!pf || pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
>> +        return 0;
>> +    ishare_key = kmalloc(pf->infix_len + 1, GFP_KERNEL);
>> +    if (!ishare_key)
>> +        return -ENOMEM;
>> +    memcpy(ishare_key, pf->prefix->infix, pf->infix_len);
>> +    ishare_key[pf->infix_len] = '\0';
>> +    sbi->ishare_key = ishare_key;
>> +    sbi->ishare_key_idx = pf->prefix->base_index;
>> +    return 0;
>> +}
>> +#endif
>> diff --git a/fs/erofs/xattr.h b/fs/erofs/xattr.h
>> index 6317caa8413e..21684359662c 100644
>> --- a/fs/erofs/xattr.h
>> +++ b/fs/erofs/xattr.h
>> @@ -67,4 +67,10 @@ struct posix_acl *erofs_get_acl(struct inode 
>> *inode, int type, bool rcu);
>>   #define erofs_get_acl    (NULL)
>>   #endif
>> +#ifdef CONFIG_EROFS_FS_INODE_SHARE
>> +int erofs_xattr_set_ishare_key(struct super_block *sb);
>> +#else
>> +static inline int erofs_xattr_set_ishare_key(struct super_block *sb) 
>> { return 0; }
>> +#endif
>> +
>>   #endif
>