[patch 1/1] PM: Adds remount fs ro at suspend

[patch 1/1] PM: Adds remount fs ro at suspend

[akuster]

This adds the ability for the file system to remounted as read only during a
system suspend.  Log the mount points so when the resume occurs, they can be 
remounted back to their original states. This is so in an advent of a power
failure, we try our best to keep data from being corrupted or lost.

Signed-of-by: Armin Kuster <AKuster@mvista.com>


---

 fs/super.c           |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/power/Kconfig |   11 ++++++++
 kernel/power/main.c  |   13 +++++++++
 kernel/power/power.h |    6 ++++
 4 files changed, 97 insertions(+)

diff -puN fs/super.c~suspend_fs_ro fs/super.c
--- linux-2.6_sdio/fs/super.c~suspend_fs_ro	2007-02-01 13:35:46.000000000 -1000
+++ linux-2.6_sdio-akuster/fs/super.c	2007-02-01 13:35:50.000000000 -1000
@@ -47,6 +47,70 @@ struct file_system_type *get_fs_type(con
 LIST_HEAD(super_blocks);
 DEFINE_SPINLOCK(sb_lock);
 
+#ifdef CONFIG_SUSPEND_REMOUNTFS
+/*
+ * Code to preserve filesystem data during suspend.
+ */
+
+struct suspremount {
+struct super_block *sb;
+struct suspremount *next;
+};
+
+static struct suspremount *suspremount_list;
+
+void suspend_remount_log_fs(struct super_block *sb)
+{
+	struct suspremount *remountp;
+
+	if ((remountp = (struct suspremount *)
+		kmalloc(sizeof(struct suspremount), GFP_KERNEL)) != NULL) {
+		remountp->sb = sb;
+		remountp->next = suspremount_list;
+		suspremount_list = remountp;
+	}
+}
+
+/*
+ * Remount filesystems prior to suspend, in case the
+ * power source is removed (ie, battery removed) or
+ * battery dies during suspend.
+ */
+
+void suspend_remount_all_fs_ro(void)
+{
+	suspremount_list = NULL;
+	emergency_remount();
+}
+EXPORT_SYMBOL(suspend_remount_all_fs_ro);
+
+void resume_remount_fs_rw(void)
+{
+	struct suspremount *remountp;
+
+	remountp = suspremount_list;
+
+	while (remountp != NULL) {
+		struct suspremount *tp;
+		struct super_block *sb;
+		int flags, ret;
+
+		sb = remountp->sb;
+		flags = 0;
+		if (sb->s_op && sb->s_op->remount_fs) {
+			ret = sb->s_op->remount_fs(sb, &flags, NULL);
+			if (ret) printk("resume_remount_rw: error %d\n", ret);
+		}
+
+		tp = remountp->next;
+		kfree(remountp);
+		remountp = tp;
+	}
+	suspremount_list = NULL;
+}
+EXPORT_SYMBOL(resume_remount_fs_rw);
+#endif
+
 /**
  *	alloc_super	-	create new superblock
  *	@type:	filesystem type superblock should belong to
@@ -613,6 +677,9 @@ int do_remount_sb(struct super_block *sb
 		unlock_super(sb);
 		if (retval)
 			return retval;
+#ifdef CONFIG_SUSPEND_REMOUNTFS
+		suspend_remount_log_fs(sb);
+#endif
 	}
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	return 0;
diff -puN kernel/power/Kconfig~suspend_fs_ro kernel/power/Kconfig
--- linux-2.6_sdio/kernel/power/Kconfig~suspend_fs_ro	2007-02-01 13:52:42.000000000 -1000
+++ linux-2.6_sdio-akuster/kernel/power/Kconfig	2007-02-02 06:20:13.000000000 -1000
@@ -131,3 +131,14 @@ config SUSPEND_SMP
 	bool
 	depends on HOTPLUG_CPU && X86 && PM
 	default y
+
+config SUSPEND_REMOUNTFS
+	bool "Remount file system as ro on suspend"
+	depends on PM
+	default n
+	---help---
+	  Say Y here if you want to remount filesystems as read only
+	  prior to suspend, in case the power source is
+	  removed (ie, battery removed) or battery dies during suspend.
+	  Upon resume, file system mount state is restored.
+
diff -puN kernel/power/main.c~suspend_fs_ro kernel/power/main.c
--- linux-2.6_sdio/kernel/power/main.c~suspend_fs_ro	2007-02-01 13:57:56.000000000 -1000
+++ linux-2.6_sdio-akuster/kernel/power/main.c	2007-02-01 13:58:08.000000000 -1000
@@ -67,6 +67,16 @@ static int suspend_prepare(suspend_state
 	if (error)
 		goto Enable_cpu;
 
+#ifdef CONFIG_SUSPEND_REMOUNTFS
+	/*
+	* Remount filesystems prior to suspend, in case the
+	* power source is removed (ie, battery removed) or
+	* battery dies during suspend.
+	*/
+
+	suspend_remount_all_fs_ro();
+#endif
+
 	if (freeze_processes()) {
 		error = -EAGAIN;
 		goto Thaw;
@@ -97,6 +107,9 @@ static int suspend_prepare(suspend_state
 	if (pm_ops->finish)
 		pm_ops->finish(state);
  Thaw:
+#ifdef CONFIG_SUSPEND_REMOUNTFS
+	resume_remount_fs_rw();
+#endif
 	thaw_processes();
  Enable_cpu:
 	enable_nonboot_cpus();
diff -puN kernel/power/power.h~suspend_fs_ro kernel/power/power.h
--- linux-2.6_sdio/kernel/power/power.h~suspend_fs_ro	2007-02-01 13:58:49.000000000 -1000
+++ linux-2.6_sdio-akuster/kernel/power/power.h	2007-02-01 13:59:03.000000000 -1000
@@ -177,3 +177,9 @@ extern int suspend_enter(suspend_state_t
 struct timeval;
 extern void swsusp_show_speed(struct timeval *, struct timeval *,
 				unsigned int, char *);
+
+/* remount file system read only on suspend
+ * and retore to rw on resume
+ */
+extern void suspend_remount_all_fs_ro(void);
+extern void resume_remount_fs_rw(void);
_

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Andrew Morton]

On Fri, 02 Feb 2007 13:50:10 -1000
akuster@mvista.com wrote:

> 
> 
> This adds the ability for the file system to remounted as read only during a
> system suspend.  Log the mount points so when the resume occurs, they can be 
> remounted back to their original states. This is so in an advent of a power
> failure, we try our best to keep data from being corrupted or lost.
> 

Well the code appears simple enough, but I've not previously heard anyone
express a need for this feature.  But I know how to cc people who might
have heard this.


Minor coding-style observations:

> 
> diff -puN fs/super.c~suspend_fs_ro fs/super.c
> --- linux-2.6_sdio/fs/super.c~suspend_fs_ro	2007-02-01 13:35:46.000000000 -1000
> +++ linux-2.6_sdio-akuster/fs/super.c	2007-02-01 13:35:50.000000000 -1000
> @@ -47,6 +47,70 @@ struct file_system_type *get_fs_type(con
>  LIST_HEAD(super_blocks);
>  DEFINE_SPINLOCK(sb_lock);
>  
> +#ifdef CONFIG_SUSPEND_REMOUNTFS
> +/*
> + * Code to preserve filesystem data during suspend.
> + */
> +
> +struct suspremount {
> +struct super_block *sb;
> +struct suspremount *next;
> +};

The fields of this struct need a leading tab.

The name "suspremount" might be unpopular.  suspend_remount_state would be
more kernely.


> +static struct suspremount *suspremount_list;
> +
> +void suspend_remount_log_fs(struct super_block *sb)
> +{
> +	struct suspremount *remountp;
> +
> +	if ((remountp = (struct suspremount *)
> +		kmalloc(sizeof(struct suspremount), GFP_KERNEL)) != NULL) {

The typecast is unneeded, and the compounded assign-and-test is not
preferred style.  So here, please use

	struct suspremount *remountp;

	remountp = kmalloc(sizeof(*remountp), GFP_KERNEL);
	if (remountp != NULL) {

> +
> +/*
> + * Remount filesystems prior to suspend, in case the
> + * power source is removed (ie, battery removed) or
> + * battery dies during suspend.
> + */
> +
> +void suspend_remount_all_fs_ro(void)
> +{
> +	suspremount_list = NULL;
> +	emergency_remount();
> +}
> +EXPORT_SYMBOL(suspend_remount_all_fs_ro);

Why is this exported to modules?

> +void resume_remount_fs_rw(void)
> +{
> +	struct suspremount *remountp;
> +
> +	remountp = suspremount_list;
> +
> +	while (remountp != NULL) {
> +		struct suspremount *tp;
> +		struct super_block *sb;
> +		int flags, ret;
> +
> +		sb = remountp->sb;
> +		flags = 0;
> +		if (sb->s_op && sb->s_op->remount_fs) {
> +			ret = sb->s_op->remount_fs(sb, &flags, NULL);
> +			if (ret) printk("resume_remount_rw: error %d\n", ret);

newline needed here.

super_block_operations.remount_fs() is supposed to be called under lock_super().
Some filesystems might go BUG over this, or something.   Was there a reason to
not do this?

> +		}
> +
> +		tp = remountp->next;
> +		kfree(remountp);
> +		remountp = tp;
> +	}
> +	suspremount_list = NULL;
> +}
> +EXPORT_SYMBOL(resume_remount_fs_rw);

Why the export?

All this code is singly-threaded at a much higher level (I hope), hence
that list doesn't need locking.  However a comment explaining this might be
good.

> @@ -613,6 +677,9 @@ int do_remount_sb(struct super_block *sb
>  		unlock_super(sb);
>  		if (retval)
>  			return retval;
> +#ifdef CONFIG_SUSPEND_REMOUNTFS
> +		suspend_remount_log_fs(sb);
> +#endif

We try to avoid putting ifdefs in C files.  So in a header file, do

struct super_block;
#ifdef CONFIG_SUSPEND_REMOUNTFS
extern void suspend_remount_log_fs(struct super_block *sb);
#else
static inline void suspend_remount_log_fs(struct super_block *sb) {}
#endif

> +#ifdef CONFIG_SUSPEND_REMOUNTFS
> +	/*
> +	* Remount filesystems prior to suspend, in case the
> +	* power source is removed (ie, battery removed) or
> +	* battery dies during suspend.
> +	*/
> +
> +	suspend_remount_all_fs_ro();
> +#endif

Ditto here.

> +#ifdef CONFIG_SUSPEND_REMOUNTFS
> +	resume_remount_fs_rw();
> +#endif

And here.

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Henrique de Moraes Holschuh]

On Fri, 02 Feb 2007, Andrew Morton wrote:
> Well the code appears simple enough, but I've not previously heard anyone
> express a need for this feature.  But I know how to cc people who might
> have heard this.

You are, now.

We usually try to do it in userspace (and it gets ugly when we fail).  If
the kernel would do it, it would be very welcome.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[akuster]

Andrew Morton wrote:
> On Fri, 02 Feb 2007 13:50:10 -1000
> akuster@mvista.com wrote:
> 
>>
<snipped>

>> +struct suspremount {
>> +struct super_block *sb;
>> +struct suspremount *next;
>> +};
> 
> The fields of this struct need a leading tab.
ok.
> 
> The name "suspremount" might be unpopular.  suspend_remount_state would be
> more kernely.
> 
ok.

> 
>> +static struct suspremount *suspremount_list;
>> +
>> +void suspend_remount_log_fs(struct super_block *sb)
>> +{
>> +	struct suspremount *remountp;
>> +
>> +	if ((remountp = (struct suspremount *)
>> +		kmalloc(sizeof(struct suspremount), GFP_KERNEL)) != NULL) {
> 
> The typecast is unneeded, and the compounded assign-and-test is not
> preferred style.  So here, please use
> 
> 	struct suspremount *remountp;
> 
> 	remountp = kmalloc(sizeof(*remountp), GFP_KERNEL);
> 	if (remountp != NULL) {

ok.

>> +EXPORT_SYMBOL(suspend_remount_all_fs_ro);
> 
> Why is this exported to modules?
> 
it shouldn't. will remove


>> +		sb = remountp->sb;
>> +		flags = 0;
>> +		if (sb->s_op && sb->s_op->remount_fs) {
>> +			ret = sb->s_op->remount_fs(sb, &flags, NULL);
>> +			if (ret) printk("resume_remount_rw: error %d\n", ret);
> 
> newline needed here.

ok.
> 
> super_block_operations.remount_fs() is supposed to be called under lock_super().
> Some filesystems might go BUG over this, or something.   Was there a reason to
> not do this?

nope. will correct

> 
>> +		}
>> +
>> +		tp = remountp->next;
>> +		kfree(remountp);
>> +		remountp = tp;
>> +	}
>> +	suspremount_list = NULL;
>> +}
>> +EXPORT_SYMBOL(resume_remount_fs_rw);
> 
> Why the export?

shouldn't
> 
> All this code is singly-threaded at a much higher level (I hope), hence
> that list doesn't need locking.  However a comment explaining this might be
> good.

ok.

> 
>> @@ -613,6 +677,9 @@ int do_remount_sb(struct super_block *sb
>>  		unlock_super(sb);
>>  		if (retval)
>>  			return retval;
>> +#ifdef CONFIG_SUSPEND_REMOUNTFS
>> +		suspend_remount_log_fs(sb);
>> +#endif
> 
> We try to avoid putting ifdefs in C files.  So in a header file, do
> 
> struct super_block;
> #ifdef CONFIG_SUSPEND_REMOUNTFS
> extern void suspend_remount_log_fs(struct super_block *sb);
> #else
> static inline void suspend_remount_log_fs(struct super_block *sb) {}
> #endif
> 
will do.

Many thanks on the feedback.

Armin

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Christoph Hellwig]

On Fri, Feb 02, 2007 at 01:50:10PM -1000, akuster@mvista.com wrote:
> 
> 
> This adds the ability for the file system to remounted as read only during a
> system suspend.  Log the mount points so when the resume occurs, they can be 
> remounted back to their original states. This is so in an advent of a power
> failure, we try our best to keep data from being corrupted or lost.

Can you please explain why this can't be done in userspace?  In fact
all existing suspend solutions seem to be doing fine doing things like
this in userspace.

> +static struct suspremount *suspremount_list;
> +
> +void suspend_remount_log_fs(struct super_block *sb)
> +{
> +	struct suspremount *remountp;
> +
> +	if ((remountp = (struct suspremount *)
> +		kmalloc(sizeof(struct suspremount), GFP_KERNEL)) != NULL) {

No need to cast kmalloc return values, and please split assignments and
conditionals into separate lines.

> + * Remount filesystems prior to suspend, in case the
> + * power source is removed (ie, battery removed) or
> + * battery dies during suspend.
> + */
> +
> +void suspend_remount_all_fs_ro(void)
> +{
> +	suspremount_list = NULL;
> +	emergency_remount();

NACK.  emergency_remount is exactly what it sais and should never ever
be used for a system that you want to keep on using later on.  If you
look at it's implementation it's not correct and can't serve as more than
a bandaid for susrq.  In fact we should probably just remove it..

> +EXPORT_SYMBOL(suspend_remount_all_fs_ro);

And something like this for sure should not be exported.

> +EXPORT_SYMBOL(resume_remount_fs_rw);

ditto.

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Pavel Machek]

Hi!

> This adds the ability for the file system to remounted as read only during a
> system suspend.  Log the mount points so when the resume occurs, they can be 
> remounted back to their original states. This is so in an advent of a power
> failure, we try our best to keep data from being corrupted or lost.

We already have sys_sync() hidden in suspend process. So user data
should be safe.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[akuster]

Christoph Hellwig wrote:
> On Fri, Feb 02, 2007 at 01:50:10PM -1000, akuster@mvista.com wrote:
>>
>> This adds the ability for the file system to remounted as read only during a
>> system suspend.  Log the mount points so when the resume occurs, they can be 
>> remounted back to their original states. This is so in an advent of a power
>> failure, we try our best to keep data from being corrupted or lost.
> 
> Can you please explain why this can't be done in userspace?  

I am sure it can.  The idea came from customer inputs, speed is my 
guess. echo mem > /sys/../state seems a whole lot simpler and cleaner 
than having userspace figure out what it mounted and then doing echo mem.

In fact
> all existing suspend solutions seem to be doing fine doing things like
> this in userspace.

I guess I missed that boat. If you don't mind, could you point at one 
solution.

> 
>> +static struct suspremount *suspremount_list;
>> +
< snipped>

>> +void suspend_remount_all_fs_ro(void)
>> +{
>> +	suspremount_list = NULL;
>> +	emergency_remount();
> 
> NACK.  emergency_remount is exactly what it sais and should never ever
> be used for a system that you want to keep on using later on.  

The code "emergency_remount" is almost identical to what we have been 
using. I wanted to reuse existing code rather than dup it.

If you
> look at it's implementation it's not correct and can't serve as more than
> a bandaid for susrq. 

 From our customer experience, we have not gotten an bad feedback on it. 
  It seems to address there needs.

  In fact we should probably just remove it..
> 
>> +EXPORT_SYMBOL(suspend_remount_all_fs_ro);
> 
> And something like this for sure should not be exported.
> 
>> +EXPORT_SYMBOL(resume_remount_fs_rw);
> 
> ditto.
> 

All the code corrects have been address in my upcoming version.

Many thanks for you time and feedback.

Armin

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Christoph Hellwig]

On Sat, Feb 03, 2007 at 03:34:57PM -1000, akuster wrote:
> >Can you please explain why this can't be done in userspace?  
> 
> I am sure it can.  The idea came from customer inputs, speed is my 
> guess. echo mem > /sys/../state seems a whole lot simpler and cleaner 
> than having userspace figure out what it mounted and then doing echo mem.

I don't think we really want people to use the kernel interface directly.
Currently the distros seem to use the s2ram and s2disk helpers IIRC.

In any case there should be a userland abstraction doing all helper work
and finally calling into the kernel.

It would be nice if the embedded folk could work together with the desktop
people on the userland bits for power managment.

> In fact
> >all existing suspend solutions seem to be doing fine doing things like
> >this in userspace.
> 
> I guess I missed that boat. If you don't mind, could you point at one 
> solution.

> >NACK.  emergency_remount is exactly what it sais and should never ever
> >be used for a system that you want to keep on using later on.  
> 
> The code "emergency_remount" is almost identical to what we have been 
> using. I wanted to reuse existing code rather than dup it.

The problem with emergency_remount is that it calls do_remount_sb with
the force argument, which then calls into mark_files_ro.  This isn't
very useful because it doesn't actually stop existing writers and seems
to have a racy f_mode access.  Then again it doesn't really help either
that we want to get rid of file_list_lock the sb->s_files :)

If you really want to stop all current writes you need to call freeze_bdev,
and I'd rather expose that through a separate user interface instead of
hooking it into the suspend process at the kernel side.  For XFS you
could use the private ioctls already today, but I'd prefer a more general
solution.

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Nigel Cunningham]

Hi.

On Fri, 2007-02-02 at 22:35 -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 02 Feb 2007, Andrew Morton wrote:
> > Well the code appears simple enough, but I've not previously heard anyone
> > express a need for this feature.  But I know how to cc people who might
> > have heard this.
> 
> You are, now.
> 
> We usually try to do it in userspace (and it gets ugly when we fail).  If
> the kernel would do it, it would be very welcome.

Why do you think remounting filesystems is necessary? Are you getting
problems with some particular filesystem?

If I recall correctly, we briefly tried remounting filesystems in
Suspend2, but it created problems with logging - some files open r/w
were readonly afterwards. We then tried bdev freezing with much better
success.

Regards,

Nigel

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Christoph Hellwig]

On Tue, Feb 06, 2007 at 08:28:33AM +1100, Nigel Cunningham wrote:
> Why do you think remounting filesystems is necessary? Are you getting
> problems with some particular filesystem?
> 
> If I recall correctly, we briefly tried remounting filesystems in
> Suspend2, but it created problems with logging - some files open r/w
> were readonly afterwards.

Yes, that's the biggest problem with the forced remount r/o hack -
FMODE_WRITE is dropped from all files, and can't be recovered.
(and at the same time in progress writes or writes through shared
mmaps are totally ignored).

> We then tried bdev freezing with much better success.

Yes, that's the right thing to do if you want to be safe, but
we already had that discussion..

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Henrique de Moraes Holschuh]

On Tue, 06 Feb 2007, Nigel Cunningham wrote:
> Why do you think remounting filesystems is necessary? Are you getting
> problems with some particular filesystem?

No.  But anything in a removable device neets to be either remounted
read-only or unmounted if that is at all possible, because the user could
unplug it.  It is of course, sync'd anyway, so if the remount/umount fails,
no corruption should happen...  but the fs will be dirty, etc.

It can get very ugly when you factor in docks and removable bays. It's not
just USB/firewire mass-storage devices and memory cards.  And there is the
patological cases where the user suspends with the device in one port, and
resumes with the device in another port.

I feel userspace *can* do all that needs to be done, but we are (currently)
very bad at it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Rafael J. Wysocki]

On Tuesday, 6 February 2007 15:32, Henrique de Moraes Holschuh wrote:
> On Tue, 06 Feb 2007, Nigel Cunningham wrote:
> > Why do you think remounting filesystems is necessary? Are you getting
> > problems with some particular filesystem?
> 
> No.  But anything in a removable device neets to be either remounted
> read-only or unmounted if that is at all possible, because the user could
> unplug it.  It is of course, sync'd anyway, so if the remount/umount fails,
> no corruption should happen...  but the fs will be dirty, etc.
> 
> It can get very ugly when you factor in docks and removable bays. It's not
> just USB/firewire mass-storage devices and memory cards.  And there is the
> patological cases where the user suspends with the device in one port, and
> resumes with the device in another port.
> 
> I feel userspace *can* do all that needs to be done, but we are (currently)
> very bad at it.

Agreed, but still I think that we should try to solve these problems in user
space and only _after_ it turns out to be impossible or too hard to do we can
start to implement such things on the kernel side.

Greetings,
Rafael

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Nigel Cunningham]

Hi.

On Tue, 2007-02-06 at 12:32 -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 06 Feb 2007, Nigel Cunningham wrote:
> > Why do you think remounting filesystems is necessary? Are you getting
> > problems with some particular filesystem?
> 
> No.  But anything in a removable device neets to be either remounted
> read-only or unmounted if that is at all possible, because the user could
> unplug it.  It is of course, sync'd anyway, so if the remount/umount fails,
> no corruption should happen...  but the fs will be dirty, etc.
> 
> It can get very ugly when you factor in docks and removable bays. It's not
> just USB/firewire mass-storage devices and memory cards.  And there is the
> patological cases where the user suspends with the device in one port, and
> resumes with the device in another port.
> 
> I feel userspace *can* do all that needs to be done, but we are (currently)
> very bad at it.

Ok, as far as usage scenario goes, that's fair enough. But as to the
solution, I wonder though whether it's making life more complicated than
it needs to be. After all, we should also be able to cope okay with
having the power suddenly go out. If we can cope with that, cleaning
filesystems prior to suspending should be a non-issue.

Likewise with changes in hardware. Once hotplugging support is mature,
suspending, switching around hardware and resuming should just result in
hot[un]plug events.

Regards,

Nigel

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Henrique de Moraes Holschuh]

On Wed, 07 Feb 2007, Nigel Cunningham wrote:
> Ok, as far as usage scenario goes, that's fair enough. But as to the
> solution, I wonder though whether it's making life more complicated than
> it needs to be. After all, we should also be able to cope okay with
> having the power suddenly go out. If we can cope with that, cleaning
> filesystems prior to suspending should be a non-issue.

We don't cope okay with the power going out, at all.  And as an user case, a
need for fsck if you do something that is a reasonable use case (unplugging
devices while suspended) is not okay, either.

> Likewise with changes in hardware. Once hotplugging support is mature,
> suspending, switching around hardware and resuming should just result in
> hot[un]plug events.

Well, if we add *move* events for when someone unplugs a usb stick in one
port and replugs it in another while the system is in lala-land... maybe :-)
It would be normal to do it, when dealing with docks.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Nigel Cunningham]

Hi.

On Wed, 2007-02-07 at 09:25 -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 07 Feb 2007, Nigel Cunningham wrote:
> > Ok, as far as usage scenario goes, that's fair enough. But as to the
> > solution, I wonder though whether it's making life more complicated than
> > it needs to be. After all, we should also be able to cope okay with
> > having the power suddenly go out. If we can cope with that, cleaning
> > filesystems prior to suspending should be a non-issue.
> 
> We don't cope okay with the power going out, at all.  And as an user case, a
> need for fsck if you do something that is a reasonable use case (unplugging
> devices while suspended) is not okay, either.

Maybe it depends on the filesystem you use. I've used ext3 for 6 or so
years of development on Suspend2, and it's never given me a single
problem, despite the fact that I've sometimes done the equivalent of
pulling the plug without a sync or unmount. I did try XFS at one stage.
It's performance was better, but it did give problems. Nevertheless, I'm
more than happy to make the above claim about ext3.

> > Likewise with changes in hardware. Once hotplugging support is mature,
> > suspending, switching around hardware and resuming should just result in
> > hot[un]plug events.
> 
> Well, if we add *move* events for when someone unplugs a usb stick in one
> port and replugs it in another while the system is in lala-land... maybe :-)
> It would be normal to do it, when dealing with docks.

