Re: [PATCH] erofs: clean up erofs_ishare_fill_inode()

On 2026/6/8 1:21, Gao Xiang wrote:
>   - Use the shorthand `si` to replace the overly long `sharedinode`;
> 
>   - Introduce erofs_warn() and get rid of barely-used _erofs_printk();
> 
>   - Get rid of the variable `hash`;
> 
>   - Simplify error paths.
> 
> Cc: Hongbo Li <[email protected]>
> Signed-off-by: Gao Xiang <[email protected]>

Reviewed-by: Hongbo Li <[email protected]>

Thanks,
Hongbo

> ---
>   fs/erofs/internal.h |  2 ++
>   fs/erofs/ishare.c   | 45 +++++++++++++++++++--------------------------
>   2 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 4792490161ec..9e2ae7b61977 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -23,6 +23,8 @@
>   __printf(2, 3) void _erofs_printk(struct super_block *sb, const char *fmt, ...);
>   #define erofs_err(sb, fmt, ...)	\
>   	_erofs_printk(sb, KERN_ERR fmt "\n", ##__VA_ARGS__)
> +#define erofs_warn(sb, fmt, ...) \
> +	_erofs_printk(sb, KERN_WARNING fmt "\n", ##__VA_ARGS__)
>   #define erofs_info(sb, fmt, ...) \
>   	_erofs_printk(sb, KERN_INFO fmt "\n", ##__VA_ARGS__)
>   
> diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
> index 6ed66b17359b..35cbd0bc04d7 100644
> --- a/fs/erofs/ishare.c
> +++ b/fs/erofs/ishare.c
> @@ -40,49 +40,42 @@ static int erofs_ishare_iget5_set(struct inode *inode, void *data)
>   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);
>   	const struct address_space_operations *aops;
> +	struct erofs_inode *vi = EROFS_I(inode);
>   	struct erofs_inode_fingerprint fp;
> -	struct inode *sharedinode;
> -	unsigned long hash;
> +	struct inode *si;
>   
>   	aops = erofs_get_aops(inode, true);
>   	if (IS_ERR(aops))
>   		return false;
>   	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;
> -	}
>   
> -	if (inode_state_read_once(sharedinode) & I_NEW) {
> -		sharedinode->i_mapping->a_ops = aops;
> -		sharedinode->i_size = vi->vfs_inode.i_size;
> -		unlock_new_inode(sharedinode);
> +	si = iget5_locked(erofs_ishare_mnt->mnt_sb,
> +			  xxh32(fp.opaque, fp.size, 0),
> +			  erofs_ishare_iget5_eq, erofs_ishare_iget5_set, &fp);
> +	if (si && (inode_state_read_once(si) & I_NEW)) {
> +		si->i_mapping->a_ops = aops;
> +		si->i_size = inode->i_size;
> +		unlock_new_inode(si);
>   	} else {
>   		kfree(fp.opaque);
> -		if (aops != sharedinode->i_mapping->a_ops) {
> -			iput(sharedinode);
> +		if (!si || aops != si->i_mapping->a_ops) {
> +			iput(si);
>   			return false;
>   		}
> -		if (sharedinode->i_size != vi->vfs_inode.i_size) {
> -			_erofs_printk(inode->i_sb, KERN_WARNING
> -				"size(%lld:%lld) not matches for the same fingerprint\n",
> -				vi->vfs_inode.i_size, sharedinode->i_size);
> -			iput(sharedinode);
> +		if (si->i_size != inode->i_size) {
> +			erofs_warn(inode->i_sb, "i_size mismatch (%lld != %lld) for the same fingerprint",
> +				   inode->i_size, si->i_size);
> +			iput(si);
>   			return false;
>   		}
>   	}
> -	vi->sharedinode = sharedinode;
> +	vi->sharedinode = si;
>   	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);
> +	spin_lock(&EROFS_I(si)->ishare_lock);
> +	list_add(&vi->ishare_list, &EROFS_I(si)->ishare_list);
> +	spin_unlock(&EROFS_I(si)->ishare_lock);
>   	return true;
>   }
>