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

[Hongzhen Luo]

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@xxxxxxxxxxxxxxxxx>
> > ---
> > v2: Make adjustments based on the latest implementation.
> > v1: 
> > https://lore.kernel.org/all/20240722065355.1396365-5-hongzhen@xxxxxxxxxxxxxxxxx/
> > ---
> >   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,