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

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

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.

Thanks,
Gao Xiang