Re: [PATCH v2 1/3] erofs: get rid of erofs_{find,insert}_workgroup

[Chao Yu]

On 2024/11/11 10:12, Gao Xiang wrote:
> Hi Chao,
>
> On 2024/11/7 11:09, Chao Yu wrote:
> > On 2024/10/21 11:53, Gao Xiang wrote:
> > > Just fold them into the only two callers since
> > > they are simple enough.
> > >
> > > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> > > ---
> > > v1: https://lore.kernel.org/r/20241017115705.877515-1-hsiangkao@xxxxxxxxxxxxxxxxx
> > > change since v1:
> > >   - !grp case should be handled properly mentioned by Chunhai;
> > >
> > >   fs/erofs/internal.h |  5 +----
> > >   fs/erofs/zdata.c    | 38 +++++++++++++++++++++++++---------
> > >   fs/erofs/zutil.c    | 50 +--------------------------------------------
> > >   3 files changed, 30 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > > index 4efd578d7c62..8081ee43cd83 100644
> > > --- a/fs/erofs/internal.h
> > > +++ b/fs/erofs/internal.h
> > > @@ -457,10 +457,7 @@ void erofs_release_pages(struct page **pagepool);
> > >   #ifdef CONFIG_EROFS_FS_ZIP
> > >   void erofs_workgroup_put(struct erofs_workgroup *grp);
> > > -struct erofs_workgroup *erofs_find_workgroup(struct super_block *sb,
> > > -                         pgoff_t index);
> > > -struct erofs_workgroup *erofs_insert_workgroup(struct super_block *sb,
> > > -                           struct erofs_workgroup *grp);
> > > +bool erofs_workgroup_get(struct erofs_workgroup *grp);
> > >   void erofs_workgroup_free_rcu(struct erofs_workgroup *grp);
> > >   void erofs_shrinker_register(struct super_block *sb);
> > >   void erofs_shrinker_unregister(struct super_block *sb);
> > > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> > > index a569ff9dfd04..bb1b73d99d07 100644
> > > --- a/fs/erofs/zdata.c
> > > +++ b/fs/erofs/zdata.c
> > > @@ -714,9 +714,10 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
> > >   {
> > >       struct erofs_map_blocks *map = &fe->map;
> > >       struct super_block *sb = fe->inode->i_sb;
> > > +    struct erofs_sb_info *sbi = EROFS_SB(sb);
> > >       bool ztailpacking = map->m_flags & EROFS_MAP_META;
> > >       struct z_erofs_pcluster *pcl;
> > > -    struct erofs_workgroup *grp;
> > > +    struct erofs_workgroup *grp, *pre;
> > >       int err;
> > >       if (!(map->m_flags & EROFS_MAP_ENCODED) ||
> > > @@ -752,15 +753,23 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
> > >           pcl->obj.index = 0;    /* which indicates ztailpacking */
> > >       } else {
> > >           pcl->obj.index = erofs_blknr(sb, map->m_pa);
> > > -
> > > -        grp = erofs_insert_workgroup(fe->inode->i_sb, &pcl->obj);
> > > -        if (IS_ERR(grp)) {
> > > -            err = PTR_ERR(grp);
> > > -            goto err_out;
> > > +        while (1) {
> > > +            xa_lock(&sbi->managed_pslots);
> > > +            pre = __xa_cmpxchg(&sbi->managed_pslots, grp->index,
> > > +                       NULL, grp, GFP_KERNEL);
> > > +            if (!pre || xa_is_err(pre) || erofs_workgroup_get(pre)) {
> > > +                xa_unlock(&sbi->managed_pslots);
> > > +                break;
> > > +            }
> > > +            /* try to legitimize the current in-tree one */
> > > +            xa_unlock(&sbi->managed_pslots);
> > > +            cond_resched();
> > >           }
> > > -
> > > -        if (grp != &pcl->obj) {
> >
> > Do we need to keep this logic?
>
> Thanks for the review.  I think
>
>          if (grp != &pcl->obj)
>
> equals to (pre && erofs_workgroup_get(pre)) here, so
>
>          } else if (pre) {
>              fe->pcl = container_of(pre,
>                  struct z_erofs_pcluster, obj);
>              err = -EEXIST;
>              goto err_out;
>          }
>
> Handles this case.

Xiang, thanks for your explanation.

Reviewed-by: Chao Yu <chao@xxxxxxxxxx>

Thanks,

> Thanks,
> Gao Xiang