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

[Christoph Hellwig]

On Fri, Feb 02, 2007 at 01:50:10PM -1000, akuster@xxxxxxxxxx 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/