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

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

[Jonas Meurer]

[-- Attachment #1.1: Type: text/plain, Size: 5823 bytes --]

[Sorry, resending with the correct mailinglist address as recipient]

Hello,

This patch adds a run-time switch at `/sys/power/suspend_sync`.

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.

Since this is my first patch against the Linux kernel and I don't
consider it ready for inclusion yet, I decided to send it to pm-linux
and the PM subsystem maintainers only first. Would be very glad if you
could take a look and comment on it :)

Some questions:

* There already is a build-time config flag[2] for en- or disabling the
  sync() in suspend.c. Is it acceptable to have both a build-time *and*
  a *run-time* switch? Or would a run-time switch have to replace the
  build-time switch? If so, a direct question to Rafael, as you added
  the build-time flag: Would that be ok for you?
* I'm unsure about the naming: since the default is to have the sync
  enabled, would `suspend_disable_sync` be a better name for the switch,
  obviously defaulting to 0 then and skipping the sync at value 1?

To give a bit more contect: In Debian, we're currently working[3] on
support to suspend unlocked dm-crypt devices before system suspend.
During that work, we realized that the final sync() from Linux Kernel
system suspend implementation can lead to a dead lock.

I wrote a simple reproducer[4] to cause the dead lock in a reliable way.


[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/power/suspend.c?id=54ecb8f#n569
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2fd77f
[3] https://salsa.debian.org/mejo/cryptsetup-suspend
[4] https://salsa.debian.org/mejo/cryptsetup-suspend/snippets/334


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

--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
-	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC)) {
+	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && suspend_sync_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 		ksys_sync_helper();
 		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -328,6 +328,7 @@ extern void arch_suspend_disable_irqs(vo
 extern void arch_suspend_enable_irqs(void);
 
 extern int pm_suspend(suspend_state_t state);
+extern bool suspend_sync_enabled;
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
@@ -340,6 +341,7 @@ static inline bool pm_suspend_via_s2idle
 
 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 suspend_sync_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) {}
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -191,6 +191,40 @@ static ssize_t mem_sleep_store(struct ko
 power_attr(mem_sleep);
 #endif /* CONFIG_SUSPEND */
 
+#ifdef CONFIG_SUSPEND
+/*
+ * suspend_sync: 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 suspend_sync_enabled = true;
+
+static ssize_t suspend_sync_show(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", suspend_sync_enabled);
+}
+
+static ssize_t suspend_sync_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;
+
+	suspend_sync_enabled = !!val;
+	return n;
+}
+
+power_attr(suspend_sync);
+#endif /* CONFIG_SUSPEND */
+
 #ifdef CONFIG_PM_SLEEP_DEBUG
 int pm_test_level = TEST_NONE;
 
@@ -769,6 +803,7 @@ static struct attribute * g[] = {
 	&wakeup_count_attr.attr,
 #ifdef CONFIG_SUSPEND
 	&mem_sleep_attr.attr,
+	&suspend_sync_attr.attr,
 #endif
 #ifdef CONFIG_PM_AUTOSLEEP
 	&autosleep_attr.attr,
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -300,4 +300,18 @@ Description:
 		attempt.
 
 		Using this sysfs file will override any values that were
-		set using the kernel command line for disk offset.
\ No newline at end of file
+		set using the kernel command line for disk offset.
+
+What:		/sys/power/suspend_sync
+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.
-- 
Jonas Meurer






[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jonas Meurer]

[-- Attachment #1.1: Type: text/plain, Size: 6328 bytes --]

Hello,

just a friendly nudge: can I expect feedback? In case I addressed the
wrong people/list, to you have a hint where else to send patch and
questions?

Thanks in advance :)

Cheers
 jonas

Jonas Meurer:
> [Sorry, resending with the correct mailinglist address as recipient]
> 
> Hello,
> 
> This patch adds a run-time switch at `/sys/power/suspend_sync`.
> 
> 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.
> 
> Since this is my first patch against the Linux kernel and I don't
> consider it ready for inclusion yet, I decided to send it to pm-linux
> and the PM subsystem maintainers only first. Would be very glad if you
> could take a look and comment on it :)
> 
> Some questions:
> 
> * There already is a build-time config flag[2] for en- or disabling the
>   sync() in suspend.c. Is it acceptable to have both a build-time *and*
>   a *run-time* switch? Or would a run-time switch have to replace the
>   build-time switch? If so, a direct question to Rafael, as you added
>   the build-time flag: Would that be ok for you?
> * I'm unsure about the naming: since the default is to have the sync
>   enabled, would `suspend_disable_sync` be a better name for the switch,
>   obviously defaulting to 0 then and skipping the sync at value 1?
> 
> To give a bit more contect: In Debian, we're currently working[3] on
> support to suspend unlocked dm-crypt devices before system suspend.
> During that work, we realized that the final sync() from Linux Kernel
> system suspend implementation can lead to a dead lock.
> 
> I wrote a simple reproducer[4] to cause the dead lock in a reliable way.
> 
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/power/suspend.c?id=54ecb8f#n569
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2fd77f
> [3] https://salsa.debian.org/mejo/cryptsetup-suspend
> [4] https://salsa.debian.org/mejo/cryptsetup-suspend/snippets/334
> 
> 
> Signed-off-by: Jonas Meurer <jonas@freesources.org>
> ---
>  Documentation/ABI/testing/sysfs-power |   16 ++++++++++++++-
>  include/linux/suspend.h               |    2 +
>  kernel/power/main.c                   |   35 ++++++++++++++++++++++++++++++++++
>  kernel/power/suspend.c                |    2 -
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
>  	if (state == PM_SUSPEND_TO_IDLE)
>  		s2idle_begin();
>  
> -	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC)) {
> +	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && suspend_sync_enabled) {
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>  		ksys_sync_helper();
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -328,6 +328,7 @@ extern void arch_suspend_disable_irqs(vo
>  extern void arch_suspend_enable_irqs(void);
>  
>  extern int pm_suspend(suspend_state_t state);
> +extern bool suspend_sync_enabled;
>  #else /* !CONFIG_SUSPEND */
>  #define suspend_valid_only_mem	NULL
>  
> @@ -340,6 +341,7 @@ static inline bool pm_suspend_via_s2idle
>  
>  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 suspend_sync_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) {}
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -191,6 +191,40 @@ static ssize_t mem_sleep_store(struct ko
>  power_attr(mem_sleep);
>  #endif /* CONFIG_SUSPEND */
>  
> +#ifdef CONFIG_SUSPEND
> +/*
> + * suspend_sync: 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 suspend_sync_enabled = true;
> +
> +static ssize_t suspend_sync_show(struct kobject *kobj,
> +				   struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", suspend_sync_enabled);
> +}
> +
> +static ssize_t suspend_sync_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;
> +
> +	suspend_sync_enabled = !!val;
> +	return n;
> +}
> +
> +power_attr(suspend_sync);
> +#endif /* CONFIG_SUSPEND */
> +
>  #ifdef CONFIG_PM_SLEEP_DEBUG
>  int pm_test_level = TEST_NONE;
>  
> @@ -769,6 +803,7 @@ static struct attribute * g[] = {
>  	&wakeup_count_attr.attr,
>  #ifdef CONFIG_SUSPEND
>  	&mem_sleep_attr.attr,
> +	&suspend_sync_attr.attr,
>  #endif
>  #ifdef CONFIG_PM_AUTOSLEEP
>  	&autosleep_attr.attr,
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -300,4 +300,18 @@ Description:
>  		attempt.
>  
>  		Using this sysfs file will override any values that were
> -		set using the kernel command line for disk offset.
> \ No newline at end of file
> +		set using the kernel command line for disk offset.
> +
> +What:		/sys/power/suspend_sync
> +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.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rafael J. Wysocki]

On Monday, October 7, 2019 12:50:14 PM CEST Jonas Meurer wrote:
>  This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
> --htRCyOoJ9Xv4BLXLNYp8X0x6Nr6crRDII
> Content-Type: multipart/mixed; boundary="GRH6EAOR51QxsekMaGMKMlm8winrq3Izm";
>  protected-headers="v1"
> From: Jonas Meurer <jonas@freesources.org>
> To: linux-pm@vger.kernel.org
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>, Pavel Machek <pavel@ucw.cz>,
>  Len Brown <len.brown@intel.com>, Tim Dittler <tim.dittler@systemli.org>
> Message-ID: <56b2db6a-2f76-a6d3-662a-819cfb18d424@freesources.org>
> Subject: [RFC PATCH] PM: Add a switch for disabling/enabling sync() before
>  suspend
> 
> --GRH6EAOR51QxsekMaGMKMlm8winrq3Izm
> Content-Type: text/plain; charset=utf-8
> Content-Language: de-DE
> Content-Transfer-Encoding: quoted-printable
> 
> [Sorry, resending with the correct mailinglist address as recipient]
> 
> Hello,
> 
> This patch adds a run-time switch at `/sys/power/suspend_sync`.

I'd prefer "sync_on_suspend".

> 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.
> 
> Since this is my first patch against the Linux kernel and I don't
> consider it ready for inclusion yet, I decided to send it to pm-linux
> and the PM subsystem maintainers only first. Would be very glad if you
> could take a look and comment on it :)
> 
> Some questions:
> 
> * There already is a build-time config flag[2] for en- or disabling the
>   sync() in suspend.c. Is it acceptable to have both a build-time *and*
>   a *run-time* switch? Or would a run-time switch have to replace the
>   build-time switch? If so, a direct question to Rafael, as you added
>   the build-time flag: Would that be ok for you?

If there is a run-time knob to disable the syncing, the only reason for
the config option to be there will be to set the default value of that.

> * I'm unsure about the naming: since the default is to have the sync
>   enabled, would `suspend_disable_sync` be a better name for the switch,
>   obviously defaulting to 0 then and skipping the sync at value 1?

The default is just the initial value of the new knob, the naming need not
be related to that.

> To give a bit more contect: In Debian, we're currently working[3] on
> support to suspend unlocked dm-crypt devices before system suspend.
> During that work, we realized that the final sync() from Linux Kernel
> system suspend implementation can lead to a dead lock.

That's also true for FUSE filesystems I think and please note that this isn't
going to work with hibernation (in which case filesystems are synced
regardless).

> I wrote a simple reproducer[4] to cause the dead lock in a reliable way.
> 
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr=
> ee/kernel/power/suspend.c?id=3D54ecb8f#n569
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/co=
> mmit/?id=3D2fd77f
> [3] https://salsa.debian.org/mejo/cryptsetup-suspend
> [4] https://salsa.debian.org/mejo/cryptsetup-suspend/snippets/334
> 
> 
> Signed-off-by: Jonas Meurer <jonas@freesources.org>
> ---
>  Documentation/ABI/testing/sysfs-power |   16 ++++++++++++++-
>  include/linux/suspend.h               |    2 +
>  kernel/power/main.c                   |   35 +++++++++++++++++++++++++++=
> +++++++
>  kernel/power/suspend.c                |    2 -
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
>  	if (state =3D=3D PM_SUSPEND_TO_IDLE)
>  		s2idle_begin();
> =20
> -	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC)) {
> +	if (!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC) && suspend_sync_enabled) {
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>  		ksys_sync_helper();
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -328,6 +328,7 @@ extern void arch_suspend_disable_irqs(vo
>  extern void arch_suspend_enable_irqs(void);
> =20
>  extern int pm_suspend(suspend_state_t state);
> +extern bool suspend_sync_enabled;
>  #else /* !CONFIG_SUSPEND */
>  #define suspend_valid_only_mem	NULL
> =20
> @@ -340,6 +341,7 @@ static inline bool pm_suspend_via_s2idle
> =20
>  static inline void suspend_set_ops(const struct platform_suspend_ops *op=
> s) {}
>  static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
> +static inline bool suspend_sync_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)=
>  {}
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -191,6 +191,40 @@ static ssize_t mem_sleep_store(struct ko
>  power_attr(mem_sleep);
>  #endif /* CONFIG_SUSPEND */
> =20
> +#ifdef CONFIG_SUSPEND
> +/*
> + * suspend_sync: 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 suspend_sync_enabled =3D true;
> +
> +static ssize_t suspend_sync_show(struct kobject *kobj,
> +				   struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", suspend_sync_enabled);
> +}
> +
> +static ssize_t suspend_sync_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;
> +
> +	suspend_sync_enabled =3D !!val;
> +	return n;
> +}
> +
> +power_attr(suspend_sync);
> +#endif /* CONFIG_SUSPEND */
> +
>  #ifdef CONFIG_PM_SLEEP_DEBUG
>  int pm_test_level =3D TEST_NONE;
> =20
> @@ -769,6 +803,7 @@ static struct attribute * g[] =3D {
>  	&wakeup_count_attr.attr,
>  #ifdef CONFIG_SUSPEND
>  	&mem_sleep_attr.attr,
> +	&suspend_sync_attr.attr,
>  #endif
>  #ifdef CONFIG_PM_AUTOSLEEP
>  	&autosleep_attr.attr,
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -300,4 +300,18 @@ Description:
>  		attempt.
> =20
>  		Using this sysfs file will override any values that were
> -		set using the kernel command line for disk offset.
> \ No newline at end of file
> +		set using the kernel command line for disk offset.
> +
> +What:		/sys/power/suspend_sync
> +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.
> --=20

The changes look reasonable to me.

Thanks,
Rafael

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

[Jonas Meurer]

Hi Rafael and linux-pm maintainers,

thanks a lot for your feedback, it's much appreciated!

Rafael J. Wysocki:
>> This patch adds a run-time switch at `/sys/power/suspend_sync`.
> 
> I'd prefer "sync_on_suspend".

Agreed and changed.

>> 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.
>>
>> Since this is my first patch against the Linux kernel and I don't
>> consider it ready for inclusion yet, I decided to send it to pm-linux
>> and the PM subsystem maintainers only first. Would be very glad if you
>> could take a look and comment on it :)
>>
>> Some questions:
>>
>> * There already is a build-time config flag[2] for en- or disabling the
>>   sync() in suspend.c. Is it acceptable to have both a build-time *and*
>>   a *run-time* switch? Or would a run-time switch have to replace the
>>   build-time switch? If so, a direct question to Rafael, as you added
>>   the build-time flag: Would that be ok for you?
> 
> If there is a run-time knob to disable the syncing, the only reason for
> the config option to be there will be to set the default value of that.

Makes sense. I changed the meaning of the build-time flag accordingly.

>> To give a bit more contect: In Debian, we're currently working[3] on
>> support to suspend unlocked dm-crypt devices before system suspend.
>> During that work, we realized that the final sync() from Linux Kernel
>> system suspend implementation can lead to a dead lock.
> 
> That's also true for FUSE filesystems I think and please note that this isn't
> going to work with hibernation (in which case filesystems are synced
> regardless).

In my opinion, hibernation doesn't matter much. Since the memory is
powered off on hibernation anyway, there's no reason to luksSuspend the
devices beforehands, don't you think so?

> The changes look reasonable to me.

Glad to read. I'll send a second version of the patches as replies soon
after this mail. How do we go on from here? Could you imagine to review
them again and sign them afterwards? Or am I supposed to send them to
the lkml and wait for feedback before getting more signers? As written,
I'm new to the Linux Kernel development process and not sure what's the
logical next step to get the patches merged.

Thanks again for your help!

Cheers
 jonas

[PATCH v2 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 |   16 ++++++++++++++-
 include/linux/suspend.h               |    2 +
 kernel/power/main.c                   |   35 ++++++++++++++++++++++++++++++++++
 kernel/power/suspend.c                |    2 -
 4 files changed, 53 insertions(+), 2 deletions(-)

--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
 	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);
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -328,6 +328,7 @@ extern void arch_suspend_disable_irqs(vo
 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
 
@@ -340,6 +341,7 @@ static inline bool pm_suspend_via_s2idle
 
 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) {}
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -191,6 +191,40 @@ static ssize_t mem_sleep_store(struct ko
 power_attr(mem_sleep);
 #endif /* CONFIG_SUSPEND */
 
+#ifdef CONFIG_SUSPEND
+/*
+ * 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
 int pm_test_level = TEST_NONE;
 
@@ -769,6 +803,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,
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -300,4 +300,18 @@ Description:
 		attempt.
 
 		Using this sysfs file will override any values that were
-		set using the kernel command line for disk offset.
\ No newline at end of file
+		set using the kernel command line for disk offset.
+
+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.

[PATCH v2 2/2] PM: Change CONFIG_SUSPEND_SKIP_SYNC to CONFIG_SKIP_SYNC_ON_SUSPEND

[Jonas Meurer]

Rename the build-time switch CONFIG_SUSPEND_SKIP_SYNC to
CONFIG_SKIP_SYNC_ON_SUSPEND and slightly change its behaviour. 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                  |    7 +++++--
 kernel/power/main.c                   |    2 +-
 kernel/power/suspend.c                |    2 +-
 4 files changed, 11 insertions(+), 7 deletions(-)

--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
 	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);
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -19,7 +19,7 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
-config SUSPEND_SKIP_SYNC
+config SKIP_SYNC_ON_SUSPEND
 	bool "Skip kernel's sys_sync() on suspend to RAM/standby"
 	depends on SUSPEND
 	depends on EXPERT
@@ -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
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -198,7 +198,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_SKIP_SYNC_ON_SUSPEND);
 
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -312,6 +312,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 "SKIP_SYNC_ON_SUSPEND".

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

[Jonas Meurer]

Hi Rafael and linux-pm maintainers,

sorry for the noise, but again: is there a chance to get a brief review
of my patchset?

Probably it was a bad idea to rename the build-time flag, right? Should
I revert that part of the patch?

Cheers,
 jonas

Jonas Meurer:
> Hi Rafael and linux-pm maintainers,
> 
> thanks a lot for your feedback, it's much appreciated!
> 
> Rafael J. Wysocki:
>>> This patch adds a run-time switch at `/sys/power/suspend_sync`.
>>
>> I'd prefer "sync_on_suspend".
> 
> Agreed and changed.
> 
>>> 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.
>>>
>>> Since this is my first patch against the Linux kernel and I don't
>>> consider it ready for inclusion yet, I decided to send it to pm-linux
>>> and the PM subsystem maintainers only first. Would be very glad if you
>>> could take a look and comment on it :)
>>>
>>> Some questions:
>>>
>>> * There already is a build-time config flag[2] for en- or disabling the
>>>   sync() in suspend.c. Is it acceptable to have both a build-time *and*
>>>   a *run-time* switch? Or would a run-time switch have to replace the
>>>   build-time switch? If so, a direct question to Rafael, as you added
>>>   the build-time flag: Would that be ok for you?
>>
>> If there is a run-time knob to disable the syncing, the only reason for
>> the config option to be there will be to set the default value of that.
> 
> Makes sense. I changed the meaning of the build-time flag accordingly.
> 
>>> To give a bit more contect: In Debian, we're currently working[3] on
>>> support to suspend unlocked dm-crypt devices before system suspend.
>>> During that work, we realized that the final sync() from Linux Kernel
>>> system suspend implementation can lead to a dead lock.
>>
>> That's also true for FUSE filesystems I think and please note that this isn't
>> going to work with hibernation (in which case filesystems are synced
>> regardless).
> 
> In my opinion, hibernation doesn't matter much. Since the memory is
> powered off on hibernation anyway, there's no reason to luksSuspend the
> devices beforehands, don't you think so?
> 
>> The changes look reasonable to me.
> 
> Glad to read. I'll send a second version of the patches as replies soon
> after this mail. How do we go on from here? Could you imagine to review
> them again and sign them afterwards? Or am I supposed to send them to
> the lkml and wait for feedback before getting more signers? As written,
> I'm new to the Linux Kernel development process and not sure what's the
> logical next step to get the patches merged.
> 
> Thanks again for your help!
> 
> Cheers
>  jonas
> 
> 

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

[Rafael J. Wysocki]

On Mon, Oct 21, 2019 at 12:47 PM Jonas Meurer <jonas@freesources.org> wrote:
>
> Hi Rafael and linux-pm maintainers,
>
> sorry for the noise, but again: is there a chance to get a brief review
> of my patchset?
>
> Probably it was a bad idea to rename the build-time flag, right? Should
> I revert that part of the patch?

Sorry for the delay, I'll get to your patches in the next couple of days.

Thanks,
Rafael

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

[Jonas Meurer]

Hi Rafael,

Rafael J. Wysocki:
> On Mon, Oct 21, 2019 at 12:47 PM Jonas Meurer <jonas@freesources.org> wrote:
>> Hi Rafael and linux-pm maintainers,
>>
>> sorry for the noise, but again: is there a chance to get a brief review
>> of my patchset?
>>
>> Probably it was a bad idea to rename the build-time flag, right? Should
>> I revert that part of the patch?
> 
> Sorry for the delay, I'll get to your patches in the next couple of days.

No worries. Thanks a lot for looking into it. It's no problem at all for
me/us if it takes a few more days. Just wanted to make sure that it
doesn't get lost.

If you find a minute to give a quick comment on whether I should revert
the renaming of build-time flag CONFIG_SUSPEND_SKIP_SYNC to
CONFIG_SKIP_SYNC_ON_SUSPEND, then I could do that in advance to your
thorough review.

Cheers,
 jonas

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

[Pavel Machek]

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

Hi!

> Hi Rafael and linux-pm maintainers,
> 
> sorry for the noise, but again: is there a chance to get a brief review
> of my patchset?
> 
> Probably it was a bad idea to rename the build-time flag, right? Should
> I revert that part of the patch?

I don't like adding more and more knobs.

We should not have added that compile-time option, either.

Perhaps it is time to declare that if the user wants the data to be
synced, he just does sys_sync() himself?

(Yes, that will mean tiny ammount of dirty data created between
sys_sync() and suspend to be unwritten, but...)

(Besides, if you add a runtime option to avoid deadlocks, you still
have not fixed the deadlocks...)

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jonas Meurer]

Hi Pavel,

thanks for your comment.

Pavel Machek:
>> sorry for the noise, but again: is there a chance to get a brief review
>> of my patchset?
>>
>> Probably it was a bad idea to rename the build-time flag, right? Should
>> I revert that part of the patch?
> 
> I don't like adding more and more knobs.
> 
> We should not have added that compile-time option, either.
> 
> Perhaps it is time to declare that if the user wants the data to be
> synced, he just does sys_sync() himself?

Mh, I don't see why you would want to do that? In most standard cases, I
think it's perfectly reasonable that the kernel takes care of a final
sync() before doing suspend to RAM in order to prevent potential data
loss when the system looses power during suspend mode.

> (Yes, that will mean tiny ammount of dirty data created between
> sys_sync() and suspend to be unwritten, but...)
> 
> (Besides, if you add a runtime option to avoid deadlocks, you still
> have not fixed the deadlocks...)

The deadlock only happens if something outside the kernel suspends the
block devices *before* doing suspend to RAM. In this edge case, the
runtime option would help a lot.

Besides, what you suggest probably requires a lot more discussion than
adding a runtime option, no? Would you be ok with adding the runtime
switch now and discuss the removal of sync() from suspend function
altogether later?

Cheers
 jonas

Re: [PATCH v3 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(-)

--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -575,7 +575,7 @@ static int enter_state(suspend_state_t s
 	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);
--- 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
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -198,7 +198,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)
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -312,6 +312,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".

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

[Jonas Meurer]

Hi Rafael, hi Pavel,

Jonas Meurer:
> Rafael J. Wysocki:
>> On Mon, Oct 21, 2019 at 12:47 PM Jonas Meurer <jonas@freesources.org> wrote:
>>> Hi Rafael and linux-pm maintainers,
>>>
>>> sorry for the noise, but again: is there a chance to get a brief review
>>> of my patchset?
>>>
>>> Probably it was a bad idea to rename the build-time flag, right? Should
>>> I revert that part of the patch?
>>
>> Sorry for the delay, I'll get to your patches in the next couple of days.
> 
> No worries. Thanks a lot for looking into it. It's no problem at all for
> me/us if it takes a few more days. Just wanted to make sure that it
> doesn't get lost.
> 
> If you find a minute to give a quick comment on whether I should revert
> the renaming of build-time flag CONFIG_SUSPEND_SKIP_SYNC to
> CONFIG_SKIP_SYNC_ON_SUSPEND, then I could do that in advance to your
> thorough review.

I went ahead now and reverted the renaming of build-time flag
CONFIG_SUSPEND_SKIP_SYNC[1]. There's no reason to do so and it breaks
backwards-compability.

Rafael, could you take a look at the patches anytime soon? I'd like to
propose them for inclusion into the Linux Kernel within the next weeks.
Again the question: would you sign them (if you consider them sensible)?
It's my first Linux Kernel contribution, so I'm unsure about the
process. My understanding is that a subsystem maintainer should approve
the patches first before they can be proposed for upstream integration,
right?

Cheers
 jonas

[1] See [PATCH v3 2/2] PM: CONFIG_SUSPEND_SKIP_SYNC sets default for
    '/sys/power/sync_on_suspend'

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

[Jonas Meurer]

Hi Rafael,

Jonas Meurer:
> Jonas Meurer:
>> Rafael J. Wysocki:
>>> On Mon, Oct 21, 2019 at 12:47 PM Jonas Meurer <jonas@freesources.org> wrote:
>>>> Hi Rafael and linux-pm maintainers,
>>>>
>>>> sorry for the noise, but again: is there a chance to get a brief review
>>>> of my patchset?
>>>>
>>>> Probably it was a bad idea to rename the build-time flag, right? Should
>>>> I revert that part of the patch?
>>>
>>> Sorry for the delay, I'll get to your patches in the next couple of days.
>>
>> No worries. Thanks a lot for looking into it. It's no problem at all for
>> me/us if it takes a few more days. Just wanted to make sure that it
>> doesn't get lost.
>>
>> If you find a minute to give a quick comment on whether I should revert
>> the renaming of build-time flag CONFIG_SUSPEND_SKIP_SYNC to
>> CONFIG_SKIP_SYNC_ON_SUSPEND, then I could do that in advance to your
>> thorough review.
> 
> I went ahead now and reverted the renaming of build-time flag
> CONFIG_SUSPEND_SKIP_SYNC[1]. There's no reason to do so and it breaks
> backwards-compability.
> 
> Rafael, could you take a look at the patches anytime soon? I'd like to
> propose them for inclusion into the Linux Kernel within the next weeks.
> Again the question: would you sign them (if you consider them sensible)?
> It's my first Linux Kernel contribution, so I'm unsure about the
> process. My understanding is that a subsystem maintainer should approve
> the patches first before they can be proposed for upstream integration,
> right?

Just another friendly reminder: do you think you find time to look into
my patchset anytime soon? Or shall I re-send them with lkml in the loop
as they are?

Kind regards
 jonas

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

[Yannik Sembritzki]

Hi Jonas,

thanks for the patch, it looks promising to me.

I'd suggest you send it to lkml for additional feedback.

Best regards
Yannik

On 12.11.19 12:00, Jonas Meurer wrote:
> Hi Rafael,
>
> Jonas Meurer:
>> Jonas Meurer:
>>> Rafael J. Wysocki:
>>>> On Mon, Oct 21, 2019 at 12:47 PM Jonas Meurer <jonas@freesources.org> wrote:
>>>>> Hi Rafael and linux-pm maintainers,
>>>>>
>>>>> sorry for the noise, but again: is there a chance to get a brief review
>>>>> of my patchset?
>>>>>
>>>>> Probably it was a bad idea to rename the build-time flag, right? Should
>>>>> I revert that part of the patch?
>>>> Sorry for the delay, I'll get to your patches in the next couple of days.
>>> No worries. Thanks a lot for looking into it. It's no problem at all for
>>> me/us if it takes a few more days. Just wanted to make sure that it
>>> doesn't get lost.
>>>
>>> If you find a minute to give a quick comment on whether I should revert
>>> the renaming of build-time flag CONFIG_SUSPEND_SKIP_SYNC to
>>> CONFIG_SKIP_SYNC_ON_SUSPEND, then I could do that in advance to your
>>> thorough review.
>> I went ahead now and reverted the renaming of build-time flag
>> CONFIG_SUSPEND_SKIP_SYNC[1]. There's no reason to do so and it breaks
>> backwards-compability.
>>
>> Rafael, could you take a look at the patches anytime soon? I'd like to
>> propose them for inclusion into the Linux Kernel within the next weeks.
>> Again the question: would you sign them (if you consider them sensible)?
>> It's my first Linux Kernel contribution, so I'm unsure about the
>> process. My understanding is that a subsystem maintainer should approve
>> the patches first before they can be proposed for upstream integration,
>> right?
> Just another friendly reminder: do you think you find time to look into
> my patchset anytime soon? Or shall I re-send them with lkml in the loop
> as they are?
>
> Kind regards
>  jonas
>

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

[Jonas Meurer]

Hi Yannik,

Yannik Sembritzki:
> thanks for the patch, it looks promising to me.
> 
> I'd suggest you send it to lkml for additional feedback.

Thanks a lot for your feedback. I have done so now.

Cheers
 jonas