[PATCH 0/2] PM: Add a switch for disabling/enabling sync() before suspend

[PATCH 0/2] PM: Add a switch for disabling/enabling sync() before suspend

[Jonas Meurer]

Hello,

Introduce a new run-time sysfs switch to disable/enable sync() before
system suspend. This is useful to avoid races and deadlocks if block
devices have been suspended before, e.g. by 'cryptsetup luksSuspend'.

The second patch changes the behaviour of build-time switch
'CONFIG_SUSPEND_SKIP_SYNC' accordingly, using the build-time switch value
as default for our new run-time switch '/sys/power/sync_on_suspend'.

Jonas Meurer (2):
  PM: Add a switch for disabling/enabling sync() before suspend
  PM: CONFIG_SUSPEND_SKIP_SYNC sets default for '/sys/power/sync_on_suspend'

 Documentation/ABI/testing/sysfs-power | 15 +++++++++++++++
 include/linux/suspend.h               |  2 ++
 kernel/power/Kconfig                  |  5 ++++-
 kernel/power/main.c                   | 33 +++++++++++++++++++++++++++++++++
 kernel/power/suspend.c                |  2 +-
 5 files changed, 55 insertions(+), 2 deletions(-)

[PATCH 1/2] PM: Add a switch for disabling/enabling sync() before, suspend

[Jonas Meurer]

The switch allows to enable or disable the final sync() from the suspend.c
Linux Kernel system suspend implementation. This is useful to avoid race
conditions if block devices have been suspended before. Be aware that you
have to take care of sync() yourself before suspending the system if you
disable it here.

Signed-off-by: Jonas Meurer <jonas@freesources.org>
---
 Documentation/ABI/testing/sysfs-power | 14 ++++++++++++
 include/linux/suspend.h               |  2 ++
 kernel/power/main.c                   | 33 +++++++++++++++++++++++++++
 kernel/power/suspend.c                |  2 +-
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 6f87b9dd384b..f164a364e89a 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -407,3 +407,17 @@ Contact:	Kalesh Singh <kaleshsingh96@gmail.com>
 Description:
 		The /sys/power/suspend_stats/last_failed_step file contains
 		the last failed step in the suspend/resume path.
+
+What:		/sys/power/sync_on_suspend
+Date:		October 2019
+Contact:	Jonas Meurer <jonas@freesources.org>
+Description:
+		This file controls the switch to enable or disable the final
+		sync() before system suspend. This is useful to avoid race
+		conditions if block devices have been suspended before. Be
+		aware that you have to take care of sync() yourself before
+		suspending the system if you disable it here.
+
+		Writing a "1" (default) to this file enables the sync() and
+		writing a "0" disables it. Reads from the file return the
+		current value.
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6fc8843f1c9e..4a230c2f1c31 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -329,6 +329,7 @@ extern void arch_suspend_disable_irqs(void);
 extern void arch_suspend_enable_irqs(void);
 
 extern int pm_suspend(suspend_state_t state);
+extern bool sync_on_suspend_enabled;
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
@@ -342,6 +343,7 @@ static inline bool pm_suspend_default_s2idle(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline bool sync_on_suspend_enabled(void) { return true; }
 static inline bool idle_should_enter_s2idle(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
diff --git a/kernel/power/main.c b/kernel/power/main.c
index e26de7af520b..7d9d579a8e6f 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -190,6 +190,38 @@ static ssize_t mem_sleep_store(struct kobject *kobj, struct kobj_attribute *attr
 }
 
 power_attr(mem_sleep);
+
+/*
+ * sync_on_suspend: invoke ksys_sync_helper() before suspend.
+ *
+ * show() returns whether ksys_sync_helper() is invoked before suspend.
+ * store() accepts 0 or 1.  0 disables ksys_sync_helper() and 1 enables it.
+ */
+bool sync_on_suspend_enabled = true;
+
+static ssize_t sync_on_suspend_show(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", sync_on_suspend_enabled);
+}
+
+static ssize_t sync_on_suspend_store(struct kobject *kobj,
+				    struct kobj_attribute *attr,
+				    const char *buf, size_t n)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val > 1)
+		return -EINVAL;
+
+	sync_on_suspend_enabled = !!val;
+	return n;
+}
+
+power_attr(sync_on_suspend);
 #endif /* CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM_SLEEP_DEBUG
