lkml.org 
[lkml]   [2007]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-02-03 01:21    [W:4.479 / U:0.244 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site