Isn't that part of the point to having those uuid thingys? I hate them
at the moment (from the point of view of suspend code), but hopefully
they'll end up being nicer to deal with.

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Henrique de Moraes Holschuh]

On Wed, 07 Feb 2007, Nigel Cunningham wrote:
> > We don't cope okay with the power going out, at all.  And as an user case, a
> > need for fsck if you do something that is a reasonable use case (unplugging
> > devices while suspended) is not okay, either.
> 
> Maybe it depends on the filesystem you use. I've used ext3 for 6 or so
> years of development on Suspend2, and it's never given me a single
> problem, despite the fact that I've sometimes done the equivalent of
> pulling the plug without a sync or unmount. I did try XFS at one stage.
> It's performance was better, but it did give problems. Nevertheless, I'm
> more than happy to make the above claim about ext3.

XFS comes to mind, indeed. But as I said, a need for fsck and unclean
partitions are enough to label it as an "unsuitable" solution.

> > > Likewise with changes in hardware. Once hotplugging support is mature,
> > > suspending, switching around hardware and resuming should just result in
> > > hot[un]plug events.
> > 
> > Well, if we add *move* events for when someone unplugs a usb stick in one
> > port and replugs it in another while the system is in lala-land... maybe :-)
> > It would be normal to do it, when dealing with docks.
> 
> Isn't that part of the point to having those uuid thingys? I hate them
> at the moment (from the point of view of suspend code), but hopefully
> they'll end up being nicer to deal with.

