[PATCH v1 0/1] erofs: start per-CPU workers on demand

[PATCH v1 0/1] erofs: start per-CPU workers on demand

[Sandeep Dhavale]

erofs currently starts per-CPU workers on module_init() which is not
necessary. This starting of resources on cpu hotplug and unplug
shows up in Android where the erofs in built-in and erofs is
not yet being used.

Following patch moves the creation of per-CPU workers on the first
mount and they are removed when on the last unmount.

I tested this with erofs-utils test as well as running reads on
erofs mountpoint while running aggressive cpu online/offline in a
loop, withoutn any issue.

Thanks,
Sandeep.

Sandeep Dhavale (1):
  erofs: lazily initialize per-CPU workers and CPU hotplug support

 fs/erofs/internal.h |  5 +++++
 fs/erofs/super.c    | 27 +++++++++++++++++++++++++++
 fs/erofs/zdata.c    | 35 +++++++++++++++++++++++------------
 3 files changed, 55 insertions(+), 12 deletions(-)

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Sandeep Dhavale]

Defer initialization of per-CPU workers and registration for CPU hotplug
events until the first mount. Similarly, unregister from hotplug events
and destroy per-CPU workers when the last mount is unmounted.

Signed-off-by: Sandeep Dhavale <dhavale@google.com>
---
 fs/erofs/internal.h |  5 +++++
 fs/erofs/super.c    | 27 +++++++++++++++++++++++++++
 fs/erofs/zdata.c    | 35 +++++++++++++++++++++++------------
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 4ac188d5d894..c88cba4da3eb 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -450,6 +450,8 @@ int z_erofs_gbuf_growsize(unsigned int nrpages);
 int __init z_erofs_gbuf_init(void);
 void z_erofs_gbuf_exit(void);
 int z_erofs_parse_cfgs(struct super_block *sb, struct erofs_super_block *dsb);
+int z_erofs_init_workers(void);
+void z_erofs_destroy_workers(void);
 #else
 static inline void erofs_shrinker_register(struct super_block *sb) {}
 static inline void erofs_shrinker_unregister(struct super_block *sb) {}
@@ -458,6 +460,9 @@ static inline void erofs_exit_shrinker(void) {}
 static inline int z_erofs_init_subsystem(void) { return 0; }
 static inline void z_erofs_exit_subsystem(void) {}
 static inline int z_erofs_init_super(struct super_block *sb) { return 0; }
+static inline int z_erofs_init_workers(void) { return 0; };
+static inline  z_erofs_exit_workers(void);
+
 #endif	/* !CONFIG_EROFS_FS_ZIP */
 
 #ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index cadec6b1b554..8e8d3a7c8dba 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -17,6 +17,7 @@
 #include <trace/events/erofs.h>
 
 static struct kmem_cache *erofs_inode_cachep __read_mostly;
