Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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.. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
Christoph Hellwig wrote:
On Fri, Feb 02, 2007 at 01:50:10PM -1000, [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
On Fri, Feb 02, 2007 at 01:50:10PM -1000, [EMAIL PROTECTED] 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
Andrew Morton wrote:
On Fri, 02 Feb 2007 13:50:10 -1000
[EMAIL PROTECTED] wrote:
+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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] PM: Adds remount fs ro at suspend
On Fri, 02 Feb 2007 13:50:10 -1000
[EMAIL PROTECTED] 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.0
> -1000
> +++ linux-2.6_sdio-akuster/fs/super.c 2007-02-01 13:35:50.0 -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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 1/1] PM: Adds remount fs ro at suspend
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 <[EMAIL PROTECTED]>
---
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.0
-1000
+++ linux-2.6_sdio-akuster/fs/super.c 2007-02-01 13:35:50.0 -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.0 -1000
+++ linux-2.6_sdio-akuster/kernel/power/Kconfig 2007-02-02 06:20:13.0
-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_ro2007-02-01
13:57:56.0 -1000
+++ linux-2.6_sdio-akuster/kernel/power/main.c 2007-02-01 13:58:08.0
-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.0 -1000
+++ linux-2.6_sdio-akuster/kernel/power/power.h 2007-02-01 13:59:03.0