@@ -855,6 +887,7 @@ static struct attribute * g[] = {
 	&wakeup_count_attr.attr,
 #ifdef CONFIG_SUSPEND
 	&mem_sleep_attr.attr,
+	&sync_on_suspend_attr.attr,
 #endif
 #ifdef CONFIG_PM_AUTOSLEEP
 	&autosleep_attr.attr,
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index f3b7239f1892..503d56419a69 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -564,7 +564,7 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
-	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC)) {
+	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 		ksys_sync_helper();
 		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
-- 
2.20.1

Re: [PATCH 1/2] PM: Add a switch for disabling/enabling sync() before, suspend

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Mon 2019-12-02 18:07:05, Jonas Meurer wrote:
> The switch allows to enable or disable the final sync() from the suspend.c
> Linux Kernel system suspend implementation. This is useful to avoid race
> conditions if block devices have been suspended before. Be aware that you
> have to take care of sync() yourself before suspending the system if you
> disable it here.
> 
> Signed-off-by: Jonas Meurer <jonas@freesources.org>
> ---
>  Documentation/ABI/testing/sysfs-power | 14 ++++++++++++
>  include/linux/suspend.h               |  2 ++
>  kernel/power/main.c                   | 33 +++++++++++++++++++++++++++
>  kernel/power/suspend.c                |  2 +-
>  4 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index 6f87b9dd384b..f164a364e89a 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -407,3 +407,17 @@ Contact:	Kalesh Singh <kaleshsingh96@gmail.com>
>  Description:
>  		The /sys/power/suspend_stats/last_failed_step file contains
>  		the last failed step in the suspend/resume path.
> +
> +What:		/sys/power/sync_on_suspend
> +Date:		October 2019
> +Contact:	Jonas Meurer <jonas@freesources.org>
> +Description:
> +		This file controls the switch to enable or disable the final
> +		sync() before system suspend. This is useful to avoid race
> +		conditions if block devices have been suspended before. Be
> +		aware that you have to take care of sync() yourself before
> +		suspending the system if you disable it here.
> +
> +		Writing a "1" (default) to this file enables the sync() and
> +		writing a "0" disables it. Reads from the file return the
> +		current value.

This pushes kernel's responsibility on the user, and adds to the mess
we already have there.

Just... don't do it? Move sync somewhere where it can be done?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/2] PM: Add a switch for disabling/enabling sync() before, suspend

[Jonas Meurer]

Hello Pavel,

Pavel Machek:
> On Mon 2019-12-02 18:07:05, Jonas Meurer wrote:
>> The switch allows to enable or disable the final sync() from the suspend.c
>> Linux Kernel system suspend implementation. This is useful to avoid race
>> conditions if block devices have been suspended before. Be aware that you
>> have to take care of sync() yourself before suspending the system if you
>> disable it here.
>>
>> Signed-off-by: Jonas Meurer <jonas@freesources.org>
>> ---
>>  Documentation/ABI/testing/sysfs-power | 14 ++++++++++++
>>  include/linux/suspend.h               |  2 ++
>>  kernel/power/main.c                   | 33 +++++++++++++++++++++++++++
>>  kernel/power/suspend.c                |  2 +-
>>  4 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index 6f87b9dd384b..f164a364e89a 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -407,3 +407,17 @@ Contact:	Kalesh Singh <kaleshsingh96@gmail.com>
>>  Description:
>>  		The /sys/power/suspend_stats/last_failed_step file contains
>>  		the last failed step in the suspend/resume path.
>> +
>> +What:		/sys/power/sync_on_suspend
>> +Date:		October 2019
>> +Contact:	Jonas Meurer <jonas@freesources.org>
>> +Description:
>> +		This file controls the switch to enable or disable the final
>> +		sync() before system suspend. This is useful to avoid race
>> +		conditions if block devices have been suspended before. Be
>> +		aware that you have to take care of sync() yourself before
>> +		suspending the system if you disable it here.
>> +
>> +		Writing a "1" (default) to this file enables the sync() and
>> +		writing a "0" disables it. Reads from the file return the
>> +		current value.
> 
> This pushes kernel's responsibility on the user, and adds to the mess
> we already have there.

I hear you and I agree that the current situation is not the cleanest one.

> Just... don't do it? Move sync somewhere where it can be done?

Can you elaborate on that? What do you mean with "where it can be done"?

I don't agree that removing sync from the suspend function is a good
idea. In the majority of use cases, the kernel actually is *expected* to
sync before suspend in order to prevent potential data loss, no?

So it's rather about the special cases like embedded world (where
suspend is used a lot for very short periods) and our use case, where
block devices are suspended before system suspend, where we don't want
the kernel to do the sync.

Also, if the sync was removed from kernel suspend function, all
userspace tools that trigger system suspend would have to be changed in
order to do it themselves. It would be a rather intrusive and
backwards-incompatible change in my eyes.

Cheers
 jonas

[PATCH 2/2] PM: CONFIG_SUSPEND_SKIP_SYNC sets default for, '/sys/power/sync_on_suspend'

[Jonas Meurer]

Slightly change the behaviour of build-time switch CONFIG_SUSPEND_SKIP_SYNC:
Make it configure the default for '/sys/power/sync_on_suspend', now that we
have a run-time switch for it.

Signed-off-by: Jonas Meurer <jonas@freesources.org>
---
 Documentation/ABI/testing/sysfs-power | 7 ++++---
 kernel/power/Kconfig                  | 5 ++++-
 kernel/power/main.c                   | 2 +-
 kernel/power/suspend.c                | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index f164a364e89a..783b3ce8eb06 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -418,6 +418,7 @@ Description:
 		aware that you have to take care of sync() yourself before
 		suspending the system if you disable it here.
 
-		Writing a "1" (default) to this file enables the sync() and
-		writing a "0" disables it. Reads from the file return the
-		current value.
+		Writing a "1" to this file enables the sync() and writing a
+		"0" disables it. Reads from the file return the current value.
+		The default is "1" but can be configured with the build-time
+		config flag "SUSPEND_SKIP_SYNC".
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index d3667b4075c1..7cbfbeacd68a 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -27,7 +27,10 @@ config SUSPEND_SKIP_SYNC
 	  Skip the kernel sys_sync() before freezing user processes.
 	  Some systems prefer not to pay this cost on every invocation
 	  of suspend, or they are content with invoking sync() from
-	  user-space before invoking suspend.  Say Y if that's your case.
+	  user-space before invoking suspend.  There's a run-time switch
+	  at '/sys/power/sync_on_suspend' to configure this behaviour.
+	  This setting changes the default for the run-tim switch. Say Y
+	  to change the default to disable the kernel sys_sync().
 
 config HIBERNATE_CALLBACKS
 	bool
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7d9d579a8e6f..69b7a8aeca3b 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -197,7 +197,7 @@ power_attr(mem_sleep);
  * show() returns whether ksys_sync_helper() is invoked before suspend.
  * store() accepts 0 or 1.  0 disables ksys_sync_helper() and 1 enables it.
  */
