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

[Jonas Meurer <jonas@freesources.org>]

[-- 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 --]