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

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

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,
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