When you have files open for writing (thus neither mount R/O or umount will
suceed)?   No, you really need kernel support for this, and yes, I imagine
it is a royal pain to deal with these cases, they clearly belong on the "20%
of stuff that causes 80% of the work" side :-)

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Rafael J. Wysocki]

On Wednesday, 7 February 2007 13:05, Henrique de Moraes Holschuh wrote:
> On Wed, 07 Feb 2007, Nigel Cunningham wrote:
> > > We don't cope okay with the power going out, at all.  And as an user case, a
> > > need for fsck if you do something that is a reasonable use case (unplugging
> > > devices while suspended) is not okay, either.
> > 
> > Maybe it depends on the filesystem you use. I've used ext3 for 6 or so
> > years of development on Suspend2, and it's never given me a single
> > problem, despite the fact that I've sometimes done the equivalent of
> > pulling the plug without a sync or unmount. I did try XFS at one stage.
> > It's performance was better, but it did give problems. Nevertheless, I'm
> > more than happy to make the above claim about ext3.
> 
> XFS comes to mind, indeed. But as I said, a need for fsck and unclean
> partitions are enough to label it as an "unsuitable" solution.
> 
> > > > Likewise with changes in hardware. Once hotplugging support is mature,
> > > > suspending, switching around hardware and resuming should just result in
> > > > hot[un]plug events.
> > > 
> > > Well, if we add *move* events for when someone unplugs a usb stick in one
> > > port and replugs it in another while the system is in lala-land... maybe :-)
> > > It would be normal to do it, when dealing with docks.
> > 
> > Isn't that part of the point to having those uuid thingys? I hate them
> > at the moment (from the point of view of suspend code), but hopefully
> > they'll end up being nicer to deal with.
> 
> When you have files open for writing (thus neither mount R/O or umount will
> suceed)?   No, you really need kernel support for this, and yes, I imagine
> it is a royal pain to deal with these cases, they clearly belong on the "20%
> of stuff that causes 80% of the work" side :-)

