Re: [PATCH RFC v2 2/2] erofs: apply the page cache share feature

[Hongzhen Luo <hongzhen@linux.alibaba.com>]

On 2024/8/1 20:43, Hongbo Li wrote:
>
>
> On 2024/7/31 16:07, Hongzhen Luo wrote:
>> This modifies relevant functions to apply the page cache
>> share feature.
>>
>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>> ---
>> v2: Make adjustments based on the latest implementation.
>> v1: 
>> https://lore.kernel.org/all/20240722065355.1396365-5-hongzhen@linux.alibaba.com/
>> ---
>>   fs/erofs/inode.c | 23 +++++++++++++++++++++++
>>   fs/erofs/super.c | 23 +++++++++++++++++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>> index 5f6439a63af7..9f1e7332cff9 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>
>>   @@ -229,10 +230,22 @@ static int erofs_fill_inode(struct inode *inode)
>>       switch (inode->i_mode & S_IFMT) {
>>       case S_IFREG:
>>           inode->i_op = &erofs_generic_iops;
>> +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +        erofs_pcs_fill_inode(inode);
>> +#endif
>>           if (erofs_inode_is_data_compressed(vi->datalayout))
>>               inode->i_fop = &generic_ro_fops;
>> +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +        else {
> If the compress data is not support, the erofs_pcs_fill_inode should 
> fill the fingerprint in this branch only.
Sure, thanks.
>> +            if (vi->fprt_len > 0)
>> +                inode->i_fop = &erofs_pcs_file_fops;
>> +            else
>> +                inode->i_fop = &erofs_file_fops;
>> +        }
>> +#else
>>           else
>>               inode->i_fop = &erofs_file_fops;
>> +#endif
>>           break;
>>       case S_IFDIR:
>>           inode->i_op = &erofs_dir_iops;
>> @@ -325,6 +338,16 @@ struct inode *erofs_iget(struct super_block *sb, 
>> erofs_nid_t nid)
>>               return ERR_PTR(err);
>>           }
>>           unlock_new_inode(inode);
>> +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +        if ((inode->i_mode & S_IFMT) == S_IFREG &&may be S_ISREG 
>> macro is better.
>
>> +            EROFS_I(inode)->fprt_len > 0) {
> Perhaps this logic need to be enclosed within unlock_new_inode.
Sure, thanks.
>> +            err = erofs_pcs_add(inode);
>> +            if (err) {
>> +                iget_failed(inode);
>> +                return ERR_PTR(err);
>> +            }
>> +        }
>> +#endif
>>       }
>>       return inode;
>>   }
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 35268263aaed..a42e65ef7fc7 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/fs_parser.h>
>>   #include <linux/exportfs.h>
>>   #include "xattr.h"
>> +#include "pagecache_share.h"
>>     #define CREATE_TRACE_POINTS
>>   #include <trace/events/erofs.h>
>> @@ -95,6 +96,10 @@ static struct inode *erofs_alloc_inode(struct 
>> super_block *sb)
>>         /* zero out everything except vfs_inode */
>>       memset(vi, 0, offsetof(struct erofs_inode, vfs_inode));
>> +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +    INIT_LIST_HEAD(&vi->pcs_list);
>> +    init_rwsem(&vi->pcs_rwsem);
>> +#endif
>>       return &vi->vfs_inode;
>>   }
>>   @@ -108,6 +113,21 @@ static void erofs_free_inode(struct inode *inode)
>>       kmem_cache_free(erofs_inode_cachep, vi);
>>   }
>>   +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +static void erofs_destroy_inode(struct inode *inode)
>> +{
>> +    struct erofs_inode *vi = EROFS_I(inode);
>> +
>> +    if ((inode->i_mode & S_IFMT) == S_IFREG &&
> using S_ISREG macro is better.

Thanks.

---

Hongzhen Luo

>> +        EROFS_I(inode)->fprt_len > 0) {
>> +        if (erofs_pcs_remove(inode))
>> +            erofs_err(inode->i_sb, "pcs: fail to remove inode.");
>> +        kfree(vi->fprt);
>> +        vi->fprt = NULL;
>> +    }
>> +}
>> +#endif
>> +
>>   static bool check_layout_compatibility(struct super_block *sb,
>>                          struct erofs_super_block *dsb)
>>   {
>> @@ -937,6 +957,9 @@ static int erofs_show_options(struct seq_file 
>> *seq, struct dentry *root)
>>   const struct super_operations erofs_sops = {
>>       .put_super = erofs_put_super,
>>       .alloc_inode = erofs_alloc_inode,
>> +#ifdef CONFIG_EROFS_FS_PAGE_CACHE_SHARE
>> +    .destroy_inode = erofs_destroy_inode,
>> +#endif
>>       .free_inode = erofs_free_inode,
>>       .statfs = erofs_statfs,
>>       .show_options = erofs_show_options,