[RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Freeze all filesystems during system suspend and (kernel-driven)
hibernation by calling freeze_supers() for all superblocks and thaw
them during the subsequent resume with the help of thaw_supers().

This makes filesystems stay in a consistent state in case something
goes wrong between system suspend (or hibernation) and the subsequent
resume (e.g. journal replays won't be necessary in those cases).  In
particular, this should help to solve a long-standing issue that, in
some cases, during resume from hibernation the boot loader causes the
journal to be replied for the filesystem containing the kernel image
and/or initrd causing it to become inconsistent with the information
stored in the hibernation image.

The user-space-driven hibernation (s2disk) is not covered by this
change, because the freezing of filesystems prevents s2disk from
accessing device special files it needs to do its job.

This change is based on earlier work by Nigel Cunningham.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h       |    3 +
 kernel/power/hibernate.c |   11 +++++--
 kernel/power/power.h     |   23 --------------
 kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
 5 files changed, 128 insertions(+), 24 deletions(-)

Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -210,6 +210,7 @@ struct inodes_stat_t {
 #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
 #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
+#define MS_FROZEN	(1<<25) /* Frozen filesystem */
 #define MS_NOSEC	(1<<28)
 #define MS_BORN		(1<<29)
 #define MS_ACTIVE	(1<<30)
@@ -2501,6 +2502,8 @@ extern void drop_super(struct super_bloc
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);
+extern int freeze_supers(void);
+extern void thaw_supers(void);
 
 extern int dcache_dir_open(struct inode *, struct file *);
 extern int dcache_dir_close(struct inode *, struct file *);
Index: linux/fs/super.c
===================================================================
--- linux.orig/fs/super.c
+++ linux/fs/super.c
@@ -594,6 +594,79 @@ void iterate_supers_type(struct file_sys
 EXPORT_SYMBOL(iterate_supers_type);
 
 /**
+ *	thaw_supers - call thaw_super() for all superblocks
+ */
+void thaw_supers(void)
+{
+	struct super_block *sb, *p = NULL;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (list_empty(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		if (sb->s_flags & MS_FROZEN) {
+			thaw_super(sb);
+			sb->s_flags &= ~MS_FROZEN;
+		}
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+}
+
+/**
+ *	freeze_supers - call freeze_super() for all superblocks
+ */
+int freeze_supers(void)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	/*
+	 * Freeze in reverse order so filesystems depending on others are
+	 * frozen in the right order (eg. loopback on ext3).
+	 */
+	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
+		if (list_empty(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
+		    && !(sb->s_flags & MS_RDONLY)) {
+			error = freeze_super(sb);
+			if (!error)
+				sb->s_flags |= MS_FROZEN;
+		}
+
+		spin_lock(&sb_lock);
+		if (error)
+			break;
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	if (error)
+		thaw_supers();
+
+	return error;
+}
+
+
+/**
  *	get_super - get the superblock of a device
  *	@bdev: device to get the superblock for
  *	
Index: linux/kernel/power/power.h
===================================================================
--- linux.orig/kernel/power/power.h
+++ linux/kernel/power/power.h
@@ -1,3 +1,4 @@
+#include <linux/fs.h>
 #include <linux/suspend.h>
 #include <linux/suspend_ioctls.h>
 #include <linux/utsname.h>
@@ -227,25 +228,3 @@ enum {
 #define TEST_MAX	(__TEST_AFTER_LAST - 1)
 
 extern int pm_test_level;
-
-#ifdef CONFIG_SUSPEND_FREEZER
-static inline int suspend_freeze_processes(void)
-{
-	int error = freeze_processes();
-	return error ? : freeze_kernel_threads();
-}
-
-static inline void suspend_thaw_processes(void)
-{
-	thaw_processes();
-}
-#else
-static inline int suspend_freeze_processes(void)
-{
-	return 0;
-}
-
-static inline void suspend_thaw_processes(void)
-{
-}
-#endif
Index: linux/kernel/power/suspend.c
===================================================================
--- linux.orig/kernel/power/suspend.c
+++ linux/kernel/power/suspend.c
@@ -29,6 +29,48 @@
 
 #include "power.h"
 
+#ifdef CONFIG_SUSPEND_FREEZER
+
+static inline int suspend_freeze_processes(void)
+{
+	int error;
+
+	error = freeze_processes();
+	if (error)
+		return error;
+
+	error = freeze_supers();
+	if (error) {
+		thaw_processes();
+		return error;
+	}
+
+	error = freeze_kernel_threads();
+	if (error)
+		thaw_supers();
+
+	return error;
+}
+
+static inline void suspend_thaw_processes(void)
+{
+	thaw_supers();
+	thaw_processes();
+}
+
+#else /* !CONFIG_SUSPEND_FREEZER */
+
+static inline int suspend_freeze_processes(void)
+{
+	return 0;
+}
+
+static inline void suspend_thaw_processes(void)
+{
+}
+
+#endif /* !CONFIG_SUSPEND_FREEZER */
+
 const char *const pm_states[PM_SUSPEND_MAX] = {
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
Index: linux/kernel/power/hibernate.c
===================================================================
--- linux.orig/kernel/power/hibernate.c
+++ linux/kernel/power/hibernate.c
@@ -626,12 +626,17 @@ int hibernate(void)
 	if (error)
 		goto Finish;
 
-	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
+	error = freeze_supers();
 	if (error)
 		goto Thaw;
+
+	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
+	if (error)
+		goto Thaw_fs;
+
 	if (freezer_test_done) {
 		freezer_test_done = false;
-		goto Thaw;
+		goto Thaw_fs;
 	}
 
 	if (in_suspend) {
@@ -655,6 +660,8 @@ int hibernate(void)
 		pr_debug("PM: Image restored successfully.\n");
 	}
 
+ Thaw_fs:
+	thaw_supers();
  Thaw:
 	thaw_processes();
  Finish:

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Nigel Cunningham]

Hi.

On 29/01/12 00:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image
> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.
> 
> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.
> 
> This change is based on earlier work by Nigel Cunningham.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h       |    3 +
>  kernel/power/hibernate.c |   11 +++++--
>  kernel/power/power.h     |   23 --------------
>  kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
>  5 files changed, 128 insertions(+), 24 deletions(-)
> 
> Index: linux/include/linux/fs.h
> ===================================================================
> --- linux.orig/include/linux/fs.h
> +++ linux/include/linux/fs.h
> @@ -210,6 +210,7 @@ struct inodes_stat_t {
>  #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
>  #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
>  #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
> +#define MS_FROZEN	(1<<25) /* Frozen filesystem */
>  #define MS_NOSEC	(1<<28)
>  #define MS_BORN		(1<<29)
>  #define MS_ACTIVE	(1<<30)
> @@ -2501,6 +2502,8 @@ extern void drop_super(struct super_bloc
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);
>  extern void iterate_supers_type(struct file_system_type *,
>  			        void (*)(struct super_block *, void *), void *);
> +extern int freeze_supers(void);
> +extern void thaw_supers(void);
>  
>  extern int dcache_dir_open(struct inode *, struct file *);
>  extern int dcache_dir_close(struct inode *, struct file *);
> Index: linux/fs/super.c
> ===================================================================
> --- linux.orig/fs/super.c
> +++ linux/fs/super.c
> @@ -594,6 +594,79 @@ void iterate_supers_type(struct file_sys
>  EXPORT_SYMBOL(iterate_supers_type);
>  
>  /**
> + *	thaw_supers - call thaw_super() for all superblocks
> + */
> +void thaw_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (list_empty(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_flags & MS_FROZEN) {
> +			thaw_super(sb);
> +			sb->s_flags &= ~MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +}

It might be helpful to explain why you delay calling __put_super (I
can't see why, though I realise that fs/super.c does it this way too),
and perhaps also where the balancing get is (it's not obvious that
s->count++ is the open coded get).

> +
> +/**
> + *	freeze_supers - call freeze_super() for all superblocks
> + */
> +int freeze_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +	int error = 0;
> +
> +	spin_lock(&sb_lock);
> +	/*
> +	 * Freeze in reverse order so filesystems depending on others are
> +	 * frozen in the right order (eg. loopback on ext3).
> +	 */
> +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> +		if (list_empty(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
> +		    && !(sb->s_flags & MS_RDONLY)) {
> +			error = freeze_super(sb);

No chance of a race against another caller of freeze_super, right?
(Userspace is already frozen and kernel threads aren't going to invoke
this without pushing). Might be good to document that there's no locking
in freeze_super anyway?

> +			if (!error)
> +				sb->s_flags |= MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (error)
> +			break;
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +
> +	if (error)
> +		thaw_supers();
> +
> +	return error;
> +}
> +
> +
> +/**
>   *	get_super - get the superblock of a device
>   *	@bdev: device to get the superblock for
>   *	
> Index: linux/kernel/power/power.h
> ===================================================================
> --- linux.orig/kernel/power/power.h
> +++ linux/kernel/power/power.h
> @@ -1,3 +1,4 @@
> +#include <linux/fs.h>
>  #include <linux/suspend.h>
>  #include <linux/suspend_ioctls.h>
>  #include <linux/utsname.h>
> @@ -227,25 +228,3 @@ enum {
>  #define TEST_MAX	(__TEST_AFTER_LAST - 1)
>  
>  extern int pm_test_level;
> -
> -#ifdef CONFIG_SUSPEND_FREEZER
> -static inline int suspend_freeze_processes(void)
> -{
> -	int error = freeze_processes();
> -	return error ? : freeze_kernel_threads();
> -}
> -
> -static inline void suspend_thaw_processes(void)
> -{
> -	thaw_processes();
> -}
> -#else
> -static inline int suspend_freeze_processes(void)
> -{
> -	return 0;
> -}
> -
> -static inline void suspend_thaw_processes(void)
> -{
> -}
> -#endif
> Index: linux/kernel/power/suspend.c
> ===================================================================
> --- linux.orig/kernel/power/suspend.c
> +++ linux/kernel/power/suspend.c
> @@ -29,6 +29,48 @@
>  
>  #include "power.h"
>  
> +#ifdef CONFIG_SUSPEND_FREEZER
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	int error;
> +
> +	error = freeze_processes();
> +	if (error)
> +		return error;
> +
> +	error = freeze_supers();
> +	if (error) {
> +		thaw_processes();
> +		return error;
> +	}
> +
> +	error = freeze_kernel_threads();
> +	if (error)
> +		thaw_supers();

Comment why there's no matching thaw_processes() as well?

That's all from me.

Regards,

Nigel

> +
> +	return error;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +	thaw_supers();
> +	thaw_processes();
> +}
> +
> +#else /* !CONFIG_SUSPEND_FREEZER */
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	return 0;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +}
> +
> +#endif /* !CONFIG_SUSPEND_FREEZER */
> +
>  const char *const pm_states[PM_SUSPEND_MAX] = {
>  	[PM_SUSPEND_STANDBY]	= "standby",
>  	[PM_SUSPEND_MEM]	= "mem",
> Index: linux/kernel/power/hibernate.c
> ===================================================================
> --- linux.orig/kernel/power/hibernate.c
> +++ linux/kernel/power/hibernate.c
> @@ -626,12 +626,17 @@ int hibernate(void)
>  	if (error)
>  		goto Finish;
>  
> -	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	error = freeze_supers();
>  	if (error)
>  		goto Thaw;
> +
> +	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	if (error)
> +		goto Thaw_fs;
> +
>  	if (freezer_test_done) {
>  		freezer_test_done = false;
> -		goto Thaw;
> +		goto Thaw_fs;
>  	}
>  
>  	if (in_suspend) {
> @@ -655,6 +660,8 @@ int hibernate(void)
>  		pr_debug("PM: Image restored successfully.\n");
>  	}
>  
> + Thaw_fs:
> +	thaw_supers();
>   Thaw:
>  	thaw_processes();
>   Finish:
> 

-- 
Evolution (n): A hypothetical process whereby improbable
events occur with alarming frequency, order arises from chaos, and
no one is given credit.

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Martin Steigerwald]

Am Samstag, 28. Januar 2012 schrieb Rafael J. Wysocki:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image

replied => replayed

> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.

Thanks,
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Srivatsa S. Bhat]

On 01/28/2012 07:15 PM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image

> and/or initrd causing it to become inconsistent with the information

> stored in the hibernation image.
> 
> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.
> 
> This change is based on earlier work by Nigel Cunningham.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h       |    3 +
>  kernel/power/hibernate.c |   11 +++++--
>  kernel/power/power.h     |   23 --------------
>  kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
>  5 files changed, 128 insertions(+), 24 deletions(-)
> 

...

> Index: linux/kernel/power/suspend.c
> ===================================================================
> --- linux.orig/kernel/power/suspend.c
> +++ linux/kernel/power/suspend.c
> @@ -29,6 +29,48 @@
> 
>  #include "power.h"
> 
> +#ifdef CONFIG_SUSPEND_FREEZER
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	int error;
> +
> +	error = freeze_processes();
> +	if (error)
> +		return error;
> +
> +	error = freeze_supers();
> +	if (error) {
> +		thaw_processes();
> +		return error;
> +	}
> +
> +	error = freeze_kernel_threads();
> +	if (error)
> +		thaw_supers();
> +


If freezing of kernel threads fails, freeze_kernel_threads() calls
thaw_processes(), which means, even userspace processes get thawed.
So, there would be a time-window in which userspace is thawed but the
filesystems are still frozen. That is not very desirable right?

If that is right, then modifying freeze_kernel_threads() to call
thaw_kernel_threads() instead of thaw_processes() would fix it
(and of course, we would need to explicitly call thaw_processes
above).

BTW, after your patch posted at https://lkml.org/lkml/2012/1/27/501,
I very much wanted to write a patch to convert the semantics of
freeze/thaw to something like:

freeze_processes() calls thaw_processes on error.
//Both touch only userspace processes.

freeze_kernel_threads() calls thaw_kernel_threads() on error.
//Both touch only kernel threads.

Of course, such a patch would need to do a lot of fixing up at several
places, but IMHO, it would really help make the overall code more logical
and easier to understand.

I can write it up and post it soon, but then you'll have to rebase
your patch (this one) on top of that. What do you say?

Regards,
Srivatsa S. Bhat

> +	return error;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +	thaw_supers();
> +	thaw_processes();
> +}
> +
> +#else /* !CONFIG_SUSPEND_FREEZER */
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	return 0;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +}
> +
> +#endif /* !CONFIG_SUSPEND_FREEZER */
> +
>  const char *const pm_states[PM_SUSPEND_MAX] = {
>  	[PM_SUSPEND_STANDBY]	= "standby",
>  	[PM_SUSPEND_MEM]	= "mem",
> Index: linux/kernel/power/hibernate.c
> ===================================================================
> --- linux.orig/kernel/power/hibernate.c
> +++ linux/kernel/power/hibernate.c
> @@ -626,12 +626,17 @@ int hibernate(void)
>  	if (error)
>  		goto Finish;
> 
> -	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	error = freeze_supers();
>  	if (error)
>  		goto Thaw;
> +
> +	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	if (error)
> +		goto Thaw_fs;
> +
>  	if (freezer_test_done) {
>  		freezer_test_done = false;
> -		goto Thaw;
> +		goto Thaw_fs;
>  	}
> 
>  	if (in_suspend) {
> @@ -655,6 +660,8 @@ int hibernate(void)
>  		pr_debug("PM: Image restored successfully.\n");
>  	}
> 
> + Thaw_fs:
> +	thaw_supers();
>   Thaw:
>  	thaw_processes();
>   Finish:
> 

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Sunday, January 29, 2012, Srivatsa S. Bhat wrote:
> On 01/28/2012 07:15 PM, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Freeze all filesystems during system suspend and (kernel-driven)
> > hibernation by calling freeze_supers() for all superblocks and thaw
> > them during the subsequent resume with the help of thaw_supers().
> > 
> > This makes filesystems stay in a consistent state in case something
> > goes wrong between system suspend (or hibernation) and the subsequent
> > resume (e.g. journal replays won't be necessary in those cases).  In
> > particular, this should help to solve a long-standing issue that, in
> > some cases, during resume from hibernation the boot loader causes the
> > journal to be replied for the filesystem containing the kernel image
> 
> > and/or initrd causing it to become inconsistent with the information
> 
> > stored in the hibernation image.
> > 
> > The user-space-driven hibernation (s2disk) is not covered by this
> > change, because the freezing of filesystems prevents s2disk from
> > accessing device special files it needs to do its job.
> > 
> > This change is based on earlier work by Nigel Cunningham.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h       |    3 +
> >  kernel/power/hibernate.c |   11 +++++--
> >  kernel/power/power.h     |   23 --------------
> >  kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
> >  5 files changed, 128 insertions(+), 24 deletions(-)
> > 
> 
> ...
> 
> > Index: linux/kernel/power/suspend.c
> > ===================================================================
> > --- linux.orig/kernel/power/suspend.c
> > +++ linux/kernel/power/suspend.c
> > @@ -29,6 +29,48 @@
> > 
> >  #include "power.h"
> > 
> > +#ifdef CONFIG_SUSPEND_FREEZER
> > +
> > +static inline int suspend_freeze_processes(void)
> > +{
> > +	int error;
> > +
> > +	error = freeze_processes();
> > +	if (error)
> > +		return error;
> > +
> > +	error = freeze_supers();
> > +	if (error) {
> > +		thaw_processes();
> > +		return error;
> > +	}
> > +
> > +	error = freeze_kernel_threads();
> > +	if (error)
> > +		thaw_supers();
> > +
> 
> 
> If freezing of kernel threads fails, freeze_kernel_threads() calls
> thaw_processes(), which means, even userspace processes get thawed.
> So, there would be a time-window in which userspace is thawed but the
> filesystems are still frozen. That is not very desirable right?

No, it is not.  I overlooked that, thanks!

> If that is right, then modifying freeze_kernel_threads() to call
> thaw_kernel_threads() instead of thaw_processes() would fix it
> (and of course, we would need to explicitly call thaw_processes
> above).
> 
> BTW, after your patch posted at https://lkml.org/lkml/2012/1/27/501,
> I very much wanted to write a patch to convert the semantics of
> freeze/thaw to something like:
> 
> freeze_processes() calls thaw_processes on error.
> //Both touch only userspace processes.
> 
> freeze_kernel_threads() calls thaw_kernel_threads() on error.
> //Both touch only kernel threads.
> 
> Of course, such a patch would need to do a lot of fixing up at several
> places, but IMHO, it would really help make the overall code more logical
> and easier to understand.
> 
> I can write it up and post it soon, but then you'll have to rebase
> your patch (this one) on top of that. What do you say?

Please do that, it wouldn't be any problem for me to rebase the $subject
patch.

Thanks,
Rafael


> > +	return error;
> > +}
> > +
> > +static inline void suspend_thaw_processes(void)
> > +{
> > +	thaw_supers();
> > +	thaw_processes();
> > +}
> > +
> > +#else /* !CONFIG_SUSPEND_FREEZER */
> > +
> > +static inline int suspend_freeze_processes(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void suspend_thaw_processes(void)
> > +{
> > +}
> > +
> > +#endif /* !CONFIG_SUSPEND_FREEZER */
> > +
> >  const char *const pm_states[PM_SUSPEND_MAX] = {
> >  	[PM_SUSPEND_STANDBY]	= "standby",
> >  	[PM_SUSPEND_MEM]	= "mem",
> > Index: linux/kernel/power/hibernate.c
> > ===================================================================
> > --- linux.orig/kernel/power/hibernate.c
> > +++ linux/kernel/power/hibernate.c
> > @@ -626,12 +626,17 @@ int hibernate(void)
> >  	if (error)
> >  		goto Finish;
> > 
> > -	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> > +	error = freeze_supers();
> >  	if (error)
> >  		goto Thaw;
> > +
> > +	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> > +	if (error)
> > +		goto Thaw_fs;
> > +
> >  	if (freezer_test_done) {
> >  		freezer_test_done = false;
> > -		goto Thaw;
> > +		goto Thaw_fs;
> >  	}
> > 
> >  	if (in_suspend) {
> > @@ -655,6 +660,8 @@ int hibernate(void)
> >  		pr_debug("PM: Image restored successfully.\n");
> >  	}
> > 
> > + Thaw_fs:
> > +	thaw_supers();
> >   Thaw:
> >  	thaw_processes();
> >   Finish:
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Sunday, January 29, 2012, Martin Steigerwald wrote:
> Am Samstag, 28. Januar 2012 schrieb Rafael J. Wysocki:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Freeze all filesystems during system suspend and (kernel-driven)
> > hibernation by calling freeze_supers() for all superblocks and thaw
> > them during the subsequent resume with the help of thaw_supers().
> > 
> > This makes filesystems stay in a consistent state in case something
> > goes wrong between system suspend (or hibernation) and the subsequent
> > resume (e.g. journal replays won't be necessary in those cases).  In
> > particular, this should help to solve a long-standing issue that, in
> > some cases, during resume from hibernation the boot loader causes the
> > journal to be replied for the filesystem containing the kernel image
> 
> replied => replayed

Right, thanks!

Rafael

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Jan Kara]

On Sat 28-01-12 14:45:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image
> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.
> 
> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.
> 
> This change is based on earlier work by Nigel Cunningham.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h       |    3 +
>  kernel/power/hibernate.c |   11 +++++--
>  kernel/power/power.h     |   23 --------------
>  kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
>  5 files changed, 128 insertions(+), 24 deletions(-)
> 
> Index: linux/fs/super.c
> ===================================================================
> --- linux.orig/fs/super.c
> +++ linux/fs/super.c
> @@ -594,6 +594,79 @@ void iterate_supers_type(struct file_sys
>  EXPORT_SYMBOL(iterate_supers_type);
>  
>  /**
> + *	thaw_supers - call thaw_super() for all superblocks
> + */
> +void thaw_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (list_empty(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_flags & MS_FROZEN) {
> +			thaw_super(sb);
> +			sb->s_flags &= ~MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +}
  At least for thawing you can use iterate_supers() helper.

> +/**
> + *	freeze_supers - call freeze_super() for all superblocks
> + */
> +int freeze_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +	int error = 0;
> +
> +	spin_lock(&sb_lock);
> +	/*
> +	 * Freeze in reverse order so filesystems depending on others are
> +	 * frozen in the right order (eg. loopback on ext3).
> +	 */
  Ho, hum, are you sure the order in super_blocks list is the one you need?
Maybe it is but I'm not sure you are guaranteed it is. I've added Al to CC,
he'll likely have opinion on this...

> +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> +		if (list_empty(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
> +		    && !(sb->s_flags & MS_RDONLY)) {
> +			error = freeze_super(sb);
> +			if (!error)
> +				sb->s_flags |= MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (error)
> +			break;
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +
> +	if (error)
> +		thaw_supers();
> +
> +	return error;
> +}
> +
> +
> +/**
>   *	get_super - get the superblock of a device
>   *	@bdev: device to get the superblock for
>   *	

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Monday, January 30, 2012, Jan Kara wrote:
> On Sat 28-01-12 14:45:49, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Freeze all filesystems during system suspend and (kernel-driven)
> > hibernation by calling freeze_supers() for all superblocks and thaw
> > them during the subsequent resume with the help of thaw_supers().
> > 
> > This makes filesystems stay in a consistent state in case something
> > goes wrong between system suspend (or hibernation) and the subsequent
> > resume (e.g. journal replays won't be necessary in those cases).  In
> > particular, this should help to solve a long-standing issue that, in
> > some cases, during resume from hibernation the boot loader causes the
> > journal to be replied for the filesystem containing the kernel image
> > and/or initrd causing it to become inconsistent with the information
> > stored in the hibernation image.
> > 
> > The user-space-driven hibernation (s2disk) is not covered by this
> > change, because the freezing of filesystems prevents s2disk from
> > accessing device special files it needs to do its job.
> > 
> > This change is based on earlier work by Nigel Cunningham.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  fs/super.c               |   73 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h       |    3 +
> >  kernel/power/hibernate.c |   11 +++++--
> >  kernel/power/power.h     |   23 --------------
> >  kernel/power/suspend.c   |   42 +++++++++++++++++++++++++++
> >  5 files changed, 128 insertions(+), 24 deletions(-)
> > 
> > Index: linux/fs/super.c
> > ===================================================================
> > --- linux.orig/fs/super.c
> > +++ linux/fs/super.c
> > @@ -594,6 +594,79 @@ void iterate_supers_type(struct file_sys
> >  EXPORT_SYMBOL(iterate_supers_type);
> >  
> >  /**
> > + *	thaw_supers - call thaw_super() for all superblocks
> > + */
> > +void thaw_supers(void)
> > +{
> > +	struct super_block *sb, *p = NULL;
> > +
> > +	spin_lock(&sb_lock);
> > +	list_for_each_entry(sb, &super_blocks, s_list) {
> > +		if (list_empty(&sb->s_instances))
> > +			continue;
> > +		sb->s_count++;
> > +		spin_unlock(&sb_lock);
> > +
> > +		if (sb->s_flags & MS_FROZEN) {
> > +			thaw_super(sb);
> > +			sb->s_flags &= ~MS_FROZEN;
> > +		}
> > +
> > +		spin_lock(&sb_lock);
> > +		if (p)
> > +			__put_super(p);
> > +		p = sb;
> > +	}
> > +	if (p)
> > +		__put_super(p);
> > +	spin_unlock(&sb_lock);
> > +}
>   At least for thawing you can use iterate_supers() helper.

OK

> > +/**
> > + *	freeze_supers - call freeze_super() for all superblocks
> > + */
> > +int freeze_supers(void)
> > +{
> > +	struct super_block *sb, *p = NULL;
> > +	int error = 0;
> > +
> > +	spin_lock(&sb_lock);
> > +	/*
> > +	 * Freeze in reverse order so filesystems depending on others are
> > +	 * frozen in the right order (eg. loopback on ext3).
> > +	 */
>   Ho, hum, are you sure the order in super_blocks list is the one you need?
> Maybe it is but I'm not sure you are guaranteed it is.

Well, is there any way I can get the right order?

> I've added Al to CC,
> he'll likely have opinion on this...
> 
> > +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> > +		if (list_empty(&sb->s_instances))
> > +			continue;
> > +		sb->s_count++;
> > +		spin_unlock(&sb_lock);
> > +
> > +		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
> > +		    && !(sb->s_flags & MS_RDONLY)) {
> > +			error = freeze_super(sb);
> > +			if (!error)
> > +				sb->s_flags |= MS_FROZEN;
> > +		}
> > +
> > +		spin_lock(&sb_lock);
> > +		if (error)
> > +			break;
> > +		if (p)
> > +			__put_super(p);
> > +		p = sb;
> > +	}
> > +	if (p)
> > +		__put_super(p);
> > +	spin_unlock(&sb_lock);
> > +
> > +	if (error)
> > +		thaw_supers();
> > +
> > +	return error;
> > +}
> > +
> > +
> > +/**
> >   *	get_super - get the superblock of a device
> >   *	@bdev: device to get the superblock for
> >   *	

Thanks,
Rafael

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Nigel Cunningham]

Hi.

On 31/01/12 08:05, Rafael J. Wysocki wrote:
> On Monday, January 30, 2012, Jan Kara wrote:
>> On Sat 28-01-12 14:45:49, Rafael J. Wysocki wrote:
>>> +	/*
>>> +	 * Freeze in reverse order so filesystems depending on others are
>>> +	 * frozen in the right order (eg. loopback on ext3).
>>> +	 */
>>   Ho, hum, are you sure the order in super_blocks list is the one you need?
>> Maybe it is but I'm not sure you are guaranteed it is.
> 
> Well, is there any way I can get the right order?

Jan, what's the case where the reverse of mount order might be the wrong
order?

Regards,

Nigel
-- 
Evolution (n): A hypothetical process whereby improbable
events occur with alarming frequency, order arises from chaos, and
no one is given credit.

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Srivatsa S. Bhat]

On 01/30/2012 01:23 AM, Rafael J. Wysocki wrote:

> On Sunday, January 29, 2012, Srivatsa S. Bhat wrote:

[...]

>>> Index: linux/kernel/power/suspend.c
>>> ===================================================================
>>> --- linux.orig/kernel/power/suspend.c
>>> +++ linux/kernel/power/suspend.c
>>> @@ -29,6 +29,48 @@
>>>
>>>  #include "power.h"
>>>
>>> +#ifdef CONFIG_SUSPEND_FREEZER
>>> +
>>> +static inline int suspend_freeze_processes(void)
>>> +{
>>> +	int error;
>>> +
>>> +	error = freeze_processes();
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	error = freeze_supers();
>>> +	if (error) {
>>> +		thaw_processes();
>>> +		return error;
>>> +	}
>>> +
>>> +	error = freeze_kernel_threads();
>>> +	if (error)
>>> +		thaw_supers();
>>> +
>>
>>
>> If freezing of kernel threads fails, freeze_kernel_threads() calls
>> thaw_processes(), which means, even userspace processes get thawed.
>> So, there would be a time-window in which userspace is thawed but the
>> filesystems are still frozen. That is not very desirable right?
> 
> No, it is not.  I overlooked that, thanks!
> 
>> If that is right, then modifying freeze_kernel_threads() to call
>> thaw_kernel_threads() instead of thaw_processes() would fix it
>> (and of course, we would need to explicitly call thaw_processes
>> above).
>>
>> BTW, after your patch posted at https://lkml.org/lkml/2012/1/27/501,
>> I very much wanted to write a patch to convert the semantics of
>> freeze/thaw to something like:
>>
>> freeze_processes() calls thaw_processes on error.
>> //Both touch only userspace processes.
>>
>> freeze_kernel_threads() calls thaw_kernel_threads() on error.
>> //Both touch only kernel threads.
>>
>> Of course, such a patch would need to do a lot of fixing up at several
>> places, but IMHO, it would really help make the overall code more logical
>> and easier to understand.
>>
>> I can write it up and post it soon, but then you'll have to rebase
>> your patch (this one) on top of that. What do you say?
> 
> Please do that, it wouldn't be any problem for me to rebase the $subject
> patch.
> 


Hi Rafael,
I have posted the patchset (v2) at https://lkml.org/lkml/2012/1/30/530

 

Regards,
Srivatsa S. Bhat

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Jan Kara]

On Mon 30-01-12 22:05:33, Rafael J. Wysocki wrote:
> On Monday, January 30, 2012, Jan Kara wrote:
> > On Sat 28-01-12 14:45:49, Rafael J. Wysocki wrote:
> > > +/**
> > > + *	freeze_supers - call freeze_super() for all superblocks
> > > + */
> > > +int freeze_supers(void)
> > > +{
> > > +	struct super_block *sb, *p = NULL;
> > > +	int error = 0;
> > > +
> > > +	spin_lock(&sb_lock);
> > > +	/*
> > > +	 * Freeze in reverse order so filesystems depending on others are
> > > +	 * frozen in the right order (eg. loopback on ext3).
> > > +	 */
> >   Ho, hum, are you sure the order in super_blocks list is the one you need?
> > Maybe it is but I'm not sure you are guaranteed it is.
> 
> Well, is there any way I can get the right order?
  None I'm aware of :( I think your reverse order scanning is the best you
can do now as we don't track the "freeze dependency" in any way. I was just
warning that noone really guarantees this is the right order. So we should
probably at least document somewhere that super_blocks list order matters
for freeze_supers().

> > > +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> > > +		if (list_empty(&sb->s_instances))
> > > +			continue;
> > > +		sb->s_count++;
> > > +		spin_unlock(&sb_lock);
> > > +
> > > +		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
> > > +		    && !(sb->s_flags & MS_RDONLY)) {
> > > +			error = freeze_super(sb);
> > > +			if (!error)
> > > +				sb->s_flags |= MS_FROZEN;
> > > +		}
> > > +
> > > +		spin_lock(&sb_lock);
> > > +		if (error)
> > > +			break;
> > > +		if (p)
> > > +			__put_super(p);
> > > +		p = sb;
> > > +	}
> > > +	if (p)
> > > +		__put_super(p);
> > > +	spin_unlock(&sb_lock);
> > > +
> > > +	if (error)
> > > +		thaw_supers();
> > > +
> > > +	return error;
> > > +}
> > > +
> > > +
> > > +/**
> > >   *	get_super - get the superblock of a device
> > >   *	@bdev: device to get the superblock for
> > >   *	

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Jan Kara]

On Tue 31-01-12 08:10:37, Nigel Cunningham wrote:
> Hi.
> 
> On 31/01/12 08:05, Rafael J. Wysocki wrote:
> > On Monday, January 30, 2012, Jan Kara wrote:
> >> On Sat 28-01-12 14:45:49, Rafael J. Wysocki wrote:
> >>> +	/*
> >>> +	 * Freeze in reverse order so filesystems depending on others are
> >>> +	 * frozen in the right order (eg. loopback on ext3).
> >>> +	 */
> >>   Ho, hum, are you sure the order in super_blocks list is the one you need?
> >> Maybe it is but I'm not sure you are guaranteed it is.
> > 
> > Well, is there any way I can get the right order?
> 
> Jan, what's the case where the reverse of mount order might be the wrong
> order?
  After some though, I'm not able to come up with a counterexample. As I
wrote to Rafael, let's at least document that we rely on some ordering of
super_blocks list.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In

Good.

> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image
> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.

Ungood. Why is bootloader/initrd doing that? If it mounts filesystem
read/write, what is the guarantee that it will not change data on the
filesystem, breaking stuff?

Bootloaders should just not replay journals.

> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.

...so bootloaders need to be fixed, anyway.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Alan Stern]

On Wed, 1 Feb 2012, Pavel Machek wrote:

> > particular, this should help to solve a long-standing issue that, in
> > some cases, during resume from hibernation the boot loader causes the
> > journal to be replied for the filesystem containing the kernel image
> > and/or initrd causing it to become inconsistent with the information
> > stored in the hibernation image.
> 
> Ungood. Why is bootloader/initrd doing that? If it mounts filesystem
> read/write, what is the guarantee that it will not change data on the
> filesystem, breaking stuff?
> 
> Bootloaders should just not replay journals.
> 
> > The user-space-driven hibernation (s2disk) is not covered by this
> > change, because the freezing of filesystems prevents s2disk from
> > accessing device special files it needs to do its job.
> 
> ...so bootloaders need to be fixed, anyway.

I don't know about bootloaders, but from what I've heard, Linux fs
drivers (including those inside initrds) always replay the journal,
even if the filesystem is mounted read-only.  This could be considered
a bug in the filesystem code.

Alan Stern

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Nigel Cunningham]

Hi.

On 29/01/12 00:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
> 
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image
> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.
> 
> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.
> 
> This change is based on earlier work by Nigel Cunningham.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Nigel Cunningham <nigel@tuxonice.net>


(Or reviewed by or whatever :> I'm out of the loop too much, and don't
remember what the right one is!)

-- 
Evolution (n): A hypothetical process whereby improbable
events occur with alarming frequency, order arises from chaos, and
no one is given credit.

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Jamie Lokier]

Alan Stern wrote:
> On Wed, 1 Feb 2012, Pavel Machek wrote:
> 
> > > particular, this should help to solve a long-standing issue that, in
> > > some cases, during resume from hibernation the boot loader causes the
> > > journal to be replied for the filesystem containing the kernel image
> > > and/or initrd causing it to become inconsistent with the information
> > > stored in the hibernation image.
> > 
> > Ungood. Why is bootloader/initrd doing that? If it mounts filesystem
> > read/write, what is the guarantee that it will not change data on the
> > filesystem, breaking stuff?
> > 
> > Bootloaders should just not replay journals.
> > 
> > > The user-space-driven hibernation (s2disk) is not covered by this
> > > change, because the freezing of filesystems prevents s2disk from
> > > accessing device special files it needs to do its job.
> > 
> > ...so bootloaders need to be fixed, anyway.
> 
> I don't know about bootloaders, but from what I've heard, Linux fs
> drivers (including those inside initrds) always replay the journal,
> even if the filesystem is mounted read-only.  This could be considered
> a bug in the filesystem code.

Theoretically a filesystem might need replay for the bootloader to see
a non-corrupt image, even for just the files it uses.

For example if the last state was in the middle of updating the root
directory, the /boot entry in the root directory might not be reliably
found without replaying the journal.

However replaying a journal when mounted read-only should probably
track journalled blocks in memory only, not commit back to the storage.

All the best,
-- Jamie

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Jan Kara]

On Fri 10-02-12 02:52:17, Jamie Lokier wrote:
> Alan Stern wrote:
> > On Wed, 1 Feb 2012, Pavel Machek wrote:
> > 
> > > > particular, this should help to solve a long-standing issue that, in
> > > > some cases, during resume from hibernation the boot loader causes the
> > > > journal to be replied for the filesystem containing the kernel image
> > > > and/or initrd causing it to become inconsistent with the information
> > > > stored in the hibernation image.
> > > 
> > > Ungood. Why is bootloader/initrd doing that? If it mounts filesystem
> > > read/write, what is the guarantee that it will not change data on the
> > > filesystem, breaking stuff?
> > > 
> > > Bootloaders should just not replay journals.
> > > 
> > > > The user-space-driven hibernation (s2disk) is not covered by this
> > > > change, because the freezing of filesystems prevents s2disk from
> > > > accessing device special files it needs to do its job.
> > > 
> > > ...so bootloaders need to be fixed, anyway.
> > 
> > I don't know about bootloaders, but from what I've heard, Linux fs
> > drivers (including those inside initrds) always replay the journal,
> > even if the filesystem is mounted read-only.  This could be considered
> > a bug in the filesystem code.
> 
> Theoretically a filesystem might need replay for the bootloader to see
> a non-corrupt image, even for just the files it uses.
> 
> For example if the last state was in the middle of updating the root
> directory, the /boot entry in the root directory might not be reliably
> found without replaying the journal.
> 
> However replaying a journal when mounted read-only should probably
> track journalled blocks in memory only, not commit back to the storage.
  Yes, that would be nice. Although memory constraints of the bootloader
might make it tricky.

  The reason why we don't have the functionality in kernel is not so much
about memory demands but more about the complexities of implementing the
caching - we'd have to create an equivalent of dm-snapshot of the device
from the mount code to trick generic code in pagecache to loading proper
replayed data).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Sat, Jan 28, 2012 at 8:45 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> Freeze all filesystems during system suspend and (kernel-driven)
> hibernation by calling freeze_supers() for all superblocks and thaw
> them during the subsequent resume with the help of thaw_supers().
>
> This makes filesystems stay in a consistent state in case something
> goes wrong between system suspend (or hibernation) and the subsequent
> resume (e.g. journal replays won't be necessary in those cases).  In
> particular, this should help to solve a long-standing issue that, in
> some cases, during resume from hibernation the boot loader causes the
> journal to be replied for the filesystem containing the kernel image
> and/or initrd causing it to become inconsistent with the information
> stored in the hibernation image.
>
> The user-space-driven hibernation (s2disk) is not covered by this
> change, because the freezing of filesystems prevents s2disk from
> accessing device special files it needs to do its job.
>
> This change is based on earlier work by Nigel Cunningham.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Do you happen to have a version of this patch that has been rebased to the
latest 3.3 git tree?  I looked in the various archives and haven't found one
yet.  I'd like to get this into Fedora rawhide to see if some of the hibernate
issues we've been seeing clear up.

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Fri, Feb 17, 2012 at 10:41 AM, Josh Boyer <jwboyer@gmail.com> wrote:
> On Sat, Jan 28, 2012 at 8:45 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>
>> Freeze all filesystems during system suspend and (kernel-driven)
>> hibernation by calling freeze_supers() for all superblocks and thaw
>> them during the subsequent resume with the help of thaw_supers().
>>
>> This makes filesystems stay in a consistent state in case something
>> goes wrong between system suspend (or hibernation) and the subsequent
>> resume (e.g. journal replays won't be necessary in those cases).  In
>> particular, this should help to solve a long-standing issue that, in
>> some cases, during resume from hibernation the boot loader causes the
>> journal to be replied for the filesystem containing the kernel image
>> and/or initrd causing it to become inconsistent with the information
>> stored in the hibernation image.
>>
>> The user-space-driven hibernation (s2disk) is not covered by this
>> change, because the freezing of filesystems prevents s2disk from
>> accessing device special files it needs to do its job.
>>
>> This change is based on earlier work by Nigel Cunningham.
>>
>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>
> Do you happen to have a version of this patch that has been rebased to the
> latest 3.3 git tree?  I looked in the various archives and haven't found one
> yet.  I'd like to get this into Fedora rawhide to see if some of the hibernate
> issues we've been seeing clear up.

I got impatient and decided to tackle this myself.  If others would like to
look at the forward port below, I'd appreciate it.

josh

commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Fri Feb 17 12:42:08 2012 -0500

    Freeze all filesystems during system suspend and (kernel-driven)
    hibernation by calling freeze_supers() for all superblocks and thaw
    them during the subsequent resume with the help of thaw_supers().

    This makes filesystems stay in a consistent state in case something
    goes wrong between system suspend (or hibernation) and the subsequent
    resume (e.g. journal replays won't be necessary in those cases).  In
    particular, this should help to solve a long-standing issue that, in
    some cases, during resume from hibernation the boot loader causes the
    journal to be replied for the filesystem containing the kernel image
    and/or initrd causing it to become inconsistent with the information
    stored in the hibernation image.

    The user-space-driven hibernation (s2disk) is not covered by this
    change, because the freezing of filesystems prevents s2disk from
    accessing device special files it needs to do its job.

    This change is based on earlier work by Nigel Cunningham.

    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

    Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>

diff --git a/fs/super.c b/fs/super.c
index 6015c02..c8057fa 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -594,6 +594,79 @@ void iterate_supers_type(struct file_system_type *type,
 EXPORT_SYMBOL(iterate_supers_type);

 /**
+ *	thaw_supers - call thaw_super() for all superblocks
+ */
+void thaw_supers(void)
+{
+	struct super_block *sb, *p = NULL;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		if (sb->s_flags & MS_FROZEN) {
+			thaw_super(sb);
+			sb->s_flags &= ~MS_FROZEN;
+		}
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+}
+
+/**
+ *	freeze_supers - call freeze_super() for all superblocks
+ */
+int freeze_supers(void)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	/*
+	 * Freeze in reverse order so filesystems depending on others are
+	 * frozen in the right order (eg. loopback on ext3).
+	 */
+	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
+		    && !(sb->s_flags & MS_RDONLY)) {
+			error = freeze_super(sb);
+			if (!error)
+				sb->s_flags |= MS_FROZEN;
+		}
+
+		spin_lock(&sb_lock);
+		if (error)
+			break;
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	if (error)
+		thaw_supers();
+
+	return error;
+}
+
+
+/**
  *	get_super - get the superblock of a device
  *	@bdev: device to get the superblock for
  *	
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 386da09..a164f4a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -210,6 +210,7 @@ struct inodes_stat_t {
 #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
 #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
+#define MS_FROZEN	(1<<25) /* Frozen filesystem */
 #define MS_NOSEC	(1<<28)
 #define MS_BORN		(1<<29)
 #define MS_ACTIVE	(1<<30)
@@ -2501,6 +2502,8 @@ extern void drop_super(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);
+extern int freeze_supers(void);
+extern void thaw_supers(void);

 extern int dcache_dir_open(struct inode *, struct file *);
 extern int dcache_dir_close(struct inode *, struct file *);
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 6d6d288..492fc62 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -626,12 +626,17 @@ int hibernate(void)
 	if (error)
 		goto Finish;

-	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
+	error = freeze_supers();
 	if (error)
 		goto Thaw;
+
+	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
+	if (error)
+		goto Thaw_fs;
+
 	if (freezer_test_done) {
 		freezer_test_done = false;
-		goto Thaw;
+		goto Thaw_fs;
 	}

 	if (in_suspend) {
@@ -655,6 +660,8 @@ int hibernate(void)
 		pr_debug("PM: Image restored successfully.\n");
 	}

+ Thaw_fs:
+	thaw_supers();
  Thaw:
 	thaw_processes();
  Finish:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 21724ee..40d6f64 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -1,3 +1,4 @@
+#include <linux/fs.h>
 #include <linux/suspend.h>
 #include <linux/suspend_ioctls.h>
 #include <linux/utsname.h>
@@ -227,45 +228,3 @@ enum {
 #define TEST_MAX	(__TEST_AFTER_LAST - 1)

 extern int pm_test_level;
-
-#ifdef CONFIG_SUSPEND_FREEZER
-static inline int suspend_freeze_processes(void)
-{
-	int error;
-
-	error = freeze_processes();
-
-	/*
-	 * freeze_processes() automatically thaws every task if freezing
-	 * fails. So we need not do anything extra upon error.
-	 */
-	if (error)
-		goto Finish;
-
-	error = freeze_kernel_threads();
-
-	/*
-	 * freeze_kernel_threads() thaws only kernel threads upon freezing
-	 * failure. So we have to thaw the userspace tasks ourselves.
-	 */
-	if (error)
-		thaw_processes();
-
- Finish:
-	return error;
-}
-
-static inline void suspend_thaw_processes(void)
-{
-	thaw_processes();
-}
-#else
-static inline int suspend_freeze_processes(void)
-{
-	return 0;
-}
-
-static inline void suspend_thaw_processes(void)
-{
-}
-#endif
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 4fd51be..5f51fc7 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -29,6 +29,62 @@

 #include "power.h"

+#ifdef CONFIG_SUSPEND_FREEZER
+
+static inline int suspend_freeze_processes(void)
+{
+	int error;
+
+	error = freeze_processes();
+
+	/*
+	 * freeze_processes() automatically thaws every task if freezing
+	 * fails. So we need not do anything extra upon error.
+	 */
+
+	if (error)
+		goto Finish;
+
+	error = freeze_supers();
+	if (error) {
+		thaw_processes();
+		goto Finish;
+	}
+
+	error = freeze_kernel_threads();
+
+	/*
+	 * freeze_kernel_threads() thaws only kernel threads upon freezing
+	 * failure. So we have to thaw the userspace tasks ourselves.
+	 */
+	if (error) {
+		thaw_supers();
+		thaw_processes();
+	}
+
+Finish:
+	return error;
+}
+
+static inline void suspend_thaw_processes(void)
+{
+	thaw_supers();
+	thaw_processes();
+}
+
+#else /* !CONFIG_SUSPEND_FREEZER */
+
+static inline int suspend_freeze_processes(void)
+{
+	return 0;
+}
+
+static inline void suspend_thaw_processes(void)
+{
+}
+
+#endif /* !CONFIG_SUSPEND_FREEZER */
+
 const char *const pm_states[PM_SUSPEND_MAX] = {
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Friday, February 17, 2012, Josh Boyer wrote:
> On Fri, Feb 17, 2012 at 10:41 AM, Josh Boyer <jwboyer@gmail.com> wrote:
> > On Sat, Jan 28, 2012 at 8:45 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>
> >> Freeze all filesystems during system suspend and (kernel-driven)
> >> hibernation by calling freeze_supers() for all superblocks and thaw
> >> them during the subsequent resume with the help of thaw_supers().
> >>
> >> This makes filesystems stay in a consistent state in case something
> >> goes wrong between system suspend (or hibernation) and the subsequent
> >> resume (e.g. journal replays won't be necessary in those cases).  In
> >> particular, this should help to solve a long-standing issue that, in
> >> some cases, during resume from hibernation the boot loader causes the
> >> journal to be replied for the filesystem containing the kernel image
> >> and/or initrd causing it to become inconsistent with the information
> >> stored in the hibernation image.
> >>
> >> The user-space-driven hibernation (s2disk) is not covered by this
> >> change, because the freezing of filesystems prevents s2disk from
> >> accessing device special files it needs to do its job.
> >>
> >> This change is based on earlier work by Nigel Cunningham.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > Do you happen to have a version of this patch that has been rebased to the
> > latest 3.3 git tree?  I looked in the various archives and haven't found one
> > yet.  I'd like to get this into Fedora rawhide to see if some of the hibernate
> > issues we've been seeing clear up.
> 
> I got impatient and decided to tackle this myself.  If others would like to
> look at the forward port below, I'd appreciate it.

It looks good at first sight, thanks for rebasing!

Rafael


> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date:   Fri Feb 17 12:42:08 2012 -0500
> 
>     Freeze all filesystems during system suspend and (kernel-driven)
>     hibernation by calling freeze_supers() for all superblocks and thaw
>     them during the subsequent resume with the help of thaw_supers().
> 
>     This makes filesystems stay in a consistent state in case something
>     goes wrong between system suspend (or hibernation) and the subsequent
>     resume (e.g. journal replays won't be necessary in those cases).  In
>     particular, this should help to solve a long-standing issue that, in
>     some cases, during resume from hibernation the boot loader causes the
>     journal to be replied for the filesystem containing the kernel image
>     and/or initrd causing it to become inconsistent with the information
>     stored in the hibernation image.
> 
>     The user-space-driven hibernation (s2disk) is not covered by this
>     change, because the freezing of filesystems prevents s2disk from
>     accessing device special files it needs to do its job.
> 
>     This change is based on earlier work by Nigel Cunningham.
> 
>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
> 
> diff --git a/fs/super.c b/fs/super.c
> index 6015c02..c8057fa 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -594,6 +594,79 @@ void iterate_supers_type(struct file_system_type *type,
>  EXPORT_SYMBOL(iterate_supers_type);
> 
>  /**
> + *	thaw_supers - call thaw_super() for all superblocks
> + */
> +void thaw_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (hlist_unhashed(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_flags & MS_FROZEN) {
> +			thaw_super(sb);
> +			sb->s_flags &= ~MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +}
> +
> +/**
> + *	freeze_supers - call freeze_super() for all superblocks
> + */
> +int freeze_supers(void)
> +{
> +	struct super_block *sb, *p = NULL;
> +	int error = 0;
> +
> +	spin_lock(&sb_lock);
> +	/*
> +	 * Freeze in reverse order so filesystems depending on others are
> +	 * frozen in the right order (eg. loopback on ext3).
> +	 */
> +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> +		if (hlist_unhashed(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
> +		    && !(sb->s_flags & MS_RDONLY)) {
> +			error = freeze_super(sb);
> +			if (!error)
> +				sb->s_flags |= MS_FROZEN;
> +		}
> +
> +		spin_lock(&sb_lock);
> +		if (error)
> +			break;
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +
> +	if (error)
> +		thaw_supers();
> +
> +	return error;
> +}
> +
> +
> +/**
>   *	get_super - get the superblock of a device
>   *	@bdev: device to get the superblock for
>   *	
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 386da09..a164f4a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -210,6 +210,7 @@ struct inodes_stat_t {
>  #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
>  #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
>  #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
> +#define MS_FROZEN	(1<<25) /* Frozen filesystem */
>  #define MS_NOSEC	(1<<28)
>  #define MS_BORN		(1<<29)
>  #define MS_ACTIVE	(1<<30)
> @@ -2501,6 +2502,8 @@ extern void drop_super(struct super_block *sb);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);
>  extern void iterate_supers_type(struct file_system_type *,
>  			        void (*)(struct super_block *, void *), void *);
> +extern int freeze_supers(void);
> +extern void thaw_supers(void);
> 
>  extern int dcache_dir_open(struct inode *, struct file *);
>  extern int dcache_dir_close(struct inode *, struct file *);
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 6d6d288..492fc62 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -626,12 +626,17 @@ int hibernate(void)
>  	if (error)
>  		goto Finish;
> 
> -	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	error = freeze_supers();
>  	if (error)
>  		goto Thaw;
> +
> +	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> +	if (error)
> +		goto Thaw_fs;
> +
>  	if (freezer_test_done) {
>  		freezer_test_done = false;
> -		goto Thaw;
> +		goto Thaw_fs;
>  	}
> 
>  	if (in_suspend) {
> @@ -655,6 +660,8 @@ int hibernate(void)
>  		pr_debug("PM: Image restored successfully.\n");
>  	}
> 
> + Thaw_fs:
> +	thaw_supers();
>   Thaw:
>  	thaw_processes();
>   Finish:
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 21724ee..40d6f64 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -1,3 +1,4 @@
> +#include <linux/fs.h>
>  #include <linux/suspend.h>
>  #include <linux/suspend_ioctls.h>
>  #include <linux/utsname.h>
> @@ -227,45 +228,3 @@ enum {
>  #define TEST_MAX	(__TEST_AFTER_LAST - 1)
> 
>  extern int pm_test_level;
> -
> -#ifdef CONFIG_SUSPEND_FREEZER
> -static inline int suspend_freeze_processes(void)
> -{
> -	int error;
> -
> -	error = freeze_processes();
> -
> -	/*
> -	 * freeze_processes() automatically thaws every task if freezing
> -	 * fails. So we need not do anything extra upon error.
> -	 */
> -	if (error)
> -		goto Finish;
> -
> -	error = freeze_kernel_threads();
> -
> -	/*
> -	 * freeze_kernel_threads() thaws only kernel threads upon freezing
> -	 * failure. So we have to thaw the userspace tasks ourselves.
> -	 */
> -	if (error)
> -		thaw_processes();
> -
> - Finish:
> -	return error;
> -}
> -
> -static inline void suspend_thaw_processes(void)
> -{
> -	thaw_processes();
> -}
> -#else
> -static inline int suspend_freeze_processes(void)
> -{
> -	return 0;
> -}
> -
> -static inline void suspend_thaw_processes(void)
> -{
> -}
> -#endif
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4fd51be..5f51fc7 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -29,6 +29,62 @@
> 
>  #include "power.h"
> 
> +#ifdef CONFIG_SUSPEND_FREEZER
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	int error;
> +
> +	error = freeze_processes();
> +
> +	/*
> +	 * freeze_processes() automatically thaws every task if freezing
> +	 * fails. So we need not do anything extra upon error.
> +	 */
> +
> +	if (error)
> +		goto Finish;
> +
> +	error = freeze_supers();
> +	if (error) {
> +		thaw_processes();
> +		goto Finish;
> +	}
> +
> +	error = freeze_kernel_threads();
> +
> +	/*
> +	 * freeze_kernel_threads() thaws only kernel threads upon freezing
> +	 * failure. So we have to thaw the userspace tasks ourselves.
> +	 */
> +	if (error) {
> +		thaw_supers();
> +		thaw_processes();
> +	}
> +
> +Finish:
> +	return error;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +	thaw_supers();
> +	thaw_processes();
> +}
> +
> +#else /* !CONFIG_SUSPEND_FREEZER */
> +
> +static inline int suspend_freeze_processes(void)
> +{
> +	return 0;
> +}
> +
> +static inline void suspend_thaw_processes(void)
> +{
> +}
> +
> +#endif /* !CONFIG_SUSPEND_FREEZER */
> +
>  const char *const pm_states[PM_SUSPEND_MAX] = {
>  	[PM_SUSPEND_STANDBY]	= "standby",
>  	[PM_SUSPEND_MEM]	= "mem",
> 
> 

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> Date:   Fri Feb 17 12:42:08 2012 -0500
>>
>>     Freeze all filesystems during system suspend and (kernel-driven)
>>     hibernation by calling freeze_supers() for all superblocks and thaw
>>     them during the subsequent resume with the help of thaw_supers().
>>
>>     This makes filesystems stay in a consistent state in case something
>>     goes wrong between system suspend (or hibernation) and the subsequent
>>     resume (e.g. journal replays won't be necessary in those cases).  In
>>     particular, this should help to solve a long-standing issue that, in
>>     some cases, during resume from hibernation the boot loader causes the
>>     journal to be replied for the filesystem containing the kernel image
>>     and/or initrd causing it to become inconsistent with the information
>>     stored in the hibernation image.
>>
>>     The user-space-driven hibernation (s2disk) is not covered by this
>>     change, because the freezing of filesystems prevents s2disk from
>>     accessing device special files it needs to do its job.
>>
>>     This change is based on earlier work by Nigel Cunningham.
>>
>>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>
>>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>

Did this patch ever wind up going anywhere?  Fedora has it sitting in
our tree with a comment that says "rebase" and I don't see it in the
linux-next tree at all.

Did if fall through the cracks or was it NAKed somewhere?

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Friday, May 25, 2012, Josh Boyer wrote:
> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> Date:   Fri Feb 17 12:42:08 2012 -0500
> >>
> >>     Freeze all filesystems during system suspend and (kernel-driven)
> >>     hibernation by calling freeze_supers() for all superblocks and thaw
> >>     them during the subsequent resume with the help of thaw_supers().
> >>
> >>     This makes filesystems stay in a consistent state in case something
> >>     goes wrong between system suspend (or hibernation) and the subsequent
> >>     resume (e.g. journal replays won't be necessary in those cases).  In
> >>     particular, this should help to solve a long-standing issue that, in
> >>     some cases, during resume from hibernation the boot loader causes the
> >>     journal to be replied for the filesystem containing the kernel image
> >>     and/or initrd causing it to become inconsistent with the information
> >>     stored in the hibernation image.
> >>
> >>     The user-space-driven hibernation (s2disk) is not covered by this
> >>     change, because the freezing of filesystems prevents s2disk from
> >>     accessing device special files it needs to do its job.
> >>
> >>     This change is based on earlier work by Nigel Cunningham.
> >>
> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>
> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
> 
> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> our tree with a comment that says "rebase" and I don't see it in the
> linux-next tree at all.
> 
> Did if fall through the cracks or was it NAKed somewhere?

No, it wasn't in principle. There were some comments I haven't addressed yet.

Thanks,
Rafael

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, May 25, 2012, Josh Boyer wrote:
>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
>> >>
>> >>     Freeze all filesystems during system suspend and (kernel-driven)
>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
>> >>     them during the subsequent resume with the help of thaw_supers().
>> >>
>> >>     This makes filesystems stay in a consistent state in case something
>> >>     goes wrong between system suspend (or hibernation) and the subsequent
>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
>> >>     particular, this should help to solve a long-standing issue that, in
>> >>     some cases, during resume from hibernation the boot loader causes the
>> >>     journal to be replied for the filesystem containing the kernel image
>> >>     and/or initrd causing it to become inconsistent with the information
>> >>     stored in the hibernation image.
>> >>
>> >>     The user-space-driven hibernation (s2disk) is not covered by this
>> >>     change, because the freezing of filesystems prevents s2disk from
>> >>     accessing device special files it needs to do its job.
>> >>
>> >>     This change is based on earlier work by Nigel Cunningham.
>> >>
>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> >>
>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
>>
>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
>> our tree with a comment that says "rebase" and I don't see it in the
>> linux-next tree at all.
>>
>> Did if fall through the cracks or was it NAKed somewhere?
>
> No, it wasn't in principle. There were some comments I haven't addressed yet.

Dredging up a really old thread, sorry.

We're still carrying this patch along in Fedora.  Should we drop it at
this point, or is it still eventually going to head upstream?

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Friday, May 25, 2012, Josh Boyer wrote:
>>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
>>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
>>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
>>> >>
>>> >>     Freeze all filesystems during system suspend and (kernel-driven)
>>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
>>> >>     them during the subsequent resume with the help of thaw_supers().
>>> >>
>>> >>     This makes filesystems stay in a consistent state in case something
>>> >>     goes wrong between system suspend (or hibernation) and the subsequent
>>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
>>> >>     particular, this should help to solve a long-standing issue that, in
>>> >>     some cases, during resume from hibernation the boot loader causes the
>>> >>     journal to be replied for the filesystem containing the kernel image
>>> >>     and/or initrd causing it to become inconsistent with the information
>>> >>     stored in the hibernation image.
>>> >>
>>> >>     The user-space-driven hibernation (s2disk) is not covered by this
>>> >>     change, because the freezing of filesystems prevents s2disk from
>>> >>     accessing device special files it needs to do its job.
>>> >>
>>> >>     This change is based on earlier work by Nigel Cunningham.
>>> >>
>>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>> >>
>>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
>>>
>>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
>>> our tree with a comment that says "rebase" and I don't see it in the
>>> linux-next tree at all.
>>>
>>> Did if fall through the cracks or was it NAKed somewhere?
>>
>> No, it wasn't in principle. There were some comments I haven't addressed yet.
>
> Dredging up a really old thread, sorry.
>
> We're still carrying this patch along in Fedora.  Should we drop it at
> this point, or is it still eventually going to head upstream?

Fixed Rafael's email address.  (Double sorry.)

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Pavel Machek]

Hi!

> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> >>> our tree with a comment that says "rebase" and I don't see it in the
> >>> linux-next tree at all.
> >>>
> >>> Did if fall through the cracks or was it NAKed somewhere?
> >>
> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> >
> > Dredging up a really old thread, sorry.
> >
> > We're still carrying this patch along in Fedora.  Should we drop it at
> > this point, or is it still eventually going to head upstream?

I'd say drop.

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

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Dave Chinner]

On Wed, Dec 18, 2013 at 12:08:43AM +0100, Pavel Machek wrote:
> Hi!
> 
> > >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> > >>> our tree with a comment that says "rebase" and I don't see it in the
> > >>> linux-next tree at all.
> > >>>
> > >>> Did if fall through the cracks or was it NAKed somewhere?
> > >>
> > >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> > >
> > > Dredging up a really old thread, sorry.
> > >
> > > We're still carrying this patch along in Fedora.  Should we drop it at
> > > this point, or is it still eventually going to head upstream?
> 
> I'd say drop.

I disagree - given the problem it is resolving leads to silent
filesystem corruption, this patch should be considered somewhat of a
priority to push...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Pavel Machek]

On Wed 2013-12-18 10:31:52, Dave Chinner wrote:
> On Wed, Dec 18, 2013 at 12:08:43AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> > > >>> our tree with a comment that says "rebase" and I don't see it in the
> > > >>> linux-next tree at all.
> > > >>>
> > > >>> Did if fall through the cracks or was it NAKed somewhere?
> > > >>
> > > >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> > > >
> > > > Dredging up a really old thread, sorry.
> > > >
> > > > We're still carrying this patch along in Fedora.  Should we drop it at
> > > > this point, or is it still eventually going to head upstream?
> > 
> > I'd say drop.
> 
> I disagree - given the problem it is resolving leads to silent
> filesystem corruption, this patch should be considered somewhat of a
> priority to push...

Umm. Ok, I forgot what it does, really.

So... for few years now suspend corrupts data on XFS? And Fedora has
the fix but it is not in mainline? That does not sound right...

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

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Tuesday, December 17, 2013 11:04:46 AM Josh Boyer wrote:
> On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> > On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Friday, May 25, 2012, Josh Boyer wrote:
> >>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
> >>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
> >>> >>
> >>> >>     Freeze all filesystems during system suspend and (kernel-driven)
> >>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
> >>> >>     them during the subsequent resume with the help of thaw_supers().
> >>> >>
> >>> >>     This makes filesystems stay in a consistent state in case something
> >>> >>     goes wrong between system suspend (or hibernation) and the subsequent
> >>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
> >>> >>     particular, this should help to solve a long-standing issue that, in
> >>> >>     some cases, during resume from hibernation the boot loader causes the
> >>> >>     journal to be replied for the filesystem containing the kernel image
> >>> >>     and/or initrd causing it to become inconsistent with the information
> >>> >>     stored in the hibernation image.
> >>> >>
> >>> >>     The user-space-driven hibernation (s2disk) is not covered by this
> >>> >>     change, because the freezing of filesystems prevents s2disk from
> >>> >>     accessing device special files it needs to do its job.
> >>> >>
> >>> >>     This change is based on earlier work by Nigel Cunningham.
> >>> >>
> >>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> >>
> >>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
> >>>
> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> >>> our tree with a comment that says "rebase" and I don't see it in the
> >>> linux-next tree at all.
> >>>
> >>> Did if fall through the cracks or was it NAKed somewhere?
> >>
> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> >
> > Dredging up a really old thread, sorry.
> >
> > We're still carrying this patch along in Fedora.  Should we drop it at
> > this point, or is it still eventually going to head upstream?
> 
> Fixed Rafael's email address.  (Double sorry.)

No biggie.

I just hadn't got sufficient response for that patch at the time it was
submitted, so I guess it would be good to resubmit it.  Please feel free to
do that if you want.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Tue, Dec 17, 2013 at 7:52 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, December 17, 2013 11:04:46 AM Josh Boyer wrote:
>> On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>> > On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> On Friday, May 25, 2012, Josh Boyer wrote:
>> >>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
>> >>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
>> >>> >>
>> >>> >>     Freeze all filesystems during system suspend and (kernel-driven)
>> >>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
>> >>> >>     them during the subsequent resume with the help of thaw_supers().
>> >>> >>
>> >>> >>     This makes filesystems stay in a consistent state in case something
>> >>> >>     goes wrong between system suspend (or hibernation) and the subsequent
>> >>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
>> >>> >>     particular, this should help to solve a long-standing issue that, in
>> >>> >>     some cases, during resume from hibernation the boot loader causes the
>> >>> >>     journal to be replied for the filesystem containing the kernel image
>> >>> >>     and/or initrd causing it to become inconsistent with the information
>> >>> >>     stored in the hibernation image.
>> >>> >>
>> >>> >>     The user-space-driven hibernation (s2disk) is not covered by this
>> >>> >>     change, because the freezing of filesystems prevents s2disk from
>> >>> >>     accessing device special files it needs to do its job.
>> >>> >>
>> >>> >>     This change is based on earlier work by Nigel Cunningham.
>> >>> >>
>> >>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> >>> >>
>> >>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
>> >>>
>> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
>> >>> our tree with a comment that says "rebase" and I don't see it in the
>> >>> linux-next tree at all.
>> >>>
>> >>> Did if fall through the cracks or was it NAKed somewhere?
>> >>
>> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
>> >
>> > Dredging up a really old thread, sorry.
>> >
>> > We're still carrying this patch along in Fedora.  Should we drop it at
>> > this point, or is it still eventually going to head upstream?
>>
>> Fixed Rafael's email address.  (Double sorry.)
>
> No biggie.
>
> I just hadn't got sufficient response for that patch at the time it was
> submitted, so I guess it would be good to resubmit it.  Please feel free to
> do that if you want.

You want me to resend a patch you authored back to you?  I mean, I can
do that but it seems a bit strange.  All I did was rebase what you
wrote to a newer kernel version.

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Tuesday, December 17, 2013 08:00:55 PM Josh Boyer wrote:
> On Tue, Dec 17, 2013 at 7:52 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, December 17, 2013 11:04:46 AM Josh Boyer wrote:
> >> On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> >> > On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> On Friday, May 25, 2012, Josh Boyer wrote:
> >> >>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
> >> >>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> >>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
> >> >>> >>
> >> >>> >>     Freeze all filesystems during system suspend and (kernel-driven)
> >> >>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
> >> >>> >>     them during the subsequent resume with the help of thaw_supers().
> >> >>> >>
> >> >>> >>     This makes filesystems stay in a consistent state in case something
> >> >>> >>     goes wrong between system suspend (or hibernation) and the subsequent
> >> >>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
> >> >>> >>     particular, this should help to solve a long-standing issue that, in
> >> >>> >>     some cases, during resume from hibernation the boot loader causes the
> >> >>> >>     journal to be replied for the filesystem containing the kernel image
> >> >>> >>     and/or initrd causing it to become inconsistent with the information
> >> >>> >>     stored in the hibernation image.
> >> >>> >>
> >> >>> >>     The user-space-driven hibernation (s2disk) is not covered by this
> >> >>> >>     change, because the freezing of filesystems prevents s2disk from
> >> >>> >>     accessing device special files it needs to do its job.
> >> >>> >>
> >> >>> >>     This change is based on earlier work by Nigel Cunningham.
> >> >>> >>
> >> >>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> >>> >>
> >> >>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
> >> >>>
> >> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> >> >>> our tree with a comment that says "rebase" and I don't see it in the
> >> >>> linux-next tree at all.
> >> >>>
> >> >>> Did if fall through the cracks or was it NAKed somewhere?
> >> >>
> >> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> >> >
> >> > Dredging up a really old thread, sorry.
> >> >
> >> > We're still carrying this patch along in Fedora.  Should we drop it at
> >> > this point, or is it still eventually going to head upstream?
> >>
> >> Fixed Rafael's email address.  (Double sorry.)
> >
> > No biggie.
> >
> > I just hadn't got sufficient response for that patch at the time it was
> > submitted, so I guess it would be good to resubmit it.  Please feel free to
> > do that if you want.
> 
> You want me to resend a patch you authored back to you?  I mean, I can
> do that but it seems a bit strange.  All I did was rebase what you
> wrote to a newer kernel version.

Well, you can send it to me in private then and I'll resubmit. :-)

I just don't have any recent version of it handy.

Thanks,
Rafael

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Josh Boyer]

On Tue, Dec 17, 2013 at 8:16 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, December 17, 2013 08:00:55 PM Josh Boyer wrote:
>> On Tue, Dec 17, 2013 at 7:52 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Tuesday, December 17, 2013 11:04:46 AM Josh Boyer wrote:
>> >> On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>> >> > On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> >> On Friday, May 25, 2012, Josh Boyer wrote:
>> >> >>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> >>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
>> >> >>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
>> >> >>> >>
>> >> >>> >>     Freeze all filesystems during system suspend and (kernel-driven)
>> >> >>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
>> >> >>> >>     them during the subsequent resume with the help of thaw_supers().
>> >> >>> >>
>> >> >>> >>     This makes filesystems stay in a consistent state in case something
>> >> >>> >>     goes wrong between system suspend (or hibernation) and the subsequent
>> >> >>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
>> >> >>> >>     particular, this should help to solve a long-standing issue that, in
>> >> >>> >>     some cases, during resume from hibernation the boot loader causes the
>> >> >>> >>     journal to be replied for the filesystem containing the kernel image
>> >> >>> >>     and/or initrd causing it to become inconsistent with the information
>> >> >>> >>     stored in the hibernation image.
>> >> >>> >>
>> >> >>> >>     The user-space-driven hibernation (s2disk) is not covered by this
>> >> >>> >>     change, because the freezing of filesystems prevents s2disk from
>> >> >>> >>     accessing device special files it needs to do its job.
>> >> >>> >>
>> >> >>> >>     This change is based on earlier work by Nigel Cunningham.
>> >> >>> >>
>> >> >>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >>> >>
>> >> >>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
>> >> >>>
>> >> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
>> >> >>> our tree with a comment that says "rebase" and I don't see it in the
>> >> >>> linux-next tree at all.
>> >> >>>
>> >> >>> Did if fall through the cracks or was it NAKed somewhere?
>> >> >>
>> >> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
>> >> >
>> >> > Dredging up a really old thread, sorry.
>> >> >
>> >> > We're still carrying this patch along in Fedora.  Should we drop it at
>> >> > this point, or is it still eventually going to head upstream?
>> >>
>> >> Fixed Rafael's email address.  (Double sorry.)
>> >
>> > No biggie.
>> >
>> > I just hadn't got sufficient response for that patch at the time it was
>> > submitted, so I guess it would be good to resubmit it.  Please feel free to
>> > do that if you want.
>>
>> You want me to resend a patch you authored back to you?  I mean, I can
>> do that but it seems a bit strange.  All I did was rebase what you
>> wrote to a newer kernel version.
>
> Well, you can send it to me in private then and I'll resubmit. :-)

Done.  Thanks.

josh

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Dave Chinner]

On Wed, Dec 18, 2013 at 01:01:28AM +0100, Pavel Machek wrote:
> On Wed 2013-12-18 10:31:52, Dave Chinner wrote:
> > On Wed, Dec 18, 2013 at 12:08:43AM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> > > > >>> our tree with a comment that says "rebase" and I don't see it in the
> > > > >>> linux-next tree at all.
> > > > >>>
> > > > >>> Did if fall through the cracks or was it NAKed somewhere?
> > > > >>
> > > > >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> > > > >
> > > > > Dredging up a really old thread, sorry.
> > > > >
> > > > > We're still carrying this patch along in Fedora.  Should we drop it at
> > > > > this point, or is it still eventually going to head upstream?
> > > 
> > > I'd say drop.
> > 
> > I disagree - given the problem it is resolving leads to silent
> > filesystem corruption, this patch should be considered somewhat of a
> > priority to push...
> 
> Umm. Ok, I forgot what it does, really.

It ensures that the filesystem is in an quiescent state both in
memory and on disk, and it cannot be modified in memory or on disk
whilst the suspend image is being generated, or by log recovery
after a resume before the suspended image has been restored.

> So... for few years now suspend corrupts data on XFS? And Fedora has
> the fix but it is not in mainline? That does not sound right...

The issues freezing the filesystem before the suspend image is
created affect every journalled filesystem linux supports, be
it XFS, ext4, reiser, btrfs, etc.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Pavel Machek]

> > > I disagree - given the problem it is resolving leads to silent
> > > filesystem corruption, this patch should be considered somewhat of a
> > > priority to push...
> > 
> > Umm. Ok, I forgot what it does, really.
> 
> It ensures that the filesystem is in an quiescent state both in
> memory and on disk, and it cannot be modified in memory or on disk
> whilst the suspend image is being generated, or by log recovery
> after a resume before the suspended image has been restored.

If someone attempts to run log recovery before resume, that's a bug
and yes, it will corrupt filesystems. (Including ext3). Don't do that.

Documentation/power/swsusp.txt:

 * BIG FAT WARNING
   *********************************************************
 *
 * If you touch anything on disk between suspend and resume...
 *                              ...kiss your data goodbye.

> > So... for few years now suspend corrupts data on XFS? And Fedora has
> > the fix but it is not in mainline? That does not sound right...
> 
> The issues freezing the filesystem before the suspend image is
> created affect every journalled filesystem linux supports, be
> it XFS, ext4, reiser, btrfs, etc.

Did not it have some problems with ext3?

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

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Rafael J. Wysocki]

On Wednesday, December 18, 2013 07:31:59 AM Josh Boyer wrote:
> On Tue, Dec 17, 2013 at 8:16 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, December 17, 2013 08:00:55 PM Josh Boyer wrote:
> >> On Tue, Dec 17, 2013 at 7:52 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Tuesday, December 17, 2013 11:04:46 AM Josh Boyer wrote:
> >> >> On Tue, Dec 17, 2013 at 11:03 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> >> >> > On Fri, May 25, 2012 at 3:13 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> >> On Friday, May 25, 2012, Josh Boyer wrote:
> >> >> >>> On Fri, Feb 17, 2012 at 3:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> >>> >> commit b94887bbc0621e1e8402e7f0ec4bc3adf46c9a6e
> >> >> >>> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> >>> >> Date:   Fri Feb 17 12:42:08 2012 -0500
> >> >> >>> >>
> >> >> >>> >>     Freeze all filesystems during system suspend and (kernel-driven)
> >> >> >>> >>     hibernation by calling freeze_supers() for all superblocks and thaw
> >> >> >>> >>     them during the subsequent resume with the help of thaw_supers().
> >> >> >>> >>
> >> >> >>> >>     This makes filesystems stay in a consistent state in case something
> >> >> >>> >>     goes wrong between system suspend (or hibernation) and the subsequent
> >> >> >>> >>     resume (e.g. journal replays won't be necessary in those cases).  In
> >> >> >>> >>     particular, this should help to solve a long-standing issue that, in
> >> >> >>> >>     some cases, during resume from hibernation the boot loader causes the
> >> >> >>> >>     journal to be replied for the filesystem containing the kernel image
> >> >> >>> >>     and/or initrd causing it to become inconsistent with the information
> >> >> >>> >>     stored in the hibernation image.
> >> >> >>> >>
> >> >> >>> >>     The user-space-driven hibernation (s2disk) is not covered by this
> >> >> >>> >>     change, because the freezing of filesystems prevents s2disk from
> >> >> >>> >>     accessing device special files it needs to do its job.
> >> >> >>> >>
> >> >> >>> >>     This change is based on earlier work by Nigel Cunningham.
> >> >> >>> >>
> >> >> >>> >>     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> >>> >>
> >> >> >>> >>     Rebased to 3.3-rc3 by Josh Boyer <jwboyer@redhat.com>
> >> >> >>>
> >> >> >>> Did this patch ever wind up going anywhere?  Fedora has it sitting in
> >> >> >>> our tree with a comment that says "rebase" and I don't see it in the
> >> >> >>> linux-next tree at all.
> >> >> >>>
> >> >> >>> Did if fall through the cracks or was it NAKed somewhere?
> >> >> >>
> >> >> >> No, it wasn't in principle. There were some comments I haven't addressed yet.
> >> >> >
> >> >> > Dredging up a really old thread, sorry.
> >> >> >
> >> >> > We're still carrying this patch along in Fedora.  Should we drop it at
> >> >> > this point, or is it still eventually going to head upstream?
> >> >>
> >> >> Fixed Rafael's email address.  (Double sorry.)
> >> >
> >> > No biggie.
> >> >
> >> > I just hadn't got sufficient response for that patch at the time it was
> >> > submitted, so I guess it would be good to resubmit it.  Please feel free to
> >> > do that if you want.
> >>
> >> You want me to resend a patch you authored back to you?  I mean, I can
> >> do that but it seems a bit strange.  All I did was rebase what you
> >> wrote to a newer kernel version.
> >
> > Well, you can send it to me in private then and I'll resubmit. :-)
> 
> Done.  Thanks.

OK, thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Dave Chinner]

On Wed, Dec 18, 2013 at 03:08:42PM +0100, Pavel Machek wrote:
> > > > I disagree - given the problem it is resolving leads to silent
> > > > filesystem corruption, this patch should be considered somewhat of a
> > > > priority to push...
> > > 
> > > Umm. Ok, I forgot what it does, really.
> > 
> > It ensures that the filesystem is in an quiescent state both in
> > memory and on disk, and it cannot be modified in memory or on disk
> > whilst the suspend image is being generated, or by log recovery
> > after a resume before the suspended image has been restored.
> 
> If someone attempts to run log recovery before resume, that's a bug
> and yes, it will corrupt filesystems. (Including ext3). Don't do that.

Freezing the filesystem prevents that accidental mount of the
filesystem from being an issue. It fixes a bug that:

> Documentation/power/swsusp.txt:
> 
>  * BIG FAT WARNING
>    *********************************************************
>  *
>  * If you touch anything on disk between suspend and resume...
>  *                              ...kiss your data goodbye.

Makes this a whole lot less dangerous.

> > > So... for few years now suspend corrupts data on XFS? And Fedora has
> > > the fix but it is not in mainline? That does not sound right...
> > 
> > The issues freezing the filesystem before the suspend image is
> > created affect every journalled filesystem linux supports, be
> > it XFS, ext4, reiser, btrfs, etc.
> 
> Did not it have some problems with ext3?

Please read more carefully: "affect every journalled filesystem
linux supports". So, ext3 is affected because it's a journalling
filesystem.  I didn't list every journalled filesystem Linux
supports - "etc" means there are more examples that aren't
explicitly listed.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Pavel Machek]

Hi!

> > > > > I disagree - given the problem it is resolving leads to silent
> > > > > filesystem corruption, this patch should be considered somewhat of a
> > > > > priority to push...
> > > > 
> > > > Umm. Ok, I forgot what it does, really.
> > > 
> > > It ensures that the filesystem is in an quiescent state both in
> > > memory and on disk, and it cannot be modified in memory or on disk
> > > whilst the suspend image is being generated, or by log recovery
> > > after a resume before the suspended image has been restored.
> > 
> > If someone attempts to run log recovery before resume, that's a bug
> > and yes, it will corrupt filesystems. (Including ext3). Don't do that.
> 
> Freezing the filesystem prevents that accidental mount of the
> filesystem from being an issue. It fixes a bug that:

Can you elaborate on that?

If you do read-write mount of that filesystem, surely filesystem
metadata will differ from what the filesystem expects. You'll still
get data corruption AFAICT.

Read-only mount... maybe that will get slightly better -- there'll be
no journal to play back. But what happens to superblock information
such as "last mount time"? Mount counts?

> > Documentation/power/swsusp.txt:
> > 
> >  * BIG FAT WARNING
> >    *********************************************************
> >  *
> >  * If you touch anything on disk between suspend and resume...
> >  *                              ...kiss your data goodbye.
> 
> Makes this a whole lot less dangerous.

Do you claim that it is now safe to mount (rw) and access filesystem
between suspend and resume?

> > > > So... for few years now suspend corrupts data on XFS? And Fedora has
> > > > the fix but it is not in mainline? That does not sound right...
> > > 
> > > The issues freezing the filesystem before the suspend image is
> > > created affect every journalled filesystem linux supports, be
> > > it XFS, ext4, reiser, btrfs, etc.
> > 
> > Did not it have some problems with ext3?
> 
> Please read more carefully: "affect every journalled filesystem
> linux supports". So, ext3 is affected because it's a journalling
> filesystem.  I didn't list every journalled filesystem Linux
> supports - "etc" means there are more examples that aren't
> explicitly listed.

Sorry for being unclear. My memory says this patch caused some
regression with ext3. Is my memory wrong? Or was the regression fixed
in the meantime?

(Clearly, ext3 has the same issues with journal replays as btrfs. But
note that mounting filesystem between suspend and resume is not safe
even on simple filesystems such as ext2).

Thanks,
								Pavel

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

Re: [RFC][PATCH] PM / Sleep: Freeze filesystems during system suspend/hibernation

[Dave Chinner]

On Sun, Dec 22, 2013 at 12:33:18AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > > > I disagree - given the problem it is resolving leads to silent
> > > > > > filesystem corruption, this patch should be considered somewhat of a
> > > > > > priority to push...
> > > > > 
> > > > > Umm. Ok, I forgot what it does, really.
> > > > 
> > > > It ensures that the filesystem is in an quiescent state both in
> > > > memory and on disk, and it cannot be modified in memory or on disk
> > > > whilst the suspend image is being generated, or by log recovery
> > > > after a resume before the suspended image has been restored.
> > > 
> > > If someone attempts to run log recovery before resume, that's a bug
> > > and yes, it will corrupt filesystems. (Including ext3). Don't do that.
> > 
> > Freezing the filesystem prevents that accidental mount of the
> > filesystem from being an issue. It fixes a bug that:
> 
> Can you elaborate on that?
> 
> If you do read-write mount of that filesystem, surely filesystem
> metadata will differ from what the filesystem expects. You'll still
> get data corruption AFAICT.

Only if you modify stuff. That's not what we are protecting against,
it's avoiding the automatic journal replay that you can't avoid if
you accidentally mount the filesystem.

> Read-only mount... maybe that will get slightly better -- there'll be
> no journal to play back. But what happens to superblock information
> such as "last mount time"? Mount counts?

If metadata is being modified on a read only mount outside of
journal replay, then the filesystem needs fixing.

> 
> > > Documentation/power/swsusp.txt:
> > > 
> > >  * BIG FAT WARNING
> > >    *********************************************************
> > >  *
> > >  * If you touch anything on disk between suspend and resume...
> > >  *                              ...kiss your data goodbye.
> > 
> > Makes this a whole lot less dangerous.
> 
> Do you claim that it is now safe to mount (rw) and access filesystem
> between suspend and resume?

No, I didn't claim that. "less dangerous" is still dangerous, just
less so than it was before.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com