However, there are devices that don't need such handling.  This means there is
a clear distinction between the devices (or filesystems) that are not going to
"move" at run time or when the machine is suspended (think PATA hard disks)
and the devices that can be "moved".

IMO the userland should be able to tell the kernel "this filesystem is movable"
to indicate that it needs special handling during the suspend etc., but none of
the existing filesystems implements something like that AFAICS.

Alternatively, if freeze_bdev() is made available to the userland somehow,
we could be able to handle the "removable filesystems" from the userland too.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Pavel Machek]

On Wed 2007-02-07 09:25:39, Henrique de Moraes Holschuh wrote:
> On Wed, 07 Feb 2007, Nigel Cunningham wrote:
> > Ok, as far as usage scenario goes, that's fair enough. But as to the
> > solution, I wonder though whether it's making life more complicated than
> > it needs to be. After all, we should also be able to cope okay with
> > having the power suddenly go out. If we can cope with that, cleaning
> > filesystems prior to suspending should be a non-issue.
> 
> We don't cope okay with the power going out, at all.  And as an user case, a
> need for fsck if you do something that is a reasonable use case (unplugging
> devices while suspended) is not okay, either.

It would be nice to umount devices over suspend, but I do not think
solution is as easy as patch that started this thread. For now it is
'dont do that' and fsck is nice reminder that you done something
wrong.

							Pavel

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

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Pavel Machek]

