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

[Chao Yu <chao@kernel.org>]

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@linux.alibaba.com>
>>> ---
>>> v1: https://lore.kernel.org/r/20241017115705.877515-1-hsiangkao@linux.alibaba.com
>>> 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@kernel.org>

Thanks,

> 
> Thanks,
> Gao Xiang