-bool sync_on_suspend_enabled = true;
+bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
 
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 503d56419a69..2c47280fbfc7 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -564,7 +564,7 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
-	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && sync_on_suspend_enabled) {
+	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 		ksys_sync_helper();
 		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
-- 
2.20.1

Re: [PATCH 2/2] PM: CONFIG_SUSPEND_SKIP_SYNC sets default for, '/sys/power/sync_on_suspend'

[Rafael J. Wysocki]

On Monday, December 2, 2019 6:07:43 PM CET Jonas Meurer wrote:
> Slightly change the behaviour of build-time switch CONFIG_SUSPEND_SKIP_SYNC:
> Make it configure the default for '/sys/power/sync_on_suspend', now that we
> have a run-time switch for it.
> 
> Signed-off-by: Jonas Meurer <jonas@freesources.org>
> ---
>  Documentation/ABI/testing/sysfs-power | 7 ++++---
>  kernel/power/Kconfig                  | 5 ++++-
>  kernel/power/main.c                   | 2 +-
>  kernel/power/suspend.c                | 2 +-
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index f164a364e89a..783b3ce8eb06 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -418,6 +418,7 @@ Description:
>  		aware that you have to take care of sync() yourself before
>  		suspending the system if you disable it here.
>  
> -		Writing a "1" (default) to this file enables the sync() and
> -		writing a "0" disables it. Reads from the file return the
> -		current value.
> +		Writing a "1" to this file enables the sync() and writing a
> +		"0" disables it. Reads from the file return the current value.
> +		The default is "1" but can be configured with the build-time
> +		config flag "SUSPEND_SKIP_SYNC".
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index d3667b4075c1..7cbfbeacd68a 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -27,7 +27,10 @@ config SUSPEND_SKIP_SYNC
>  	  Skip the kernel sys_sync() before freezing user processes.
>  	  Some systems prefer not to pay this cost on every invocation
>  	  of suspend, or they are content with invoking sync() from
> -	  user-space before invoking suspend.  Say Y if that's your case.
> +	  user-space before invoking suspend.  There's a run-time switch
> +	  at '/sys/power/sync_on_suspend' to configure this behaviour.
> +	  This setting changes the default for the run-tim switch. Say Y
> +	  to change the default to disable the kernel sys_sync().
>  
>  config HIBERNATE_CALLBACKS
>  	bool
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 7d9d579a8e6f..69b7a8aeca3b 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -197,7 +197,7 @@ power_attr(mem_sleep);
>   * show() returns whether ksys_sync_helper() is invoked before suspend.
>   * store() accepts 0 or 1.  0 disables ksys_sync_helper() and 1 enables it.
>   */
> -bool sync_on_suspend_enabled = true;
> +bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
>  
>  static ssize_t sync_on_suspend_show(struct kobject *kobj,
>  				   struct kobj_attribute *attr, char *buf)
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 503d56419a69..2c47280fbfc7 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -564,7 +564,7 @@ static int enter_state(suspend_state_t state)
>  	if (state == PM_SUSPEND_TO_IDLE)
>  		s2idle_begin();
>  
> -	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && sync_on_suspend_enabled) {
> +	if (sync_on_suspend_enabled) {
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>  		ksys_sync_helper();
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> 

I don't see much value in splitting the changes into two patches.
On the contrary, this patch is a logical part of the full change, so IMO
it should be folded into the [1/2].

Please do so and resubmit.

Thanks!

Re: [PATCH 2/2] PM: CONFIG_SUSPEND_SKIP_SYNC sets default for, '/sys/power/sync_on_suspend'

[Jonas Meurer]

Hey Rafael,

Rafael J. Wysocki:
> On Monday, December 2, 2019 6:07:43 PM CET Jonas Meurer wrote:
>> Slightly change the behaviour of build-time switch CONFIG_SUSPEND_SKIP_SYNC:
>> Make it configure the default for '/sys/power/sync_on_suspend', now that we
>> have a run-time switch for it.
>> [...]
> 
> I don't see much value in splitting the changes into two patches.
> On the contrary, this patch is a logical part of the full change, so IMO
> it should be folded into the [1/2].
> 
> Please do so and resubmit.

Thanks for looking into the patch!

I resubmitted it now as a single patch:

[PATCH v2] PM: Add a switch for disabling/enabling sync() before suspend

Kind regards
 jonas

Re: [PATCH 0/2] PM: Add a switch for disabling/enabling sync() before suspend

[Jonas Meurer]

Hello,

Jonas Meurer:
> Hello,
> 
> Introduce a new run-time sysfs switch to disable/enable sync() before
> system suspend. This is useful to avoid races and deadlocks if block
> devices have been suspended before, e.g. by 'cryptsetup luksSuspend'.
> 
> The second patch changes the behaviour of build-time switch
> 'CONFIG_SUSPEND_SKIP_SYNC' accordingly, using the build-time switch value
> as default for our new run-time switch '/sys/power/sync_on_suspend'.
> 
> Jonas Meurer (2):
>   PM: Add a switch for disabling/enabling sync() before suspend
>   PM: CONFIG_SUSPEND_SKIP_SYNC sets default for '/sys/power/sync_on_suspend'
> 
>  Documentation/ABI/testing/sysfs-power | 15 +++++++++++++++
>  include/linux/suspend.h               |  2 ++
>  kernel/power/Kconfig                  |  5 ++++-
>  kernel/power/main.c                   | 33 +++++++++++++++++++++++++++++++++
>  kernel/power/suspend.c                |  2 +-
>  5 files changed, 55 insertions(+), 2 deletions(-)

Any chance to get a review/comment on this patch? What's the next
logical steps to get it merged?

Cheers
 jonas

Re: [PATCH 0/2] PM: Add a switch for disabling/enabling sync() before suspend

[Rafael J. Wysocki]

On Wednesday, December 11, 2019 4:59:29 PM CET Jonas Meurer wrote:
> Hello,
> 
> Jonas Meurer:
> > Hello,
> > 
> > Introduce a new run-time sysfs switch to disable/enable sync() before
> > system suspend. This is useful to avoid races and deadlocks if block
> > devices have been suspended before, e.g. by 'cryptsetup luksSuspend'.
> > 
> > The second patch changes the behaviour of build-time switch
> > 'CONFIG_SUSPEND_SKIP_SYNC' accordingly, using the build-time switch value
> > as default for our new run-time switch '/sys/power/sync_on_suspend'.
> > 
> > Jonas Meurer (2):
> >   PM: Add a switch for disabling/enabling sync() before suspend
> >   PM: CONFIG_SUSPEND_SKIP_SYNC sets default for '/sys/power/sync_on_suspend'
> > 
> >  Documentation/ABI/testing/sysfs-power | 15 +++++++++++++++
> >  include/linux/suspend.h               |  2 ++
> >  kernel/power/Kconfig                  |  5 ++++-
> >  kernel/power/main.c                   | 33 +++++++++++++++++++++++++++++++++
> >  kernel/power/suspend.c                |  2 +-
> >  5 files changed, 55 insertions(+), 2 deletions(-)
> 
> Any chance to get a review/comment on this patch? What's the next
> logical steps to get it merged?

This is not a super-high priority patchset IMO, but it is in my list of things
to look at.

Thanks!