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

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

[Gao Xiang]

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) {
-			fe->pcl = container_of(grp,
+		if (xa_is_err(pre)) {
+			err = xa_err(pre);
+			goto err_out;
+		} else if (pre) {
+			fe->pcl = container_of(pre,
 					struct z_erofs_pcluster, obj);
 			err = -EEXIST;
 			goto err_out;
@@ -789,7 +798,16 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
 	DBG_BUGON(fe->owned_head == Z_EROFS_PCLUSTER_NIL);
 
 	if (!(map->m_flags & EROFS_MAP_META)) {
-		grp = erofs_find_workgroup(sb, blknr);
+		while (1) {
+			rcu_read_lock();
+			grp = xa_load(&EROFS_SB(sb)->managed_pslots, blknr);
+			if (!grp || erofs_workgroup_get(grp)) {
+				DBG_BUGON(grp && blknr != grp->index);
+				rcu_read_unlock();
+				break;
+			}
+			rcu_read_unlock();
+		}
 	} else if ((map->m_pa & ~PAGE_MASK) + map->m_plen > PAGE_SIZE) {
 		DBG_BUGON(1);
 		return -EFSCORRUPTED;
diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
index 37afe2024840..218b0249a482 100644
--- a/fs/erofs/zutil.c
+++ b/fs/erofs/zutil.c
@@ -214,7 +214,7 @@ void erofs_release_pages(struct page **pagepool)
 	}
 }
 
-static bool erofs_workgroup_get(struct erofs_workgroup *grp)
+bool erofs_workgroup_get(struct erofs_workgroup *grp)
 {
 	if (lockref_get_not_zero(&grp->lockref))
 		return true;
@@ -231,54 +231,6 @@ static bool erofs_workgroup_get(struct erofs_workgroup *grp)
 	return true;
 }
 
-struct erofs_workgroup *erofs_find_workgroup(struct super_block *sb,
-					     pgoff_t index)
-{
-	struct erofs_sb_info *sbi = EROFS_SB(sb);
-	struct erofs_workgroup *grp;
-
-repeat:
-	rcu_read_lock();
-	grp = xa_load(&sbi->managed_pslots, index);
-	if (grp) {
-		if (!erofs_workgroup_get(grp)) {
-			/* prefer to relax rcu read side */
-			rcu_read_unlock();
-			goto repeat;
-		}
-
-		DBG_BUGON(index != grp->index);
-	}
-	rcu_read_unlock();
-	return grp;
-}
-
-struct erofs_workgroup *erofs_insert_workgroup(struct super_block *sb,
-					       struct erofs_workgroup *grp)
-{
-	struct erofs_sb_info *const sbi = EROFS_SB(sb);
-	struct erofs_workgroup *pre;
-
-	DBG_BUGON(grp->lockref.count < 1);
-repeat:
-	xa_lock(&sbi->managed_pslots);
-	pre = __xa_cmpxchg(&sbi->managed_pslots, grp->index,
-			   NULL, grp, GFP_KERNEL);
-	if (pre) {
-		if (xa_is_err(pre)) {
-			pre = ERR_PTR(xa_err(pre));
-		} else if (!erofs_workgroup_get(pre)) {
-			/* try to legitimize the current in-tree one */
-			xa_unlock(&sbi->managed_pslots);
-			cond_resched();
-			goto repeat;
-		}
-		grp = pre;
-	}
-	xa_unlock(&sbi->managed_pslots);
-	return grp;
-}
-
 static void  __erofs_workgroup_free(struct erofs_workgroup *grp)
 {
 	atomic_long_dec(&erofs_global_shrink_cnt);
-- 
2.43.5

[PATCH v2 2/3] erofs: move erofs_workgroup operations into zdata.c

[Gao Xiang]

Move related helpers into zdata.c as an intermediate step of getting
rid of `struct erofs_workgroup`, and rename:

 erofs_workgroup_put => z_erofs_put_pcluster
 erofs_workgroup_get => z_erofs_get_pcluster
 erofs_try_to_release_workgroup => erofs_try_to_release_pcluster
 erofs_shrink_workstation => z_erofs_shrink_scan

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/internal.h |   8 ++--
 fs/erofs/zdata.c    | 102 ++++++++++++++++++++++++++++++++++++++---
 fs/erofs/zutil.c    | 107 +++-----------------------------------------
 3 files changed, 105 insertions(+), 112 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 8081ee43cd83..5fa7ac0575b2 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -456,17 +456,15 @@ static inline void erofs_pagepool_add(struct page **pagepool, struct page *page)
 void erofs_release_pages(struct page **pagepool);
 
 #ifdef CONFIG_EROFS_FS_ZIP
-void erofs_workgroup_put(struct erofs_workgroup *grp);
-bool erofs_workgroup_get(struct erofs_workgroup *grp);
-void erofs_workgroup_free_rcu(struct erofs_workgroup *grp);
+extern atomic_long_t erofs_global_shrink_cnt;
 void erofs_shrinker_register(struct super_block *sb);
 void erofs_shrinker_unregister(struct super_block *sb);
 int __init erofs_init_shrinker(void);
 void erofs_exit_shrinker(void);
 int __init z_erofs_init_subsystem(void);
 void z_erofs_exit_subsystem(void);
-int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi,
-					struct erofs_workgroup *egrp);
+unsigned long z_erofs_shrink_scan(struct erofs_sb_info *sbi,
+				  unsigned long nr_shrink);
 int z_erofs_map_blocks_iter(struct inode *inode, struct erofs_map_blocks *map,
 			    int flags);
 void *z_erofs_get_gbuf(unsigned int requiredpages);
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index bb1b73d99d07..578e6f3612a7 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -587,8 +587,8 @@ static void z_erofs_bind_cache(struct z_erofs_decompress_frontend *fe)
 }
 
 /* (erofs_shrinker) disconnect cached encoded data with pclusters */
-int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi,
-					struct erofs_workgroup *grp)
+static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi,
+					       struct erofs_workgroup *grp)
 {
 	struct z_erofs_pcluster *const pcl =
 		container_of(grp, struct z_erofs_pcluster, obj);
@@ -710,6 +710,23 @@ static int z_erofs_attach_page(struct z_erofs_decompress_frontend *fe,
 	return ret;
 }
 
+static bool z_erofs_get_pcluster(struct erofs_workgroup *grp)
+{
+	if (lockref_get_not_zero(&grp->lockref))
+		return true;
+
+	spin_lock(&grp->lockref.lock);
+	if (__lockref_is_dead(&grp->lockref)) {
+		spin_unlock(&grp->lockref.lock);
+		return false;
+	}
+
+	if (!grp->lockref.count++)
+		atomic_long_dec(&erofs_global_shrink_cnt);
+	spin_unlock(&grp->lockref.lock);
+	return true;
+}
+
 static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 {
 	struct erofs_map_blocks *map = &fe->map;
@@ -757,7 +774,7 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 			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)) {
+			if (!pre || xa_is_err(pre) || z_erofs_get_pcluster(pre)) {
 				xa_unlock(&sbi->managed_pslots);
 				break;
 			}
@@ -801,7 +818,7 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
 		while (1) {
 			rcu_read_lock();
 			grp = xa_load(&EROFS_SB(sb)->managed_pslots, blknr);
-			if (!grp || erofs_workgroup_get(grp)) {
+			if (!grp || z_erofs_get_pcluster(grp)) {
 				DBG_BUGON(grp && blknr != grp->index);
 				rcu_read_unlock();
 				break;
@@ -869,7 +886,7 @@ static void z_erofs_rcu_callback(struct rcu_head *head)
 			struct z_erofs_pcluster, rcu));
 }
 
-void erofs_workgroup_free_rcu(struct erofs_workgroup *grp)
+static void erofs_workgroup_free_rcu(struct erofs_workgroup *grp)
 {
 	struct z_erofs_pcluster *const pcl =
 		container_of(grp, struct z_erofs_pcluster, obj);
@@ -877,6 +894,77 @@ void erofs_workgroup_free_rcu(struct erofs_workgroup *grp)
 	call_rcu(&pcl->rcu, z_erofs_rcu_callback);
 }
 
+static bool erofs_try_to_release_pcluster(struct erofs_sb_info *sbi,
+					  struct erofs_workgroup *grp)
+{
+	int free = false;
+
+	spin_lock(&grp->lockref.lock);
+	if (grp->lockref.count)
+		goto out;
+
+	/*
+	 * Note that all cached folios should be detached before deleted from
+	 * the XArray.  Otherwise some folios could be still attached to the
+	 * orphan old pcluster when the new one is available in the tree.
+	 */
+	if (erofs_try_to_free_all_cached_folios(sbi, grp))
+		goto out;
+
+	/*
+	 * It's impossible to fail after the pcluster is freezed, but in order
+	 * to avoid some race conditions, add a DBG_BUGON to observe this.
+	 */
+	DBG_BUGON(__xa_erase(&sbi->managed_pslots, grp->index) != grp);
+
+	lockref_mark_dead(&grp->lockref);
+	free = true;
+out:
+	spin_unlock(&grp->lockref.lock);
+	if (free) {
+		atomic_long_dec(&erofs_global_shrink_cnt);
+		erofs_workgroup_free_rcu(grp);
+	}
+	return free;
+}
+
+unsigned long z_erofs_shrink_scan(struct erofs_sb_info *sbi,
+				  unsigned long nr_shrink)
+{
+	struct erofs_workgroup *grp;
+	unsigned int freed = 0;
+	unsigned long index;
+
+	xa_lock(&sbi->managed_pslots);
+	xa_for_each(&sbi->managed_pslots, index, grp) {
+		/* try to shrink each valid pcluster */
+		if (!erofs_try_to_release_pcluster(sbi, grp))
+			continue;
+		xa_unlock(&sbi->managed_pslots);
+
+		++freed;
+		if (!--nr_shrink)
+			return freed;
+		xa_lock(&sbi->managed_pslots);
+	}
+	xa_unlock(&sbi->managed_pslots);
+	return freed;
+}
+
+static void z_erofs_put_pcluster(struct z_erofs_pcluster *pcl)
+{
+	struct erofs_workgroup *grp = &pcl->obj;
+
+	if (lockref_put_or_lock(&grp->lockref))
+		return;
+
+	DBG_BUGON(__lockref_is_dead(&grp->lockref));
+	if (grp->lockref.count == 1)
+		atomic_long_inc(&erofs_global_shrink_cnt);
+	--grp->lockref.count;
+	spin_unlock(&grp->lockref.lock);
+}
+
 static void z_erofs_pcluster_end(struct z_erofs_decompress_frontend *fe)
 {
 	struct z_erofs_pcluster *pcl = fe->pcl;
@@ -895,7 +983,7 @@ static void z_erofs_pcluster_end(struct z_erofs_decompress_frontend *fe)
 	 * any longer if the pcluster isn't hosted by ourselves.
 	 */
 	if (fe->mode < Z_EROFS_PCLUSTER_FOLLOWED_NOINPLACE)
-		erofs_workgroup_put(&pcl->obj);
+		z_erofs_put_pcluster(pcl);
 
 	fe->pcl = NULL;
 }
@@ -1327,7 +1415,7 @@ static int z_erofs_decompress_queue(const struct z_erofs_decompressqueue *io,
 		if (z_erofs_is_inline_pcluster(be.pcl))
 			z_erofs_free_pcluster(be.pcl);
 		else
-			erofs_workgroup_put(&be.pcl->obj);
+			z_erofs_put_pcluster(be.pcl);
 	}
 	return err;
 }
diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
index 218b0249a482..75704f58ecfa 100644
--- a/fs/erofs/zutil.c
+++ b/fs/erofs/zutil.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2018 HUAWEI, Inc.
  *             https://www.huawei.com/
+ * Copyright (C) 2024 Alibaba Cloud
  */
 #include "internal.h"
 
@@ -19,13 +20,12 @@ static unsigned int z_erofs_gbuf_count, z_erofs_gbuf_nrpages,
 module_param_named(global_buffers, z_erofs_gbuf_count, uint, 0444);
 module_param_named(reserved_pages, z_erofs_rsv_nrpages, uint, 0444);
 
-static atomic_long_t erofs_global_shrink_cnt;	/* for all mounted instances */
-/* protected by 'erofs_sb_list_lock' */
-static unsigned int shrinker_run_no;
+atomic_long_t erofs_global_shrink_cnt;	/* for all mounted instances */
 
-/* protects the mounted 'erofs_sb_list' */
+/* protects `erofs_sb_list_lock` and the mounted `erofs_sb_list` */
 static DEFINE_SPINLOCK(erofs_sb_list_lock);
 static LIST_HEAD(erofs_sb_list);
+static unsigned int shrinker_run_no;
 static struct shrinker *erofs_shrinker_info;
 
 static unsigned int z_erofs_gbuf_id(void)