+static atomic_t erofs_mount_count = ATOMIC_INIT(0);
 
 void _erofs_printk(struct super_block *sb, const char *fmt, ...)
 {
@@ -777,9 +778,28 @@ static const struct fs_context_operations erofs_context_ops = {
 	.free		= erofs_fc_free,
 };
 
+static inline int erofs_init_zip_workers_if_needed(void)
+{
+	int ret;
+
+	if (atomic_inc_return(&erofs_mount_count) == 1) {
+		ret = z_erofs_init_workers();
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static inline void erofs_destroy_zip_workers_if_last(void)
+{
+	if (atomic_dec_and_test(&erofs_mount_count))
+		z_erofs_destroy_workers();
+}
+
 static int erofs_init_fs_context(struct fs_context *fc)
 {
 	struct erofs_sb_info *sbi;
+	int err;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
@@ -790,6 +810,12 @@ static int erofs_init_fs_context(struct fs_context *fc)
 		kfree(sbi);
 		return -ENOMEM;
 	}
+	err = erofs_init_zip_workers_if_needed();
+	if (err) {
+		kfree(sbi->devs);
+		kfree(sbi);
+		return err;
+	}
 	fc->s_fs_info = sbi;
 
 	idr_init(&sbi->devs->tree);
@@ -823,6 +849,7 @@ static void erofs_kill_sb(struct super_block *sb)
 	erofs_fscache_unregister_fs(sb);
 	erofs_sb_free(sbi);
 	sb->s_fs_info = NULL;
+	erofs_destroy_zip_workers_if_last();
 }
 
 static void erofs_put_super(struct super_block *sb)
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 0671184d9cf1..4cd91b798716 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -403,10 +403,32 @@ static inline int erofs_cpu_hotplug_init(void) { return 0; }
 static inline void erofs_cpu_hotplug_destroy(void) {}
 #endif
 
-void z_erofs_exit_subsystem(void)
+int z_erofs_init_workers(void)
+{
+	int err;
+
+	err = erofs_init_percpu_workers();
+	if (err)
+		return err;
+
+	err = erofs_cpu_hotplug_init();
+	if (err < 0)
+		goto err_cpuhp_init;
+	return err;
+
+err_cpuhp_init:
+	erofs_destroy_percpu_workers();
+	return err;
+}
+
+void z_erofs_destroy_workers(void)
 {
 	erofs_cpu_hotplug_destroy();
 	erofs_destroy_percpu_workers();
+}
+
+void z_erofs_exit_subsystem(void)
+{
 	destroy_workqueue(z_erofs_workqueue);
 	z_erofs_destroy_pcluster_pool();
 	z_erofs_exit_decompressor();
@@ -430,19 +452,8 @@ int __init z_erofs_init_subsystem(void)
 		goto err_workqueue_init;
 	}
 
-	err = erofs_init_percpu_workers();
-	if (err)
-		goto err_pcpu_worker;
-
-	err = erofs_cpu_hotplug_init();
-	if (err < 0)
-		goto err_cpuhp_init;
 	return err;
 
-err_cpuhp_init:
-	erofs_destroy_percpu_workers();
-err_pcpu_worker:
-	destroy_workqueue(z_erofs_workqueue);
 err_workqueue_init:
 	z_erofs_destroy_pcluster_pool();
 err_pcluster_pool:
-- 
2.49.0.472.ge94155a9ec-goog

Re: [PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Gao Xiang]

Hi Sandeep,

On 2025/3/31 10:20, Sandeep Dhavale wrote:
> Defer initialization of per-CPU workers and registration for CPU hotplug
> events until the first mount. Similarly, unregister from hotplug events
> and destroy per-CPU workers when the last mount is unmounted.
> 
> Signed-off-by: Sandeep Dhavale <dhavale@google.com>
> ---
>   fs/erofs/internal.h |  5 +++++
>   fs/erofs/super.c    | 27 +++++++++++++++++++++++++++
>   fs/erofs/zdata.c    | 35 +++++++++++++++++++++++------------
>   3 files changed, 55 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 4ac188d5d894..c88cba4da3eb 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -450,6 +450,8 @@ int z_erofs_gbuf_growsize(unsigned int nrpages);
>   int __init z_erofs_gbuf_init(void);
>   void z_erofs_gbuf_exit(void);
>   int z_erofs_parse_cfgs(struct super_block *sb, struct erofs_super_block *dsb);
> +int z_erofs_init_workers(void);
> +void z_erofs_destroy_workers(void);
>   #else
>   static inline void erofs_shrinker_register(struct super_block *sb) {}
>   static inline void erofs_shrinker_unregister(struct super_block *sb) {}
> @@ -458,6 +460,9 @@ static inline void erofs_exit_shrinker(void) {}
>   static inline int z_erofs_init_subsystem(void) { return 0; }
>   static inline void z_erofs_exit_subsystem(void) {}
>   static inline int z_erofs_init_super(struct super_block *sb) { return 0; }
> +static inline int z_erofs_init_workers(void) { return 0; };
> +static inline  z_erofs_exit_workers(void);
> +
>   #endif	/* !CONFIG_EROFS_FS_ZIP */
>   
>   #ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index cadec6b1b554..8e8d3a7c8dba 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -17,6 +17,7 @@
>   #include <trace/events/erofs.h>
>   
>   static struct kmem_cache *erofs_inode_cachep __read_mostly;
> +static atomic_t erofs_mount_count = ATOMIC_INIT(0);
>   
>   void _erofs_printk(struct super_block *sb, const char *fmt, ...)
>   {
> @@ -777,9 +778,28 @@ static const struct fs_context_operations erofs_context_ops = {
>   	.free		= erofs_fc_free,
>   };
>   
> +static inline int erofs_init_zip_workers_if_needed(void)
> +{
> +	int ret;
> +
> +	if (atomic_inc_return(&erofs_mount_count) == 1) {
> +		ret = z_erofs_init_workers();
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static inline void erofs_destroy_zip_workers_if_last(void)

Do we really need to destroy workers on the last mount?
it could cause many unnecessary init/uninit cycles.

Or your requirement is just to defer per-CPU workers to
the first mount?

If your case is the latter, I guess you could just call
erofs_init_percpu_workers() in z_erofs_init_super().

> +{
> +	if (atomic_dec_and_test(&erofs_mount_count))

So in that case, we won't need erofs_mount_count anymore,
you could just add a pcpu_worker_initialized atomic bool
to control that.

Thanks,
Gao Xiang

Re: [PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Sandeep Dhavale]

Hi Gao,
> Do we really need to destroy workers on the last mount?
> it could cause many unnecessary init/uninit cycles.
>
> Or your requirement is just to defer per-CPU workers to
> the first mount?
>
> If your case is the latter, I guess you could just call
> erofs_init_percpu_workers() in z_erofs_init_super().
>
> > +{
> > +     if (atomic_dec_and_test(&erofs_mount_count))
>
> So in that case, we won't need erofs_mount_count anymore,
> you could just add a pcpu_worker_initialized atomic bool
> to control that.
>
Android devices go through suspend and resume cycles aggressively.

And currently long running traces showed that erofs_workers being
created and destroyed without active erofs mount.
Your suggestion is good and could work for devices which do not use
erofs at all. But if erofs is used once (and unmounted later),
we will not destroy the percpu workers.

Can you please expand a little bit more on your concern
> it could cause many unnecessary init/uninit cycles.
Did you mean on the cases where only one erofs fs
is mounted at time? Just trying to see if there is a better
way to address your concern.

Thanks,
Sandeep.

Re: [PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Gao Xiang]

On 2025/3/31 11:14, Sandeep Dhavale wrote:
> Hi Gao,
>> Do we really need to destroy workers on the last mount?
>> it could cause many unnecessary init/uninit cycles.
>>
>> Or your requirement is just to defer per-CPU workers to
>> the first mount?
>>
>> If your case is the latter, I guess you could just call
>> erofs_init_percpu_workers() in z_erofs_init_super().
>>
>>> +{
>>> +     if (atomic_dec_and_test(&erofs_mount_count))
>>
>> So in that case, we won't need erofs_mount_count anymore,
>> you could just add a pcpu_worker_initialized atomic bool
>> to control that.
>>
> Android devices go through suspend and resume cycles aggressively.
> 
> And currently long running traces showed that erofs_workers being
> created and destroyed without active erofs mount.
> Your suggestion is good and could work for devices which do not use
> erofs at all.

> But if erofs is used once (and unmounted later),> we will not destroy the percpu workers.

Is there a real use case in Android like this?  It
would be really useful to write down something in the
commit message.

> 
> Can you please expand a little bit more on your concern
>> it could cause many unnecessary init/uninit cycles.
> Did you mean on the cases where only one erofs fs
> is mounted at time? Just trying to see if there is a better
> way to address your concern.

My concern is that it could slow down the mount time (on
the single mount/unmount) if there are too many CPUs
(especially on the server side.. 96 CPUs or more...)

Or I guess if kworker CPU hotplug is not used at all
for Android if "suspend and resume" latency is really
important, could we just add a mode to always initialize
pcpu kworkers for all possible CPUs.

Thanks,
Gao Xiang

Re: [PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Sandeep Dhavale]

>
> Is there a real use case in Android like this?  It
> would be really useful to write down something in the
> commit message.
>
No, not that I am aware of.
I was just trying to cover all the possibilities.
> >
> > Can you please expand a little bit more on your concern
> >> it could cause many unnecessary init/uninit cycles.
> > Did you mean on the cases where only one erofs fs
> > is mounted at time? Just trying to see if there is a better
> > way to address your concern.
>
> My concern is that it could slow down the mount time (on
> the single mount/unmount) if there are too many CPUs
> (especially on the server side.. 96 CPUs or more...)
>
Yeah, that would be slower.
> Or I guess if kworker CPU hotplug is not used at all
> for Android if "suspend and resume" latency is really
> important, could we just add a mode to always initialize
> pcpu kworkers for all possible CPUs.
>
In Android CPU hotplug is used and has been working for
couple of years without issues for devices. This was brought up
to me for the devices which are not using erofs yet.

I will take your suggestion and work on V2.

Thanks for the feedback!

Regards,
Sandeep.

Re: [PATCH v1 1/1] erofs: lazily initialize per-CPU workers and CPU hotplug hooks

[Gao Xiang]

On 2025/3/31 11:28, Gao Xiang wrote:
> 
> 
> On 2025/3/31 11:14, Sandeep Dhavale wrote:
>> Hi Gao,
>>> Do we really need to destroy workers on the last mount?
>>> it could cause many unnecessary init/uninit cycles.
>>>
>>> Or your requirement is just to defer per-CPU workers to
>>> the first mount?
>>>
>>> If your case is the latter, I guess you could just call
>>> erofs_init_percpu_workers() in z_erofs_init_super().
>>>
>>>> +{
>>>> +     if (atomic_dec_and_test(&erofs_mount_count))
>>>
>>> So in that case, we won't need erofs_mount_count anymore,
>>> you could just add a pcpu_worker_initialized atomic bool
>>> to control that.
>>>
>> Android devices go through suspend and resume cycles aggressively.
>>
>> And currently long running traces showed that erofs_workers being
>> created and destroyed without active erofs mount.
>> Your suggestion is good and could work for devices which do not use
>> erofs at all.
> 
>> But if erofs is used once (and unmounted later),> we will not destroy the percpu workers.
> 
> Is there a real use case in Android like this?  It
> would be really useful to write down something in the
> commit message.
> 
>>
>> Can you please expand a little bit more on your concern
>>> it could cause many unnecessary init/uninit cycles.
>> Did you mean on the cases where only one erofs fs
>> is mounted at time? Just trying to see if there is a better
>> way to address your concern.
> 
> My concern is that it could slow down the mount time (on
> the single mount/unmount) if there are too many CPUs
> (especially on the server side.. 96 CPUs or more...)
> 
> Or I guess if kworker CPU hotplug is not used at all
> for Android if "suspend and resume" latency is really
> important, could we just add a mode to always initialize
> pcpu kworkers for all possible CPUs.

Ok, ignore this part after more thinking.

Thanks,
Gao Xiang