[PATCH] PM / suspend: move userspace freeze before disk sync

[PATCH] PM / suspend: move userspace freeze before disk sync

[Sean Nyekjaer]

This fixes an issue where data remains unwritten before suspend.
UBI tried to write data, before the mtd device had been unsuspended.

Seen on an iMX6ULL board fitted with rawnand flash.

Before:
PM: suspend entry (deep)
Filesystems sync: 0.088 seconds
Freezing user space processes ... (elapsed 0.000 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
PM: suspend devices took 0.030 seconds
Disabling non-boot CPUs ...
ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 1762:94208, written 0 bytes

After:
PM: suspend entry (deep)
Freezing user space processes ... (elapsed 0.001 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
Filesystems sync: 0.060 seconds
printk: Suspending console(s) (use no_console_suspend to debug)
PM: suspend devices took 0.040 seconds
Disabling non-boot CPUs ...
PM: resume devices took 0.100 seconds

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Some explanation/discussion here:
http://lists.infradead.org/pipermail/linux-mtd/2021-July/087390.html

 kernel/power/suspend.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 87331565e505..4ee3fcbb0e7a 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -562,6 +562,12 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
+	pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
+	pm_suspend_clear_flags();
+	error = suspend_prepare(state);
+	if (error)
+		goto Unlock;
+
 #ifndef CONFIG_SUSPEND_SKIP_SYNC
 	trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 	pr_info("Syncing filesystems ... ");
@@ -570,12 +576,6 @@ static int enter_state(suspend_state_t state)
 	trace_suspend_resume(TPS("sync_filesystems"), 0, false);
 #endif
 
-	pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
-	pm_suspend_clear_flags();
-	error = suspend_prepare(state);
-	if (error)
-		goto Unlock;
-
 	if (suspend_test(TEST_FREEZER))
 		goto Finish;
 
-- 
2.33.0

Re: [PATCH] PM / suspend: move userspace freeze before disk sync

[Rafael J. Wysocki]

On Wed, Sep 1, 2021 at 10:35 AM Sean Nyekjaer <sean@geanix.com> wrote:
>
> This fixes an issue where data remains unwritten before suspend.

Well, it doesn't make the issue go away, it just hides it for you and
it may very well expose it for someone else.

In particular, note that you can compile the kernel with
CONFIG_SUSPEND_SKIP_SYNC set which makes it not sync at all during
system-wide suspend and this is a valid case expected to work.  It
looks like this case doesn't work for you.

> UBI tried to write data, before the mtd device had been unsuspended.

Well, that's the real problem and it can happen regardless of whether
or not this patch is there.

> Seen on an iMX6ULL board fitted with rawnand flash.
>
> Before:
> PM: suspend entry (deep)
> Filesystems sync: 0.088 seconds
> Freezing user space processes ... (elapsed 0.000 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> printk: Suspending console(s) (use no_console_suspend to debug)
> PM: suspend devices took 0.030 seconds
> Disabling non-boot CPUs ...
> ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 1762:94208, written 0 bytes
>
> After:
> PM: suspend entry (deep)
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> Filesystems sync: 0.060 seconds
> printk: Suspending console(s) (use no_console_suspend to debug)
> PM: suspend devices took 0.040 seconds
> Disabling non-boot CPUs ...
> PM: resume devices took 0.100 seconds
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>
> Some explanation/discussion here:
> http://lists.infradead.org/pipermail/linux-mtd/2021-July/087390.html
>
>  kernel/power/suspend.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 87331565e505..4ee3fcbb0e7a 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -562,6 +562,12 @@ static int enter_state(suspend_state_t state)
>         if (state == PM_SUSPEND_TO_IDLE)
>                 s2idle_begin();
>
> +       pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> +       pm_suspend_clear_flags();
> +       error = suspend_prepare(state);
> +       if (error)
> +               goto Unlock;
> +
>  #ifndef CONFIG_SUSPEND_SKIP_SYNC
>         trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>         pr_info("Syncing filesystems ... ");
> @@ -570,12 +576,6 @@ static int enter_state(suspend_state_t state)
>         trace_suspend_resume(TPS("sync_filesystems"), 0, false);
>  #endif
>
> -       pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> -       pm_suspend_clear_flags();
> -       error = suspend_prepare(state);
> -       if (error)
> -               goto Unlock;
> -
>         if (suspend_test(TEST_FREEZER))
>                 goto Finish;
>
> --
> 2.33.0
>

Re: [PATCH] PM / suspend: move userspace freeze before disk sync

[Sean Nyekjaer]

On Thu, Sep 02, 2021 at 06:28:14PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 1, 2021 at 10:35 AM Sean Nyekjaer <sean@geanix.com> wrote:
> >
> > This fixes an issue where data remains unwritten before suspend.
> 
> Well, it doesn't make the issue go away, it just hides it for you and
> it may very well expose it for someone else.

OK

> 
> In particular, note that you can compile the kernel with
> CONFIG_SUSPEND_SKIP_SYNC set which makes it not sync at all during
> system-wide suspend and this is a valid case expected to work.  It
> looks like this case doesn't work for you.
> 
> > UBI tried to write data, before the mtd device had been unsuspended.
> 
> Well, that's the real problem and it can happen regardless of whether
> or not this patch is there.

How to fix it then?
We can't live with devices that bricks on unsuspend :/

/Sean

> 
> > Seen on an iMX6ULL board fitted with rawnand flash.
> >
> > Before:
> > PM: suspend entry (deep)
> > Filesystems sync: 0.088 seconds
> > Freezing user space processes ... (elapsed 0.000 seconds) done.
> > OOM killer disabled.
> > Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> > printk: Suspending console(s) (use no_console_suspend to debug)
> > PM: suspend devices took 0.030 seconds
> > Disabling non-boot CPUs ...
> > ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 1762:94208, written 0 bytes
> >
> > After:
> > PM: suspend entry (deep)
> > Freezing user space processes ... (elapsed 0.001 seconds) done.
> > OOM killer disabled.
> > Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> > Filesystems sync: 0.060 seconds
> > printk: Suspending console(s) (use no_console_suspend to debug)
> > PM: suspend devices took 0.040 seconds
> > Disabling non-boot CPUs ...
> > PM: resume devices took 0.100 seconds
> >
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Re: [PATCH] PM / suspend: move userspace freeze before disk sync

[Miquel Raynal]

Hi Sean,

sean@geanix.com wrote on Fri, 3 Sep 2021 10:17:51 +0200:

> On Thu, Sep 02, 2021 at 06:28:14PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Sep 1, 2021 at 10:35 AM Sean Nyekjaer <sean@geanix.com> wrote:  
> > >
> > > This fixes an issue where data remains unwritten before suspend.  
> > 
> > Well, it doesn't make the issue go away, it just hides it for you and
> > it may very well expose it for someone else.  
> 
> OK
> 
> > 
> > In particular, note that you can compile the kernel with
> > CONFIG_SUSPEND_SKIP_SYNC set which makes it not sync at all during
> > system-wide suspend and this is a valid case expected to work.  It
> > looks like this case doesn't work for you.
> >   
> > > UBI tried to write data, before the mtd device had been unsuspended.  
> > 
> > Well, that's the real problem and it can happen regardless of whether
> > or not this patch is there.  
> 
> How to fix it then?
> We can't live with devices that bricks on unsuspend :/

We then need to focus on UBI/UBIFS implementation (Richard is currently
off), maybe there is more logic than just a basic retry to implement.

Thanks,
Miquèl