@@ -214,97 +214,6 @@ void erofs_release_pages(struct page **pagepool)
 	}
 }
 
-bool erofs_workgroup_get(struct erofs_workgroup *grp)
-{
-	if (lockref_get_not_zero(&grp->lockref))
-		return true;
-
-	spin_lock(&grp->lockref.lock);
-	if (__lockref_is_dead(&grp->lockref)) {
-		spin_unlock(&grp->lockref.lock);
-		return false;
-	}
-
-	if (!grp->lockref.count++)
-		atomic_long_dec(&erofs_global_shrink_cnt);
-	spin_unlock(&grp->lockref.lock);
-	return true;
-}
-
-static void  __erofs_workgroup_free(struct erofs_workgroup *grp)
-{
-	atomic_long_dec(&erofs_global_shrink_cnt);
-	erofs_workgroup_free_rcu(grp);
-}
-
-void erofs_workgroup_put(struct erofs_workgroup *grp)
-{
-	if (lockref_put_or_lock(&grp->lockref))
-		return;
-
-	DBG_BUGON(__lockref_is_dead(&grp->lockref));
-	if (grp->lockref.count == 1)
-		atomic_long_inc(&erofs_global_shrink_cnt);
-	--grp->lockref.count;
-	spin_unlock(&grp->lockref.lock);
-}
-
-static bool erofs_try_to_release_workgroup(struct erofs_sb_info *sbi,
-					   struct erofs_workgroup *grp)
-{
-	int free = false;
-
-	spin_lock(&grp->lockref.lock);
-	if (grp->lockref.count)
-		goto out;
-
-	/*
-	 * Note that all cached pages should be detached before deleted from
-	 * the XArray. Otherwise some cached pages could be still attached to
-	 * the orphan old workgroup when the new one is available in the tree.
-	 */
-	if (erofs_try_to_free_all_cached_folios(sbi, grp))
-		goto out;
-
-	/*
-	 * It's impossible to fail after the workgroup is freezed,
-	 * however in order to avoid some race conditions, add a
-	 * DBG_BUGON to observe this in advance.
-	 */
-	DBG_BUGON(__xa_erase(&sbi->managed_pslots, grp->index) != grp);
-
-	lockref_mark_dead(&grp->lockref);
-	free = true;
-out:
-	spin_unlock(&grp->lockref.lock);
-	if (free)
-		__erofs_workgroup_free(grp);
-	return free;
-}
-
-static unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
-					      unsigned long nr_shrink)
-{
-	struct erofs_workgroup *grp;
-	unsigned int freed = 0;
-	unsigned long index;
-
-	xa_lock(&sbi->managed_pslots);
-	xa_for_each(&sbi->managed_pslots, index, grp) {
-		/* try to shrink each valid workgroup */
-		if (!erofs_try_to_release_workgroup(sbi, grp))
-			continue;
-		xa_unlock(&sbi->managed_pslots);
-
-		++freed;
-		if (!--nr_shrink)
-			return freed;
-		xa_lock(&sbi->managed_pslots);
-	}
-	xa_unlock(&sbi->managed_pslots);
-	return freed;
-}
-
 void erofs_shrinker_register(struct super_block *sb)
 {
 	struct erofs_sb_info *sbi = EROFS_SB(sb);
@@ -321,8 +230,8 @@ void erofs_shrinker_unregister(struct super_block *sb)
 	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 
 	mutex_lock(&sbi->umount_mutex);
-	/* clean up all remaining workgroups in memory */
-	erofs_shrink_workstation(sbi, ~0UL);
+	/* clean up all remaining pclusters in memory */
+	z_erofs_shrink_scan(sbi, ~0UL);
 
 	spin_lock(&erofs_sb_list_lock);
 	list_del(&sbi->list);
@@ -370,9 +279,7 @@ static unsigned long erofs_shrink_scan(struct shrinker *shrink,
 
 		spin_unlock(&erofs_sb_list_lock);
 		sbi->shrinker_run_no = run_no;
-
-		freed += erofs_shrink_workstation(sbi, nr - freed);
-
+		freed += z_erofs_shrink_scan(sbi, nr - freed);
 		spin_lock(&erofs_sb_list_lock);
 		/* Get the next list element before we move this one */
 		p = p->next;
-- 
2.43.5

[PATCH v2 3/3] erofs: sunset `struct erofs_workgroup`

[Gao Xiang]

`struct erofs_workgroup` was introduced to provide a unique header
for all physically indexed objects.  However, after big pclusters and
shared pclusters are implemented upstream, it seems that all EROFS
encoded data (which requires transformation) can be represented with
`struct z_erofs_pcluster` directly.

Move all members into `struct z_erofs_pcluster` for simplicity.

Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/internal.h |   6 --
 fs/erofs/zdata.c    | 131 ++++++++++++++++++++------------------------
 2 files changed, 60 insertions(+), 77 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 5fa7ac0575b2..3905d991c49b 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -208,12 +208,6 @@ enum {
 	EROFS_ZIP_CACHE_READAROUND
 };
 
-/* basic unit of the workstation of a super_block */
-struct erofs_workgroup {
-	pgoff_t index;
-	struct lockref lockref;
-};
-
 enum erofs_kmap_type {
 	EROFS_NO_KMAP,		/* don't map the buffer */
 	EROFS_KMAP,		/* use kmap_local_page() to map the buffer */
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 578e6f3612a7..6b73a2307460 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -44,12 +44,15 @@ __Z_EROFS_BVSET(z_erofs_bvset_inline, Z_EROFS_INLINE_BVECS);
  * A: Field should be accessed / updated in atomic for parallelized code.
  */
 struct z_erofs_pcluster {
-	struct erofs_workgroup obj;
 	struct mutex lock;
+	struct lockref lockref;
 
 	/* A: point to next chained pcluster or TAILs */
 	z_erofs_next_pcluster_t next;
 
+	/* I: start block address of this pcluster */
+	erofs_off_t index;
+
 	/* L: the maximum decompression size of this round */
 	unsigned int length;
 
@@ -108,7 +111,7 @@ struct z_erofs_decompressqueue {
 
 static inline bool z_erofs_is_inline_pcluster(struct z_erofs_pcluster *pcl)
 {
-	return !pcl->obj.index;
+	return !pcl->index;
 }
 
 static inline unsigned int z_erofs_pclusterpages(struct z_erofs_pcluster *pcl)
@@ -548,7 +551,7 @@ static void z_erofs_bind_cache(struct z_erofs_decompress_frontend *fe)
 		if (READ_ONCE(pcl->compressed_bvecs[i].page))
 			continue;
 
-		page = find_get_page(mc, pcl->obj.index + i);
+		page = find_get_page(mc, pcl->index + i);
 		if (!page) {
 			/* I/O is needed, no possible to decompress directly */
 			standalone = false;
@@ -564,13 +567,13 @@ static void z_erofs_bind_cache(struct z_erofs_decompress_frontend *fe)
 				continue;
 			set_page_private(newpage, Z_EROFS_PREALLOCATED_PAGE);
 		}
-		spin_lock(&pcl->obj.lockref.lock);
+		spin_lock(&pcl->lockref.lock);
 		if (!pcl->compressed_bvecs[i].page) {
 			pcl->compressed_bvecs[i].page = page ? page : newpage;
-			spin_unlock(&pcl->obj.lockref.lock);
+			spin_unlock(&pcl->lockref.lock);
 			continue;
 		}
-		spin_unlock(&pcl->obj.lockref.lock);
+		spin_unlock(&pcl->lockref.lock);
 
 		if (page)
 			put_page(page);
@@ -588,10 +591,8 @@ static void z_erofs_bind_cache(struct z_erofs_decompress_frontend *fe)
 
 /* (erofs_shrinker) disconnect cached encoded data with pclusters */
 static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi,
-					       struct erofs_workgroup *grp)
+					       struct z_erofs_pcluster *pcl)
 {
-	struct z_erofs_pcluster *const pcl =
-		container_of(grp, struct z_erofs_pcluster, obj);
 	unsigned int pclusterpages = z_erofs_pclusterpages(pcl);
 	struct folio *folio;
 	int i;
@@ -626,8 +627,8 @@ static bool z_erofs_cache_release_folio(struct folio *folio, gfp_t gfp)
 		return true;
 
 	ret = false;
-	spin_lock(&pcl->obj.lockref.lock);
-	if (pcl->obj.lockref.count <= 0) {
+	spin_lock(&pcl->lockref.lock);
+	if (pcl->lockref.count <= 0) {
 		DBG_BUGON(z_erofs_is_inline_pcluster(pcl));
 		for (; bvec < end; ++bvec) {
 			if (bvec->page && page_folio(bvec->page) == folio) {
@@ -638,7 +639,7 @@ static bool z_erofs_cache_release_folio(struct folio *folio, gfp_t gfp)
 			}
 		}
 	}
-	spin_unlock(&pcl->obj.lockref.lock);
+	spin_unlock(&pcl->lockref.lock);
 	return ret;
 }
 
@@ -689,15 +690,15 @@ static int z_erofs_attach_page(struct z_erofs_decompress_frontend *fe,
 
 	if (exclusive) {
 		/* give priority for inplaceio to use file pages first */
-		spin_lock(&pcl->obj.lockref.lock);
+		spin_lock(&pcl->lockref.lock);
 		while (fe->icur > 0) {
 			if (pcl->compressed_bvecs[--fe->icur].page)
 				continue;
 			pcl->compressed_bvecs[fe->icur] = *bvec;
-			spin_unlock(&pcl->obj.lockref.lock);
+			spin_unlock(&pcl->lockref.lock);
 			return 0;
 		}
-		spin_unlock(&pcl->obj.lockref.lock);
+		spin_unlock(&pcl->lockref.lock);
 
 		/* otherwise, check if it can be used as a bvpage */
 		if (fe->mode >= Z_EROFS_PCLUSTER_FOLLOWED &&
@@ -710,20 +711,20 @@ static int z_erofs_attach_page(struct z_erofs_decompress_frontend *fe,
 	return ret;
 }
 
-static bool z_erofs_get_pcluster(struct erofs_workgroup *grp)
+static bool z_erofs_get_pcluster(struct z_erofs_pcluster *pcl)
 {
-	if (lockref_get_not_zero(&grp->lockref))
+	if (lockref_get_not_zero(&pcl->lockref))
 		return true;
 
-	spin_lock(&grp->lockref.lock);
-	if (__lockref_is_dead(&grp->lockref)) {
-		spin_unlock(&grp->lockref.lock);
+	spin_lock(&pcl->lockref.lock);
+	if (__lockref_is_dead(&pcl->lockref)) {
+		spin_unlock(&pcl->lockref.lock);
 		return false;
 	}
 
-	if (!grp->lockref.count++)
+	if (!pcl->lockref.count++)
 		atomic_long_dec(&erofs_global_shrink_cnt);
-	spin_unlock(&grp->lockref.lock);
+	spin_unlock(&pcl->lockref.lock);
 	return true;
 }
 
@@ -733,8 +734,7 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 	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, *pre;
+	struct z_erofs_pcluster *pcl, *pre;
 	int err;
 
 	if (!(map->m_flags & EROFS_MAP_ENCODED) ||
@@ -748,8 +748,8 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 	if (IS_ERR(pcl))
 		return PTR_ERR(pcl);
 
-	spin_lock_init(&pcl->obj.lockref.lock);
-	pcl->obj.lockref.count = 1;	/* one ref for this request */
+	spin_lock_init(&pcl->lockref.lock);
+	pcl->lockref.count = 1;		/* one ref for this request */
 	pcl->algorithmformat = map->m_algorithmformat;
 	pcl->length = 0;
 	pcl->partial = true;
@@ -767,13 +767,13 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 	DBG_BUGON(!mutex_trylock(&pcl->lock));
 
 	if (ztailpacking) {
-		pcl->obj.index = 0;	/* which indicates ztailpacking */
+		pcl->index = 0;		/* which indicates ztailpacking */
 	} else {
-		pcl->obj.index = erofs_blknr(sb, map->m_pa);
+		pcl->index = erofs_blknr(sb, map->m_pa);
 		while (1) {
 			xa_lock(&sbi->managed_pslots);
-			pre = __xa_cmpxchg(&sbi->managed_pslots, grp->index,
-					   NULL, grp, GFP_KERNEL);
+			pre = __xa_cmpxchg(&sbi->managed_pslots, pcl->index,
+					   NULL, pcl, GFP_KERNEL);
 			if (!pre || xa_is_err(pre) || z_erofs_get_pcluster(pre)) {
 				xa_unlock(&sbi->managed_pslots);
 				break;
@@ -786,8 +786,7 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 			err = xa_err(pre);
 			goto err_out;
 		} else if (pre) {
-			fe->pcl = container_of(pre,
-					struct z_erofs_pcluster, obj);
+			fe->pcl = pre;
 			err = -EEXIST;
 			goto err_out;
 		}
@@ -807,7 +806,7 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
 	struct erofs_map_blocks *map = &fe->map;
 	struct super_block *sb = fe->inode->i_sb;
 	erofs_blk_t blknr = erofs_blknr(sb, map->m_pa);
-	struct erofs_workgroup *grp = NULL;
+	struct z_erofs_pcluster *pcl = NULL;
 	int ret;
 
 	DBG_BUGON(fe->pcl);
@@ -817,9 +816,9 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
 	if (!(map->m_flags & EROFS_MAP_META)) {
 		while (1) {
 			rcu_read_lock();
-			grp = xa_load(&EROFS_SB(sb)->managed_pslots, blknr);
-			if (!grp || z_erofs_get_pcluster(grp)) {
-				DBG_BUGON(grp && blknr != grp->index);
+			pcl = xa_load(&EROFS_SB(sb)->managed_pslots, blknr);
+			if (!pcl || z_erofs_get_pcluster(pcl)) {
+				DBG_BUGON(pcl && blknr != pcl->index);
 				rcu_read_unlock();
 				break;
 			}
@@ -830,8 +829,8 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
 		return -EFSCORRUPTED;
 	}
 
-	if (grp) {
-		fe->pcl = container_of(grp, struct z_erofs_pcluster, obj);
+	if (pcl) {
+		fe->pcl = pcl;
 		ret = -EEXIST;
 	} else {
 		ret = z_erofs_register_pcluster(fe);
@@ -886,21 +885,13 @@ static void z_erofs_rcu_callback(struct rcu_head *head)
 			struct z_erofs_pcluster, rcu));
 }
 
-static void erofs_workgroup_free_rcu(struct erofs_workgroup *grp)
-{
-	struct z_erofs_pcluster *const pcl =
-		container_of(grp, struct z_erofs_pcluster, obj);
-
-	call_rcu(&pcl->rcu, z_erofs_rcu_callback);
-}
-
 static bool erofs_try_to_release_pcluster(struct erofs_sb_info *sbi,
-					  struct erofs_workgroup *grp)
+					  struct z_erofs_pcluster *pcl)
 {
 	int free = false;
 
-	spin_lock(&grp->lockref.lock);
-	if (grp->lockref.count)
+	spin_lock(&pcl->lockref.lock);
+	if (pcl->lockref.count)
 		goto out;
 
 	/*
@@ -908,22 +899,22 @@ static bool erofs_try_to_release_pcluster(struct erofs_sb_info *sbi,
 	 * the XArray.  Otherwise some folios could be still attached to the
 	 * orphan old pcluster when the new one is available in the tree.
 	 */
-	if (erofs_try_to_free_all_cached_folios(sbi, grp))
+	if (erofs_try_to_free_all_cached_folios(sbi, pcl))
 		goto out;
 
 	/*
 	 * It's impossible to fail after the pcluster is freezed, but in order
 	 * to avoid some race conditions, add a DBG_BUGON to observe this.
 	 */
-	DBG_BUGON(__xa_erase(&sbi->managed_pslots, grp->index) != grp);
+	DBG_BUGON(__xa_erase(&sbi->managed_pslots, pcl->index) != pcl);
 
-	lockref_mark_dead(&grp->lockref);
+	lockref_mark_dead(&pcl->lockref);
 	free = true;
 out:
-	spin_unlock(&grp->lockref.lock);
+	spin_unlock(&pcl->lockref.lock);
 	if (free) {
 		atomic_long_dec(&erofs_global_shrink_cnt);
-		erofs_workgroup_free_rcu(grp);
+		call_rcu(&pcl->rcu, z_erofs_rcu_callback);
 	}
 	return free;
 }
@@ -931,14 +922,14 @@ static bool erofs_try_to_release_pcluster(struct erofs_sb_info *sbi,
 unsigned long z_erofs_shrink_scan(struct erofs_sb_info *sbi,
 				  unsigned long nr_shrink)
 {
-	struct erofs_workgroup *grp;
+	struct z_erofs_pcluster *pcl;
 	unsigned int freed = 0;
 	unsigned long index;
 
 	xa_lock(&sbi->managed_pslots);
-	xa_for_each(&sbi->managed_pslots, index, grp) {
+	xa_for_each(&sbi->managed_pslots, index, pcl) {
 		/* try to shrink each valid pcluster */
-		if (!erofs_try_to_release_pcluster(sbi, grp))
+		if (!erofs_try_to_release_pcluster(sbi, pcl))
 			continue;
 		xa_unlock(&sbi->managed_pslots);
 
@@ -953,16 +944,14 @@ unsigned long z_erofs_shrink_scan(struct erofs_sb_info *sbi,
 
 static void z_erofs_put_pcluster(struct z_erofs_pcluster *pcl)
 {
-	struct erofs_workgroup *grp = &pcl->obj;
-
-	if (lockref_put_or_lock(&grp->lockref))
+	if (lockref_put_or_lock(&pcl->lockref))
 		return;
 
-	DBG_BUGON(__lockref_is_dead(&grp->lockref));
-	if (grp->lockref.count == 1)
+	DBG_BUGON(__lockref_is_dead(&pcl->lockref));
+	if (pcl->lockref.count == 1)
 		atomic_long_inc(&erofs_global_shrink_cnt);
-	--grp->lockref.count;
-	spin_unlock(&grp->lockref.lock);
+	--pcl->lockref.count;
+	spin_unlock(&pcl->lockref.lock);
 }
 
 static void z_erofs_pcluster_end(struct z_erofs_decompress_frontend *fe)
@@ -1497,9 +1486,9 @@ static void z_erofs_fill_bio_vec(struct bio_vec *bvec,
 	bvec->bv_offset = 0;
 	bvec->bv_len = PAGE_SIZE;
 repeat:
-	spin_lock(&pcl->obj.lockref.lock);
+	spin_lock(&pcl->lockref.lock);
 	zbv = pcl->compressed_bvecs[nr];
-	spin_unlock(&pcl->obj.lockref.lock);
+	spin_unlock(&pcl->lockref.lock);
 	if (!zbv.page)
 		goto out_allocfolio;
 
@@ -1561,23 +1550,23 @@ static void z_erofs_fill_bio_vec(struct bio_vec *bvec,
 	folio_put(folio);
 out_allocfolio:
 	page = __erofs_allocpage(&f->pagepool, gfp, true);
-	spin_lock(&pcl->obj.lockref.lock);
+	spin_lock(&pcl->lockref.lock);
 	if (unlikely(pcl->compressed_bvecs[nr].page != zbv.page)) {
 		if (page)
 			erofs_pagepool_add(&f->pagepool, page);
-		spin_unlock(&pcl->obj.lockref.lock);
+		spin_unlock(&pcl->lockref.lock);
 		cond_resched();
 		goto repeat;
 	}
 	pcl->compressed_bvecs[nr].page = page ? page : ERR_PTR(-ENOMEM);
-	spin_unlock(&pcl->obj.lockref.lock);
+	spin_unlock(&pcl->lockref.lock);
 	bvec->bv_page = page;
 	if (!page)
 		return;
 	folio = page_folio(page);
 out_tocache:
 	if (!tocache || bs != PAGE_SIZE ||
-	    filemap_add_folio(mc, folio, pcl->obj.index + nr, gfp)) {
+	    filemap_add_folio(mc, folio, pcl->index + nr, gfp)) {
 		/* turn into a temporary shortlived folio (1 ref) */
 		folio->private = (void *)Z_EROFS_SHORTLIVED_PAGE;
 		return;
@@ -1709,7 +1698,7 @@ static void z_erofs_submit_queue(struct z_erofs_decompress_frontend *f,
 
 		/* no device id here, thus it will always succeed */
 		mdev = (struct erofs_map_dev) {
-			.m_pa = erofs_pos(sb, pcl->obj.index),
+			.m_pa = erofs_pos(sb, pcl->index),
 		};
 		(void)erofs_map_dev(sb, &mdev);
 
-- 
2.43.5

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

[Chao Yu]

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,

> -			fe->pcl = container_of(grp,
> +		if (xa_is_err(pre)) {
> +			err = xa_err(pre);
> +			goto err_out;
> +		} else if (pre) {
> +			fe->pcl = container_of(pre,
>   					struct z_erofs_pcluster, obj);
>   			err = -EEXIST;
>   			goto err_out;
> @@ -789,7 +798,16 @@ static int z_erofs_pcluster_begin(struct z_erofs_decompress_frontend *fe)
>   	DBG_BUGON(fe->owned_head == Z_EROFS_PCLUSTER_NIL);
>   
>   	if (!(map->m_flags & EROFS_MAP_META)) {
> -		grp = erofs_find_workgroup(sb, blknr);
> +		while (1) {
> +			rcu_read_lock();
> +			grp = xa_load(&EROFS_SB(sb)->managed_pslots, blknr);
> +			if (!grp || erofs_workgroup_get(grp)) {
> +				DBG_BUGON(grp && blknr != grp->index);
> +				rcu_read_unlock();
> +				break;
> +			}
> +			rcu_read_unlock();
> +		}
>   	} else if ((map->m_pa & ~PAGE_MASK) + map->m_plen > PAGE_SIZE) {
>   		DBG_BUGON(1);
>   		return -EFSCORRUPTED;
> diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
> index 37afe2024840..218b0249a482 100644
> --- a/fs/erofs/zutil.c
> +++ b/fs/erofs/zutil.c
> @@ -214,7 +214,7 @@ void erofs_release_pages(struct page **pagepool)
>   	}
>   }
>   
> -static bool erofs_workgroup_get(struct erofs_workgroup *grp)
> +bool erofs_workgroup_get(struct erofs_workgroup *grp)
>   {
>   	if (lockref_get_not_zero(&grp->lockref))
>   		return true;
> @@ -231,54 +231,6 @@ static bool erofs_workgroup_get(struct erofs_workgroup *grp)
>   	return true;
>   }
>   
> -struct erofs_workgroup *erofs_find_workgroup(struct super_block *sb,
> -					     pgoff_t index)
> -{
> -	struct erofs_sb_info *sbi = EROFS_SB(sb);
> -	struct erofs_workgroup *grp;
> -
> -repeat:
> -	rcu_read_lock();
> -	grp = xa_load(&sbi->managed_pslots, index);
> -	if (grp) {
> -		if (!erofs_workgroup_get(grp)) {
> -			/* prefer to relax rcu read side */
> -			rcu_read_unlock();
> -			goto repeat;
> -		}
> -
> -		DBG_BUGON(index != grp->index);
> -	}
> -	rcu_read_unlock();
> -	return grp;
> -}
> -
> -struct erofs_workgroup *erofs_insert_workgroup(struct super_block *sb,
> -					       struct erofs_workgroup *grp)
> -{
> -	struct erofs_sb_info *const sbi = EROFS_SB(sb);
> -	struct erofs_workgroup *pre;
> -
> -	DBG_BUGON(grp->lockref.count < 1);
> -repeat:
> -	xa_lock(&sbi->managed_pslots);
> -	pre = __xa_cmpxchg(&sbi->managed_pslots, grp->index,
> -			   NULL, grp, GFP_KERNEL);
> -	if (pre) {
> -		if (xa_is_err(pre)) {
> -			pre = ERR_PTR(xa_err(pre));
> -		} else if (!erofs_workgroup_get(pre)) {
> -			/* try to legitimize the current in-tree one */
> -			xa_unlock(&sbi->managed_pslots);
> -			cond_resched();
> -			goto repeat;
> -		}
> -		grp = pre;
> -	}
> -	xa_unlock(&sbi->managed_pslots);
> -	return grp;
> -}
> -
>   static void  __erofs_workgroup_free(struct erofs_workgroup *grp)
>   {
>   	atomic_long_dec(&erofs_global_shrink_cnt);

Re: [PATCH v2 2/3] erofs: move erofs_workgroup operations into zdata.c

[Chao Yu]

On 2024/10/21 11:53, Gao Xiang wrote:
> Move related helpers into zdata.c as an intermediate step of getting
> rid of `struct erofs_workgroup`, and rename:
> 
>   erofs_workgroup_put => z_erofs_put_pcluster
>   erofs_workgroup_get => z_erofs_get_pcluster
>   erofs_try_to_release_workgroup => erofs_try_to_release_pcluster
>   erofs_shrink_workstation => z_erofs_shrink_scan
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [PATCH v2 3/3] erofs: sunset `struct erofs_workgroup`

[Chao Yu]

On 2024/10/21 11:53, Gao Xiang wrote:
> `struct erofs_workgroup` was introduced to provide a unique header
> for all physically indexed objects.  However, after big pclusters and
> shared pclusters are implemented upstream, it seems that all EROFS
> encoded data (which requires transformation) can be represented with
> `struct z_erofs_pcluster` directly.
> 
> Move all members into `struct z_erofs_pcluster` for simplicity.
> 
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

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

[Gao Xiang]

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

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@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