Re: [PATCH] erofs: allow sharing page cache with the same aops only

[Gao Xiang <xiang@kernel.org>]

Hi Hongbo,

On Fri, Feb 13, 2026 at 07:33:45AM +0000, Hongbo Li wrote:
> Inode with identical data but different @aops cannot be mixed
> because the page cache is managed by different subsystems (e.g.,
> @aops for compressed on-disk inodes cannot handle plain on-disk
> inodes).
> 
> In this patch, we never allow inodes to share the page cache
> among plain, compressed, and fileio cases. When a shared inode
> is created, we initialize @aops that is the same as the initial
> real inode, and subsequent inodes cannot share the page cache
> if the inferred @aops differ from the corresponding shared inode.
> 
> This is reasonable as a first step because, in typical use cases,
> if an inode is compressible, it will fall into compressed
> inodes across different filesystem images unless users use plain
> filesystems. However, in that cases, users will use plain
> filesystems all the time.
> 
> Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/erofs/inode.c    |  3 ++-
>  fs/erofs/internal.h | 20 +++++++++-----------
>  fs/erofs/ishare.c   | 14 +++++++++-----
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index 4f86169c23f1..5b05272fd9c4 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -254,7 +254,8 @@ static int erofs_fill_inode(struct inode *inode)
>  	}
>  
>  	mapping_set_large_folios(inode->i_mapping);
> -	return erofs_inode_set_aops(inode, inode, false);
> +	inode->i_mapping->a_ops = erofs_get_aops(inode, false);
> +	return IS_ERR(inode->i_mapping->a_ops) ? PTR_ERR(inode->i_mapping->a_ops) : 0;

I hope there is an aops variable instead of assigning
inode->i_mapping->a_ops directly.

>  }
>  
>  /*
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index d1634455e389..764e81b3bc08 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -471,26 +471,24 @@ static inline void *erofs_vm_map_ram(struct page **pages, unsigned int count)
>  	return NULL;
>  }
>  
> -static inline int erofs_inode_set_aops(struct inode *inode,
> -				       struct inode *realinode, bool no_fscache)
> +static inline const struct address_space_operations *
> +erofs_get_aops(struct inode *realinode, bool no_fscache)
>  {
>  	if (erofs_inode_is_data_compressed(EROFS_I(realinode)->datalayout)) {
>  		if (!IS_ENABLED(CONFIG_EROFS_FS_ZIP))
> -			return -EOPNOTSUPP;
> +			return ERR_PTR(-EOPNOTSUPP);
>  		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;
> -		return 0;
> +		return &z_erofs_aops;
>  	}
> -	inode->i_mapping->a_ops = &erofs_aops;
> -	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && !no_fscache &&
> -	    erofs_is_fscache_mode(realinode->i_sb))
> -		inode->i_mapping->a_ops = &erofs_fscache_access_aops;
>  	if (IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) &&
>  	    erofs_is_fileio_mode(EROFS_SB(realinode->i_sb)))
> -		inode->i_mapping->a_ops = &erofs_fileio_aops;
> -	return 0;
> +		return &erofs_fileio_aops;
> +	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && !no_fscache &&
> +	    erofs_is_fscache_mode(realinode->i_sb))
> +		return &erofs_fscache_access_aops;

Can you rearrange it as
	if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && !no_fscache &&
	    erofs_is_fscache_mode(realinode->i_sb))
		return &erofs_fscache_access_aops;
	if (IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE) &&
	    erofs_is_fileio_mode(EROFS_SB(realinode->i_sb)))
		inode->i_mapping->a_ops = &erofs_fileio_aops;
	return &erofs_aops;

?

Thanks,
Gao Xiang