Hi!

> > Why do you think remounting filesystems is necessary? Are you getting
> > problems with some particular filesystem?
> 
> No.  But anything in a removable device neets to be either remounted
> read-only or unmounted if that is at all possible, because the user could
> unplug it.  It is of course, sync'd anyway, so if the remount/umount fails,
> no corruption should happen...  but the fs will be dirty, etc.
> 
> It can get very ugly when you factor in docks and removable bays. It's not
> just USB/firewire mass-storage devices and memory cards.  And there is the
> patological cases where the user suspends with the device in one port, and
> resumes with the device in another port.
> 
> I feel userspace *can* do all that needs to be done, but we are (currently)
> very bad at it.

Fix it in userspace, then. fsck when you unplug disk while suspended
should serve as a reminder to fix that userspace :-).

(And yes, you may need some kernel support to do it nicely; hopefully
same kernel support that will solve users unplugging disks without
umount.)
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/1] PM: Adds remount fs ro at suspend

[Pavel Machek]

Hi!

> >>This adds the ability for the file system to remounted as read only 
> >>during a
> >>system suspend.  Log the mount points so when the resume occurs, they can 
> >>be remounted back to their original states. This is so in an advent of a 
> >>power
> >>failure, we try our best to keep data from being corrupted or lost.
> >
> >Can you please explain why this can't be done in userspace?  
> 
> I am sure it can.  The idea came from customer inputs, speed is my 
> guess. echo mem > /sys/../state seems a whole lot simpler and cleaner 
> than having userspace figure out what it mounted and then doing echo
>mem.

Well, customers have little knowledge of linux kernel, sorry.

Do it in userspace, then find out if it is too slow, and if it is,
develop extensions to kernel to do it properly.

But no, you can't simply do it in kernel.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html