Re: [RFC PATCH v5 3/4] erofs: apply the page cache share feature

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

On 2025/1/5 23:12, Hongzhen Luo wrote:

...

> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index 0cd6b5c4df98..fb08acbeaab6 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -5,6 +5,7 @@
>    * Copyright (C) 2021, Alibaba Cloud
>    */
>   #include "internal.h"
> +#include "pagecache_share.h"
>   #include <linux/sched/mm.h>
>   #include <trace/events/erofs.h>
>   
> @@ -370,12 +371,21 @@ int erofs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>    */
>   static int erofs_read_folio(struct file *file, struct folio *folio)
>   {
> -	return iomap_read_folio(folio, &erofs_iomap_ops);
> +	int ret, pcshr;
> +
> +	pcshr = erofs_pcshr_read_begin(file, folio);
> +	ret = iomap_read_folio(folio, &erofs_iomap_ops);
> +	erofs_pcshr_read_end(file, folio, pcshr);
> +	return ret;
>   }
>   
>   static void erofs_readahead(struct readahead_control *rac)
>   {
> -	return iomap_readahead(rac, &erofs_iomap_ops);
> +	int pcshr;
> +
> +	pcshr = erofs_pcshr_readahead_begin(rac);
> +	iomap_readahead(rac, &erofs_iomap_ops);
> +	erofs_pcshr_readahead_end(rac, pcshr);
>   }
>   
>   static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index d4b89407822a..0b070f4b46b8 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -5,6 +5,7 @@
>    * Copyright (C) 2021, Alibaba Cloud
>    */
>   #include "xattr.h"
> +#include "pagecache_share.h"
>   #include <trace/events/erofs.h>
>   
>   static int erofs_fill_symlink(struct inode *inode, void *kaddr,
> @@ -212,7 +213,9 @@ static int erofs_fill_inode(struct inode *inode)
>   	switch (inode->i_mode & S_IFMT) {
>   	case S_IFREG:
>   		inode->i_op = &erofs_generic_iops;
> -		if (erofs_inode_is_data_compressed(vi->datalayout))
> +		if (erofs_pcshr_fill_inode(inode) == 0)
> +			inode->i_fop = &erofs_pcshr_fops;
> +		else if (erofs_inode_is_data_compressed(vi->datalayout))
>   			inode->i_fop = &generic_ro_fops;
>   		else
>   			inode->i_fop = &erofs_file_fops;
> diff --git a/fs/erofs/pagecache_share.c b/fs/erofs/pagecache_share.c
> index 703fd17c002c..22172b5e21c7 100644
> --- a/fs/erofs/pagecache_share.c
> +++ b/fs/erofs/pagecache_share.c
> @@ -22,6 +22,7 @@ struct erofs_pcshr_counter {
>   
>   struct erofs_pcshr_private {
>   	char fprt[PCSHR_FPRT_MAXLEN];
> +	struct mutex mutex;
>   };
>   
>   static struct erofs_pcshr_counter mnt_counter = {
> @@ -84,6 +85,7 @@ static int erofs_fprt_set(struct inode *inode, void *data)
>   	if (!ano_private)
>   		return -ENOMEM;
>   	memcpy(ano_private, data, sizeof(size_t) + *(size_t *)data);
> +	mutex_init(&ano_private->mutex);
>   	inode->i_private = ano_private;
>   	return 0;
>   }
> @@ -226,3 +228,64 @@ const struct file_operations erofs_pcshr_fops = {
>   	.get_unmapped_area = thp_get_unmapped_area,
>   	.splice_read	= filemap_splice_read,
>   };
> +
> +int erofs_pcshr_read_begin(struct file *file, struct folio *folio)
> +{
> +	struct erofs_inode *vi;
> +	struct erofs_pcshr_private *ano_private;
> +
> +	if (!(file && file->private_data))
> +		return 0;
> +
> +	vi = file->private_data;
> +	if (vi->ano_inode != file_inode(file))
> +		return 0;
> +
> +	ano_private = vi->ano_inode->i_private;
> +	mutex_lock(&ano_private->mutex);
> +	folio->mapping->host = &vi->vfs_inode;

you shouldn't change `folio->mapping->host` directly.

> +	return 1;
> +}
> +
> +void erofs_pcshr_read_end(struct file *file, struct folio *folio, int pcshr)
> +{
> +	struct erofs_pcshr_private *ano_private;
> +
> +	if (pcshr == 0)
> +		return;
> +
> +	ano_private = file_inode(file)->i_private;
> +	folio->mapping->host = file_inode(file);


you shouldn't change `folio->mapping->host` directly
and then switch back.  It's too hacky.

> +	mutex_unlock(&ano_private->mutex);
> +}
> +
> +int erofs_pcshr_readahead_begin(struct readahead_control *rac)
> +{
> +	struct erofs_inode *vi;
> +	struct file *file = rac->file;
> +	struct erofs_pcshr_private *ano_private;
> +
> +	if (!(file && file->private_data))
> +		return 0;
> +
> +	vi = file->private_data;
> +	if (vi->ano_inode != file_inode(file))
> +		return 0;
> +
> +	ano_private = file_inode(file)->i_private;
> +	mutex_lock(&ano_private->mutex);
> +	rac->mapping->host = &vi->vfs_inode;

Same here.

Thanks,
Gao Xiang