| Message ID | 201108062317.19033.rjw@sisk.pl |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 06, 2011 at 11:17:18PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Freeze all filesystems during the freezing of tasks by calling > freeze_bdev() for each of them and thaw them during the thawing > of tasks with the help of thaw_bdev(). > > This is needed by hibernation, because some filesystems (e.g. XFS) > deadlock with the preallocation of memory used by it if the memory > pressure caused by it is too heavy. > > The additional benefit of this change is that, if something goes > wrong after filesystems have been frozen, they will stay in a > consistent state and journal replays won't be necessary (e.g. after > a failing suspend or resume). In particular, this should help to > solve a long-standing issue that in some cases during resume from > hibernation the boot loader causes the journal to be replied for the > filesystem containing the kernel image and initrd causing it to > become inconsistent with the information stored in the hibernation > image. > > This change is based on earlier work by Nigel Cunningham. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > > OK, so nobody except for Pavel appears to have any comments, so I assume > that everyone except for Pavel is fine with the approach, interestingly enough. > > I've removed the MS_FROZEN Pavel complained about from freeze_filesystems() > and added comments explaining why lockdep_off/on() are used. > > Thanks, > Rafael > > --- > fs/block_dev.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 6 +++++ > kernel/power/process.c | 7 +++++- > 3 files changed, 68 insertions(+), 1 deletion(-) > > Index: linux-2.6/include/linux/fs.h > =================================================================== > --- linux-2.6.orig/include/linux/fs.h > +++ linux-2.6/include/linux/fs.h > @@ -211,6 +211,7 @@ struct inodes_stat_t { > #define MS_KERNMOUNT (1<<22) /* this is a kern_mount call */ > #define MS_I_VERSION (1<<23) /* Update inode I_version field */ > #define MS_STRICTATIME (1<<24) /* Always perform atime updates */ > +#define MS_FROZEN (1<<25) /* bdev has been frozen */ > #define MS_NOSEC (1<<28) > #define MS_BORN (1<<29) > #define MS_ACTIVE (1<<30) > @@ -2047,6 +2048,8 @@ extern struct super_block *freeze_bdev(s > extern void emergency_thaw_all(void); > extern int thaw_bdev(struct block_device *bdev, struct super_block *sb); > extern int fsync_bdev(struct block_device *); > +extern void freeze_filesystems(void); > +extern void thaw_filesystems(void); > #else > static inline void bd_forget(struct inode *inode) {} > static inline int sync_blockdev(struct block_device *bdev) { return 0; } > @@ -2061,6 +2064,9 @@ static inline int thaw_bdev(struct block > { > return 0; > } > + > +static inline void freeze_filesystems(void) {} > +static inline void thaw_filesystems(void) {} > #endif > extern int sync_filesystem(struct super_block *); > extern const struct file_operations def_blk_fops; > Index: linux-2.6/fs/block_dev.c > =================================================================== > --- linux-2.6.orig/fs/block_dev.c > +++ linux-2.6/fs/block_dev.c > @@ -314,6 +314,62 @@ out: > } > EXPORT_SYMBOL(thaw_bdev); > > +/** > + * freeze_filesystems - Force all filesystems into a consistent state. > + */ > +void freeze_filesystems(void) > +{ > + struct super_block *sb; > + > + /* > + * This is necessary, because some filesystems (e.g. ext3) lock > + * mutexes in their .freeze_fs() callbacks and leave them locked for > + * their .unfreeze_fs() callbacks to unlock. This is done under > + * bdev->bd_fsfreeze_mutex, which is then released, but it makes > + * lockdep think something may be wrong when freeze_bdev() attempts > + * to acquire bdev->bd_fsfreeze_mutex for the next filesystem. > + */ > + lockdep_off(); I thought those problems were fixed. If they aren't, then they most certainly need to be because holding mutexes over system calls is a bug. Well, well: [252182.603134] ================================================ [252182.604832] [ BUG: lock held when returning to user space! ] [252182.606086] ------------------------------------------------ [252182.607400] xfs_io/4917 is leaving the kernel with locks still held! [252182.608905] 1 lock held by xfs_io/4917: [252182.609739] #0: (&journal->j_barrier){+.+...}, at: [<ffffffff812a2aaf>] journal_lock_updates+0xef/0x100 <sigh> Looks like the problem was fixed for ext4, but not ext3. Please report this to the ext3/4 list and get it fixed, don't work around it here. > + /* > + * Freeze in reverse order so filesystems depending on others are > + * frozen in the right order (eg. loopback on ext3). > + */ > + list_for_each_entry_reverse(sb, &super_blocks, s_list) { > + if (!sb->s_root || !sb->s_bdev || > + (sb->s_frozen == SB_FREEZE_TRANS) || > + (sb->s_flags & MS_RDONLY)) > + continue; > + > + freeze_bdev(sb->s_bdev); > + sb->s_flags |= MS_FROZEN; > + } AFAIK, that won't work for btrfs - you have to call freeze_super() directly for btrfs because it has a special relationship with sb->s_bdev. And besides, all freeze_bdev does is get an active reference on the superblock and call freeze_super(). Also, that's traversing the list of superblock with locking and dereferencing the superblock without properly checking that the superblock is not being torn down. You should probably use iterate_supers (or at least copy the code), with a function that drops the s_umount read lock befor calling freeze_super() and then picks it back up afterwards. > + > + lockdep_on(); > +} > + > +/** > + * thaw_filesystems - Make all filesystems active again. > + */ > +void thaw_filesystems(void) > +{ > + struct super_block *sb; > + > + /* > + * This is necessary for the same reason as in freeze_filesystems() > + * above. > + */ > + lockdep_off(); > + > + list_for_each_entry(sb, &super_blocks, s_list) > + if (sb->s_flags & MS_FROZEN) { > + sb->s_flags &= ~MS_FROZEN; > + thaw_bdev(sb->s_bdev, sb); > + } And once again, iterate_supers() is what you want here. And you should only call thaw_bdev() as it needs to do checks other than checking MS_FROZEN e.g. the above will unfreeze filesystems that were already frozen at the time a suspend occurs, and that could lead to corruption depending on why the filesystem was frozen... Also, you still need to check for a valid sb->s_bdev here, otherwise <splat>. Cheers, Dave.
On Sunday, August 07, 2011, Dave Chinner wrote: > On Sat, Aug 06, 2011 at 11:17:18PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > Freeze all filesystems during the freezing of tasks by calling > > freeze_bdev() for each of them and thaw them during the thawing > > of tasks with the help of thaw_bdev(). > > > > This is needed by hibernation, because some filesystems (e.g. XFS) > > deadlock with the preallocation of memory used by it if the memory > > pressure caused by it is too heavy. > > > > The additional benefit of this change is that, if something goes > > wrong after filesystems have been frozen, they will stay in a > > consistent state and journal replays won't be necessary (e.g. after > > a failing suspend or resume). In particular, this should help to > > solve a long-standing issue that in some cases during resume from > > hibernation the boot loader causes the journal to be replied for the > > filesystem containing the kernel image and initrd causing it to > > become inconsistent with the information stored in the hibernation > > image. > > > > This change is based on earlier work by Nigel Cunningham. > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > > --- > > > > OK, so nobody except for Pavel appears to have any comments, so I assume > > that everyone except for Pavel is fine with the approach, interestingly enough. > > > > I've removed the MS_FROZEN Pavel complained about from freeze_filesystems() > > and added comments explaining why lockdep_off/on() are used. > > > > Thanks, > > Rafael > > > > --- > > fs/block_dev.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/fs.h | 6 +++++ > > kernel/power/process.c | 7 +++++- > > 3 files changed, 68 insertions(+), 1 deletion(-) > > > > Index: linux-2.6/include/linux/fs.h > > =================================================================== > > --- linux-2.6.orig/include/linux/fs.h > > +++ linux-2.6/include/linux/fs.h > > @@ -211,6 +211,7 @@ struct inodes_stat_t { > > #define MS_KERNMOUNT (1<<22) /* this is a kern_mount call */ > > #define MS_I_VERSION (1<<23) /* Update inode I_version field */ > > #define MS_STRICTATIME (1<<24) /* Always perform atime updates */ > > +#define MS_FROZEN (1<<25) /* bdev has been frozen */ > > #define MS_NOSEC (1<<28) > > #define MS_BORN (1<<29) > > #define MS_ACTIVE (1<<30) > > @@ -2047,6 +2048,8 @@ extern struct super_block *freeze_bdev(s > > extern void emergency_thaw_all(void); > > extern int thaw_bdev(struct block_device *bdev, struct super_block *sb); > > extern int fsync_bdev(struct block_device *); > > +extern void freeze_filesystems(void); > > +extern void thaw_filesystems(void); > > #else > > static inline void bd_forget(struct inode *inode) {} > > static inline int sync_blockdev(struct block_device *bdev) { return 0; } > > @@ -2061,6 +2064,9 @@ static inline int thaw_bdev(struct block > > { > > return 0; > > } > > + > > +static inline void freeze_filesystems(void) {} > > +static inline void thaw_filesystems(void) {} > > #endif > > extern int sync_filesystem(struct super_block *); > > extern const struct file_operations def_blk_fops; > > Index: linux-2.6/fs/block_dev.c > > =================================================================== > > --- linux-2.6.orig/fs/block_dev.c > > +++ linux-2.6/fs/block_dev.c > > @@ -314,6 +314,62 @@ out: > > } > > EXPORT_SYMBOL(thaw_bdev); > > > > +/** > > + * freeze_filesystems - Force all filesystems into a consistent state. > > + */ > > +void freeze_filesystems(void) > > +{ > > + struct super_block *sb; > > + > > + /* > > + * This is necessary, because some filesystems (e.g. ext3) lock > > + * mutexes in their .freeze_fs() callbacks and leave them locked for > > + * their .unfreeze_fs() callbacks to unlock. This is done under > > + * bdev->bd_fsfreeze_mutex, which is then released, but it makes > > + * lockdep think something may be wrong when freeze_bdev() attempts > > + * to acquire bdev->bd_fsfreeze_mutex for the next filesystem. > > + */ > > + lockdep_off(); > > I thought those problems were fixed. If they aren't, then they most > certainly need to be because holding mutexes over system calls is a > bug. > > Well, well: > > [252182.603134] ================================================ > [252182.604832] [ BUG: lock held when returning to user space! ] > [252182.606086] ------------------------------------------------ > [252182.607400] xfs_io/4917 is leaving the kernel with locks still held! > [252182.608905] 1 lock held by xfs_io/4917: > [252182.609739] #0: (&journal->j_barrier){+.+...}, at: [<ffffffff812a2aaf>] journal_lock_updates+0xef/0x100 > > <sigh> > > Looks like the problem was fixed for ext4, but not ext3. Please > report this to the ext3/4 list and get it fixed, don't work around > it here. OK, but I guess I'll have to post a patch to fix this myself so that anyone notices. :-) > > + /* > > + * Freeze in reverse order so filesystems depending on others are > > + * frozen in the right order (eg. loopback on ext3). > > + */ > > + list_for_each_entry_reverse(sb, &super_blocks, s_list) { > > + if (!sb->s_root || !sb->s_bdev || > > + (sb->s_frozen == SB_FREEZE_TRANS) || > > + (sb->s_flags & MS_RDONLY)) > > + continue; > > + > > + freeze_bdev(sb->s_bdev); > > + sb->s_flags |= MS_FROZEN; > > + } > > AFAIK, that won't work for btrfs - you have to call freeze_super() > directly for btrfs because it has a special relationship with > sb->s_bdev. And besides, all freeze_bdev does is get an active > reference on the superblock and call freeze_super(). OK, so do you mean I should call freeze_super() rather than freeze_bdev()? > Also, that's traversing the list of superblock with locking and > dereferencing the superblock without properly checking that the > superblock is not being torn down. You should probably use > iterate_supers (or at least copy the code), with a function that > drops the s_umount read lock befor calling freeze_super() and then > picks it back up afterwards. Hmm, I'll try that, but I doubt I'll get it right first time. :-) > > + > > + lockdep_on(); > > +} > > + > > +/** > > + * thaw_filesystems - Make all filesystems active again. > > + */ > > +void thaw_filesystems(void) > > +{ > > + struct super_block *sb; > > + > > + /* > > + * This is necessary for the same reason as in freeze_filesystems() > > + * above. > > + */ > > + lockdep_off(); > > + > > + list_for_each_entry(sb, &super_blocks, s_list) > > + if (sb->s_flags & MS_FROZEN) { > > + sb->s_flags &= ~MS_FROZEN; > > + thaw_bdev(sb->s_bdev, sb); > > + } > > And once again, iterate_supers() is what you want here. OK > And you should only call thaw_bdev() as it needs to do checks other > than checking MS_FROZEN Hmm, I'm not really sure what you mean? > e.g. the above will unfreeze filesystems that > were already frozen at the time a suspend occurs, and that could > lead to corruption depending on why the filesystem was frozen... > > Also, you still need to check for a valid sb->s_bdev here, otherwise > <splat>. I see. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/include/linux/fs.h =================================================================== --- linux-2.6.orig/include/linux/fs.h +++ linux-2.6/include/linux/fs.h @@ -211,6 +211,7 @@ struct inodes_stat_t { #define MS_KERNMOUNT (1<<22) /* this is a kern_mount call */ #define MS_I_VERSION (1<<23) /* Update inode I_version field */ #define MS_STRICTATIME (1<<24) /* Always perform atime updates */ +#define MS_FROZEN (1<<25) /* bdev has been frozen */ #define MS_NOSEC (1<<28) #define MS_BORN (1<<29) #define MS_ACTIVE (1<<30) @@ -2047,6 +2048,8 @@ extern struct super_block *freeze_bdev(s extern void emergency_thaw_all(void); extern int thaw_bdev(struct block_device *bdev, struct super_block *sb); extern int fsync_bdev(struct block_device *); +extern void freeze_filesystems(void); +extern void thaw_filesystems(void); #else static inline void bd_forget(struct inode *inode) {} static inline int sync_blockdev(struct block_device *bdev) { return 0; } @@ -2061,6 +2064,9 @@ static inline int thaw_bdev(struct block { return 0; } + +static inline void freeze_filesystems(void) {} +static inline void thaw_filesystems(void) {} #endif extern int sync_filesystem(struct super_block *); extern const struct file_operations def_blk_fops; Index: linux-2.6/fs/block_dev.c =================================================================== --- linux-2.6.orig/fs/block_dev.c +++ linux-2.6/fs/block_dev.c @@ -314,6 +314,62 @@ out: } EXPORT_SYMBOL(thaw_bdev); +/** + * freeze_filesystems - Force all filesystems into a consistent state. + */ +void freeze_filesystems(void) +{ + struct super_block *sb; + + /* + * This is necessary, because some filesystems (e.g. ext3) lock + * mutexes in their .freeze_fs() callbacks and leave them locked for + * their .unfreeze_fs() callbacks to unlock. This is done under + * bdev->bd_fsfreeze_mutex, which is then released, but it makes + * lockdep think something may be wrong when freeze_bdev() attempts + * to acquire bdev->bd_fsfreeze_mutex for the next filesystem. + */ + lockdep_off(); + + /* + * Freeze in reverse order so filesystems depending on others are + * frozen in the right order (eg. loopback on ext3). + */ + list_for_each_entry_reverse(sb, &super_blocks, s_list) { + if (!sb->s_root || !sb->s_bdev || + (sb->s_frozen == SB_FREEZE_TRANS) || + (sb->s_flags & MS_RDONLY)) + continue; + + freeze_bdev(sb->s_bdev); + sb->s_flags |= MS_FROZEN; + } + + lockdep_on(); +} + +/** + * thaw_filesystems - Make all filesystems active again. + */ +void thaw_filesystems(void) +{ + struct super_block *sb; + + /* + * This is necessary for the same reason as in freeze_filesystems() + * above. + */ + lockdep_off(); + + list_for_each_entry(sb, &super_blocks, s_list) + if (sb->s_flags & MS_FROZEN) { + sb->s_flags &= ~MS_FROZEN; + thaw_bdev(sb->s_bdev, sb); + } + + lockdep_on(); +} + static int blkdev_writepage(struct page *page, struct writeback_control *wbc) { return block_write_full_page(page, blkdev_get_block, wbc); Index: linux-2.6/kernel/power/process.c =================================================================== --- linux-2.6.orig/kernel/power/process.c +++ linux-2.6/kernel/power/process.c @@ -12,10 +12,10 @@ #include <linux/oom.h> #include <linux/suspend.h> #include <linux/module.h> -#include <linux/syscalls.h> #include <linux/freezer.h> #include <linux/delay.h> #include <linux/workqueue.h> +#include <linux/fs.h> /* * Timeout for stopping processes @@ -147,6 +147,10 @@ int freeze_processes(void) goto Exit; printk("done.\n"); + pr_info("Freezing filesystems ... "); + freeze_filesystems(); + pr_info("done.\n"); + printk("Freezing remaining freezable tasks ... "); error = try_to_freeze_tasks(false); if (error) @@ -188,6 +192,7 @@ void thaw_processes(void) printk("Restarting tasks ... "); thaw_workqueues(); thaw_tasks(true); + thaw_filesystems(); thaw_tasks(false); schedule(); printk("done.\n");