Messages in this thread Patch in this message | | | Date | Fri, 2 Feb 2007 16:16:11 -0800 | From | Andrew Morton <> | Subject | Re: [patch 1/1] PM: Adds remount fs ro at suspend |
| |
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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |