| Message ID | 20230508011717.4034511-2-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | vfs: provide automatic kernel freeze / resume | expand |
On Sun, May 07, 2023 at 06:17:12PM -0700, Luis Chamberlain wrote: > Right now freeze_super() and thaw_super() are called with > different locking contexts. To expand on this is messy, so > just unify the requirement to require grabbing an active > reference and keep the superblock locked. > > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > block/bdev.c | 5 +++- > fs/f2fs/gc.c | 5 ++++ > fs/gfs2/super.c | 9 +++++-- > fs/gfs2/sys.c | 6 +++++ > fs/gfs2/util.c | 5 ++++ > fs/ioctl.c | 12 ++++++++-- > fs/super.c | 62 ++++++++++++++++++------------------------------- > 7 files changed, 60 insertions(+), 44 deletions(-) > > diff --git a/block/bdev.c b/block/bdev.c > index 21c63bfef323..dc54a2a1c46e 100644 > --- a/block/bdev.c > +++ b/block/bdev.c > @@ -251,7 +251,7 @@ int freeze_bdev(struct block_device *bdev) > error = sb->s_op->freeze_super(sb); > else > error = freeze_super(sb); > - deactivate_super(sb); > + deactivate_locked_super(sb); > > if (error) { > bdev->bd_fsfreeze_count--; > @@ -289,6 +289,8 @@ int thaw_bdev(struct block_device *bdev) > sb = bdev->bd_fsfreeze_sb; > if (!sb) > goto out; > + if (!get_active_super(bdev)) > + goto out; > > if (sb->s_op->thaw_super) > error = sb->s_op->thaw_super(sb); > @@ -298,6 +300,7 @@ int thaw_bdev(struct block_device *bdev) > bdev->bd_fsfreeze_count++; > else > bdev->bd_fsfreeze_sb = NULL; > + deactivate_locked_super(sb); > out: > mutex_unlock(&bdev->bd_fsfreeze_mutex); > return error; > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 61c5f9d26018..e31d6791d3e3 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -2166,7 +2166,10 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count) > if (err) > return err; > > + if (!get_active_super(sbi->sb->s_bdev)) > + return -ENOTTY; > freeze_super(sbi->sb); > + > f2fs_down_write(&sbi->gc_lock); > f2fs_down_write(&sbi->cp_global_sem); > > @@ -2217,6 +2220,8 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count) > out_err: > f2fs_up_write(&sbi->cp_global_sem); > f2fs_up_write(&sbi->gc_lock); > + /* We use the same active reference from freeze */ > thaw_super(sbi->sb); > + deactivate_locked_super(sbi->sb); > return err; > } > diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c > index 5eed8c237500..e57cb593e2f3 100644 > --- a/fs/gfs2/super.c > +++ b/fs/gfs2/super.c > @@ -676,7 +676,12 @@ void gfs2_freeze_func(struct work_struct *work) > struct gfs2_sbd *sdp = container_of(work, struct gfs2_sbd, sd_freeze_work); > struct super_block *sb = sdp->sd_vfs; > > - atomic_inc(&sb->s_active); > + if (!get_active_super(sb->s_bdev)) { > + fs_info(sdp, "GFS2: couldn't grap super for thaw for filesystem\n"); > + gfs2_assert_withdraw(sdp, 0); > + return; > + } > + > error = gfs2_freeze_lock(sdp, &freeze_gh, 0); > if (error) { > gfs2_assert_withdraw(sdp, 0); > @@ -690,7 +695,7 @@ void gfs2_freeze_func(struct work_struct *work) > } > gfs2_freeze_unlock(&freeze_gh); > } > - deactivate_super(sb); > + deactivate_locked_super(sb); > clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags); > wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN); > return; > diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c > index 454dc2ff8b5e..cbb71c3520c0 100644 > --- a/fs/gfs2/sys.c > +++ b/fs/gfs2/sys.c > @@ -164,6 +164,9 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len) > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > + if (!get_active_super(sb->s_bdev)) > + return -ENOTTY; > + > switch (n) { > case 0: > error = thaw_super(sdp->sd_vfs); > @@ -172,9 +175,12 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len) > error = freeze_super(sdp->sd_vfs); > break; > default: > + deactivate_locked_super(sb); > return -EINVAL; > } > > + deactivate_locked_super(sb); > + > if (error) { > fs_warn(sdp, "freeze %d error %d\n", n, error); > return error; > diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c > index 7a6aeffcdf5c..3a0cd5e9ad84 100644 > --- a/fs/gfs2/util.c > +++ b/fs/gfs2/util.c > @@ -345,10 +345,15 @@ int gfs2_withdraw(struct gfs2_sbd *sdp) > set_bit(SDF_WITHDRAW_IN_PROG, &sdp->sd_flags); > > if (sdp->sd_args.ar_errors == GFS2_ERRORS_WITHDRAW) { > + if (!get_active_super(sb->s_bdev)) { > + fs_err(sdp, "could not grab super on withdraw for file system\n"); > + return -1; > + } > fs_err(sdp, "about to withdraw this file system\n"); > BUG_ON(sdp->sd_args.ar_debug); > > signal_our_withdraw(sdp); > + deactivate_locked_super(sb); > > kobject_uevent(&sdp->sd_kobj, KOBJ_OFFLINE); > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 5b2481cd4750..1d20af762e0d 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -386,6 +386,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > static int ioctl_fsfreeze(struct file *filp) > { > struct super_block *sb = file_inode(filp)->i_sb; > + int ret; > > if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) > return -EPERM; > @@ -394,10 +395,17 @@ static int ioctl_fsfreeze(struct file *filp) > if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL) > return -EOPNOTSUPP; > > + if (!get_active_super(sb->s_bdev)) Um... doesn't this mean that we now cannot freeze network and pseudo filesystems? > + return -ENOTTY; > + > /* Freeze */ > if (sb->s_op->freeze_super) > - return sb->s_op->freeze_super(sb); > - return freeze_super(sb); > + ret = sb->s_op->freeze_super(sb); > + ret = freeze_super(sb); > + > + deactivate_locked_super(sb); > + > + return ret; > } > > static int ioctl_fsthaw(struct file *filp) > diff --git a/fs/super.c b/fs/super.c > index 34afe411cf2b..0e9d48846684 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -39,8 +39,6 @@ > #include <uapi/linux/mount.h> > #include "internal.h" > > -static int thaw_super_locked(struct super_block *sb); > - > static LIST_HEAD(super_blocks); > static DEFINE_SPINLOCK(sb_lock); > > @@ -851,13 +849,13 @@ struct super_block *get_active_super(struct block_device *bdev) > if (sb->s_bdev == bdev) { > if (!grab_super(sb)) > goto restart; > - up_write(&sb->s_umount); > return sb; > } > } > spin_unlock(&sb_lock); > return NULL; > } > +EXPORT_SYMBOL_GPL(get_active_super); > > struct super_block *user_get_super(dev_t dev, bool excl) > { > @@ -1024,13 +1022,13 @@ void emergency_remount(void) > > static void do_thaw_all_callback(struct super_block *sb) > { > - down_write(&sb->s_umount); > + if (!get_active_super(sb->s_bdev)) > + return; > if (sb->s_root && sb->s_flags & SB_BORN) { > emergency_thaw_bdev(sb); > - thaw_super_locked(sb); > - } else { > - up_write(&sb->s_umount); > + thaw_super(sb); > } > + deactivate_locked_super(sb); > } > > static void do_thaw_all(struct work_struct *work) > @@ -1636,10 +1634,13 @@ static void sb_freeze_unlock(struct super_block *sb, int level) > } > > /** > - * freeze_super - lock the filesystem and force it into a consistent state > + * freeze_super - force a filesystem backed by a block device into a consistent state > * @sb: the super to lock > * > - * Syncs the super to make sure the filesystem is consistent and calls the fs's > + * Used by filesystems and the kernel to freeze a fileystem backed by a block > + * device into a consistent state. Callers must use get_active_super(bdev) to > + * lock the @sb and when done must unlock it with deactivate_locked_super(). > + * Syncs the filesystem backed by the @sb and calls the filesystem's optional > * freeze_fs. Subsequent calls to this without first thawing the fs will return > * -EBUSY. > * > @@ -1672,22 +1673,15 @@ int freeze_super(struct super_block *sb) > { > int ret; > > - atomic_inc(&sb->s_active); > - down_write(&sb->s_umount); > - if (sb->s_writers.frozen != SB_UNFROZEN) { > - deactivate_locked_super(sb); > + if (sb->s_writers.frozen != SB_UNFROZEN) > return -EBUSY; > - } > > - if (!(sb->s_flags & SB_BORN)) { > - up_write(&sb->s_umount); > + if (!(sb->s_flags & SB_BORN)) > return 0; /* sic - it's "nothing to do" */ > - } > > if (sb_rdonly(sb)) { > /* Nothing to do really... */ > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > - up_write(&sb->s_umount); > return 0; > } > > @@ -1707,7 +1701,6 @@ int freeze_super(struct super_block *sb) > sb->s_writers.frozen = SB_UNFROZEN; > sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT); > wake_up(&sb->s_writers.wait_unfrozen); > - deactivate_locked_super(sb); > return ret; > } > > @@ -1723,7 +1716,6 @@ int freeze_super(struct super_block *sb) > sb->s_writers.frozen = SB_UNFROZEN; > sb_freeze_unlock(sb, SB_FREEZE_FS); > wake_up(&sb->s_writers.wait_unfrozen); > - deactivate_locked_super(sb); > return ret; > } > } > @@ -1733,19 +1725,25 @@ int freeze_super(struct super_block *sb) > */ > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > lockdep_sb_freeze_release(sb); > - up_write(&sb->s_umount); > return 0; Also ... before this change, a successful freeze means that we retain the elevated s_active during the freeze. Now the callers of this function always call deactivate_locked_super, which decrements the active count, even if the fs is actually frozen. The active count no longer works the way it used to. Can that lead to freeing a frozen super? Alternately, I guess we could take our own s_active reference here. --D > } > EXPORT_SYMBOL(freeze_super); > > -static int thaw_super_locked(struct super_block *sb) > +/** > + * thaw_super -- unlock a filesystem backed by a block device > + * @sb: the super to thaw > + * > + * Used by filesystems and the kernel to thaw a fileystem backed by a block > + * device. Callers must use get_active_super(bdev) to lock the @sb and when > + * done must unlock it with deactivate_locked_super(). Once done, this marks > + * the filesystem as writeable. > + */ > +int thaw_super(struct super_block *sb) > { > int error; > > - if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) { > - up_write(&sb->s_umount); > + if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) > return -EINVAL; > - } > > if (sb_rdonly(sb)) { > sb->s_writers.frozen = SB_UNFROZEN; > @@ -1760,7 +1758,6 @@ static int thaw_super_locked(struct super_block *sb) > printk(KERN_ERR > "VFS:Filesystem thaw failed\n"); > lockdep_sb_freeze_release(sb); > - up_write(&sb->s_umount); > return error; > } > } > @@ -1769,21 +1766,8 @@ static int thaw_super_locked(struct super_block *sb) > sb_freeze_unlock(sb, SB_FREEZE_FS); > out: > wake_up(&sb->s_writers.wait_unfrozen); > - deactivate_locked_super(sb); > return 0; > } > - > -/** > - * thaw_super -- unlock filesystem > - * @sb: the super to thaw > - * > - * Unlocks the filesystem and marks it writeable again after freeze_super(). > - */ > -int thaw_super(struct super_block *sb) > -{ > - down_write(&sb->s_umount); > - return thaw_super_locked(sb); > -} > EXPORT_SYMBOL(thaw_super); > > /* > -- > 2.39.2 >
On Sun 07-05-23 18:17:12, Luis Chamberlain wrote: > Right now freeze_super() and thaw_super() are called with > different locking contexts. To expand on this is messy, so > just unify the requirement to require grabbing an active > reference and keep the superblock locked. > > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Finally got around to looking at this. Sorry for the delay. In principle I like the direction but see below: > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 61c5f9d26018..e31d6791d3e3 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -2166,7 +2166,10 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count) > if (err) > return err; > > + if (!get_active_super(sbi->sb->s_bdev)) > + return -ENOTTY; Calling get_active_super() like this is just sick. You rather want to provide a helper for grabbing another active sb reference and locking the sb when you already have sb reference. Because that is what is needed in the vast majority of the places. Something like void grab_active_super(struct super_block *sb) { down_write(sb->s_umount); atomic_inc(&s->s_active); } > @@ -851,13 +849,13 @@ struct super_block *get_active_super(struct block_device *bdev) > if (sb->s_bdev == bdev) { > if (!grab_super(sb)) > goto restart; > - up_write(&sb->s_umount); > return sb; > } > } > spin_unlock(&sb_lock); > return NULL; > } > +EXPORT_SYMBOL_GPL(get_active_super); And I'd call this grab_bdev_super() and no need to export it when you have grab_active_super(). > @@ -1636,10 +1634,13 @@ static void sb_freeze_unlock(struct super_block *sb, int level) > } > > /** > - * freeze_super - lock the filesystem and force it into a consistent state > + * freeze_super - force a filesystem backed by a block device into a consistent state > * @sb: the super to lock > * > - * Syncs the super to make sure the filesystem is consistent and calls the fs's > + * Used by filesystems and the kernel to freeze a fileystem backed by a block > + * device into a consistent state. Callers must use get_active_super(bdev) to > + * lock the @sb and when done must unlock it with deactivate_locked_super(). > + * Syncs the filesystem backed by the @sb and calls the filesystem's optional > * freeze_fs. Subsequent calls to this without first thawing the fs will return > * -EBUSY. > * > @@ -1672,22 +1673,15 @@ int freeze_super(struct super_block *sb) > { > int ret; > > - atomic_inc(&sb->s_active); > - down_write(&sb->s_umount); > - if (sb->s_writers.frozen != SB_UNFROZEN) { > - deactivate_locked_super(sb); At least add a warning for s_umount not being held here? > + if (sb->s_writers.frozen != SB_UNFROZEN) > return -EBUSY; > - } > > - if (!(sb->s_flags & SB_BORN)) { > - up_write(&sb->s_umount); > + if (!(sb->s_flags & SB_BORN)) > return 0; /* sic - it's "nothing to do" */ > - } > > if (sb_rdonly(sb)) { > /* Nothing to do really... */ > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > - up_write(&sb->s_umount); > return 0; > } Honza
On Sun, May 07, 2023 at 06:17:12PM -0700, Luis Chamberlain wrote: > Right now freeze_super() and thaw_super() are called with > different locking contexts. To expand on this is messy, so > just unify the requirement to require grabbing an active > reference and keep the superblock locked. > > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Maybe I'm just getting old, but where did I suggest this? That being said, holding an active reference over any operation is a good thing. As Jan said it can be done way simpler than this, though. Also please explain the actual different locking contexts and what semantics you unify in the commit log please. > - * freeze_super - lock the filesystem and force it into a consistent state > + * freeze_super - force a filesystem backed by a block device into a consistent state > * @sb: the super to lock This is not actually true. Nothing in here has anything to do with block devices, and it is used on a at least one non-block file system.
On Wed, Jun 07, 2023 at 10:01:14PM -0700, Christoph Hellwig wrote: > On Sun, May 07, 2023 at 06:17:12PM -0700, Luis Chamberlain wrote: > > Right now freeze_super() and thaw_super() are called with > > different locking contexts. To expand on this is messy, so > > just unify the requirement to require grabbing an active > > reference and keep the superblock locked. > > > > Suggested-by: Christoph Hellwig <hch@infradead.org> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > Maybe I'm just getting old, but where did I suggest this? https://lore.kernel.org/all/20210420120335.GA3604224@infradead.org/ "I don't think we need both variants, just move the locking and s_active acquisition out of free_super. Same for the thaw side." > That being said, holding an active reference over any operation is a > good thing. As Jan said it can be done way simpler than this, though. Great. Luis
diff --git a/block/bdev.c b/block/bdev.c index 21c63bfef323..dc54a2a1c46e 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -251,7 +251,7 @@ int freeze_bdev(struct block_device *bdev) error = sb->s_op->freeze_super(sb); else error = freeze_super(sb); - deactivate_super(sb); + deactivate_locked_super(sb); if (error) { bdev->bd_fsfreeze_count--; @@ -289,6 +289,8 @@ int thaw_bdev(struct block_device *bdev) sb = bdev->bd_fsfreeze_sb; if (!sb) goto out; + if (!get_active_super(bdev)) + goto out; if (sb->s_op->thaw_super) error = sb->s_op->thaw_super(sb); @@ -298,6 +300,7 @@ int thaw_bdev(struct block_device *bdev) bdev->bd_fsfreeze_count++; else bdev->bd_fsfreeze_sb = NULL; + deactivate_locked_super(sb); out: mutex_unlock(&bdev->bd_fsfreeze_mutex); return error; diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 61c5f9d26018..e31d6791d3e3 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -2166,7 +2166,10 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count) if (err) return err; + if (!get_active_super(sbi->sb->s_bdev)) + return -ENOTTY; freeze_super(sbi->sb); + f2fs_down_write(&sbi->gc_lock); f2fs_down_write(&sbi->cp_global_sem); @@ -2217,6 +2220,8 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count) out_err: f2fs_up_write(&sbi->cp_global_sem); f2fs_up_write(&sbi->gc_lock); + /* We use the same active reference from freeze */ thaw_super(sbi->sb); + deactivate_locked_super(sbi->sb); return err; } diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c index 5eed8c237500..e57cb593e2f3 100644 --- a/fs/gfs2/super.c +++ b/fs/gfs2/super.c @@ -676,7 +676,12 @@ void gfs2_freeze_func(struct work_struct *work) struct gfs2_sbd *sdp = container_of(work, struct gfs2_sbd, sd_freeze_work); struct super_block *sb = sdp->sd_vfs; - atomic_inc(&sb->s_active); + if (!get_active_super(sb->s_bdev)) { + fs_info(sdp, "GFS2: couldn't grap super for thaw for filesystem\n"); + gfs2_assert_withdraw(sdp, 0); + return; + } + error = gfs2_freeze_lock(sdp, &freeze_gh, 0); if (error) { gfs2_assert_withdraw(sdp, 0); @@ -690,7 +695,7 @@ void gfs2_freeze_func(struct work_struct *work) } gfs2_freeze_unlock(&freeze_gh); } - deactivate_super(sb); + deactivate_locked_super(sb); clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags); wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN); return; diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c index 454dc2ff8b5e..cbb71c3520c0 100644 --- a/fs/gfs2/sys.c +++ b/fs/gfs2/sys.c @@ -164,6 +164,9 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len) if (!capable(CAP_SYS_ADMIN)) return -EPERM; + if (!get_active_super(sb->s_bdev)) + return -ENOTTY; + switch (n) { case 0: error = thaw_super(sdp->sd_vfs); @@ -172,9 +175,12 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len) error = freeze_super(sdp->sd_vfs); break; default: + deactivate_locked_super(sb); return -EINVAL; } + deactivate_locked_super(sb); + if (error) { fs_warn(sdp, "freeze %d error %d\n", n, error); return error; diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c index 7a6aeffcdf5c..3a0cd5e9ad84 100644 --- a/fs/gfs2/util.c +++ b/fs/gfs2/util.c @@ -345,10 +345,15 @@ int gfs2_withdraw(struct gfs2_sbd *sdp) set_bit(SDF_WITHDRAW_IN_PROG, &sdp->sd_flags); if (sdp->sd_args.ar_errors == GFS2_ERRORS_WITHDRAW) { + if (!get_active_super(sb->s_bdev)) { + fs_err(sdp, "could not grab super on withdraw for file system\n"); + return -1; + } fs_err(sdp, "about to withdraw this file system\n"); BUG_ON(sdp->sd_args.ar_debug); signal_our_withdraw(sdp); + deactivate_locked_super(sb); kobject_uevent(&sdp->sd_kobj, KOBJ_OFFLINE); diff --git a/fs/ioctl.c b/fs/ioctl.c index 5b2481cd4750..1d20af762e0d 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -386,6 +386,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, static int ioctl_fsfreeze(struct file *filp) { struct super_block *sb = file_inode(filp)->i_sb; + int ret; if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) return -EPERM; @@ -394,10 +395,17 @@ static int ioctl_fsfreeze(struct file *filp) if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL) return -EOPNOTSUPP; + if (!get_active_super(sb->s_bdev)) + return -ENOTTY; + /* Freeze */ if (sb->s_op->freeze_super) - return sb->s_op->freeze_super(sb); - return freeze_super(sb); + ret = sb->s_op->freeze_super(sb); + ret = freeze_super(sb); + + deactivate_locked_super(sb); + + return ret; } static int ioctl_fsthaw(struct file *filp) diff --git a/fs/super.c b/fs/super.c index 34afe411cf2b..0e9d48846684 100644 --- a/fs/super.c +++ b/fs/super.c @@ -39,8 +39,6 @@ #include <uapi/linux/mount.h> #include "internal.h" -static int thaw_super_locked(struct super_block *sb); - static LIST_HEAD(super_blocks); static DEFINE_SPINLOCK(sb_lock); @@ -851,13 +849,13 @@ struct super_block *get_active_super(struct block_device *bdev) if (sb->s_bdev == bdev) { if (!grab_super(sb)) goto restart; - up_write(&sb->s_umount); return sb; } } spin_unlock(&sb_lock); return NULL; } +EXPORT_SYMBOL_GPL(get_active_super); struct super_block *user_get_super(dev_t dev, bool excl) { @@ -1024,13 +1022,13 @@ void emergency_remount(void) static void do_thaw_all_callback(struct super_block *sb) { - down_write(&sb->s_umount); + if (!get_active_super(sb->s_bdev)) + return; if (sb->s_root && sb->s_flags & SB_BORN) { emergency_thaw_bdev(sb); - thaw_super_locked(sb); - } else { - up_write(&sb->s_umount); + thaw_super(sb); } + deactivate_locked_super(sb); } static void do_thaw_all(struct work_struct *work) @@ -1636,10 +1634,13 @@ static void sb_freeze_unlock(struct super_block *sb, int level) } /** - * freeze_super - lock the filesystem and force it into a consistent state + * freeze_super - force a filesystem backed by a block device into a consistent state * @sb: the super to lock * - * Syncs the super to make sure the filesystem is consistent and calls the fs's + * Used by filesystems and the kernel to freeze a fileystem backed by a block + * device into a consistent state. Callers must use get_active_super(bdev) to + * lock the @sb and when done must unlock it with deactivate_locked_super(). + * Syncs the filesystem backed by the @sb and calls the filesystem's optional * freeze_fs. Subsequent calls to this without first thawing the fs will return * -EBUSY. * @@ -1672,22 +1673,15 @@ int freeze_super(struct super_block *sb) { int ret; - atomic_inc(&sb->s_active); - down_write(&sb->s_umount); - if (sb->s_writers.frozen != SB_UNFROZEN) { - deactivate_locked_super(sb); + if (sb->s_writers.frozen != SB_UNFROZEN) return -EBUSY; - } - if (!(sb->s_flags & SB_BORN)) { - up_write(&sb->s_umount); + if (!(sb->s_flags & SB_BORN)) return 0; /* sic - it's "nothing to do" */ - } if (sb_rdonly(sb)) { /* Nothing to do really... */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; - up_write(&sb->s_umount); return 0; } @@ -1707,7 +1701,6 @@ int freeze_super(struct super_block *sb) sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT); wake_up(&sb->s_writers.wait_unfrozen); - deactivate_locked_super(sb); return ret; } @@ -1723,7 +1716,6 @@ int freeze_super(struct super_block *sb) sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb, SB_FREEZE_FS); wake_up(&sb->s_writers.wait_unfrozen); - deactivate_locked_super(sb); return ret; } } @@ -1733,19 +1725,25 @@ int freeze_super(struct super_block *sb) */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; lockdep_sb_freeze_release(sb); - up_write(&sb->s_umount); return 0; } EXPORT_SYMBOL(freeze_super); -static int thaw_super_locked(struct super_block *sb) +/** + * thaw_super -- unlock a filesystem backed by a block device + * @sb: the super to thaw + * + * Used by filesystems and the kernel to thaw a fileystem backed by a block + * device. Callers must use get_active_super(bdev) to lock the @sb and when + * done must unlock it with deactivate_locked_super(). Once done, this marks + * the filesystem as writeable. + */ +int thaw_super(struct super_block *sb) { int error; - if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) { - up_write(&sb->s_umount); + if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) return -EINVAL; - } if (sb_rdonly(sb)) { sb->s_writers.frozen = SB_UNFROZEN; @@ -1760,7 +1758,6 @@ static int thaw_super_locked(struct super_block *sb) printk(KERN_ERR "VFS:Filesystem thaw failed\n"); lockdep_sb_freeze_release(sb); - up_write(&sb->s_umount); return error; } } @@ -1769,21 +1766,8 @@ static int thaw_super_locked(struct super_block *sb) sb_freeze_unlock(sb, SB_FREEZE_FS); out: wake_up(&sb->s_writers.wait_unfrozen); - deactivate_locked_super(sb); return 0; } - -/** - * thaw_super -- unlock filesystem - * @sb: the super to thaw - * - * Unlocks the filesystem and marks it writeable again after freeze_super(). - */ -int thaw_super(struct super_block *sb) -{ - down_write(&sb->s_umount); - return thaw_super_locked(sb); -} EXPORT_SYMBOL(thaw_super); /*
Right now freeze_super() and thaw_super() are called with different locking contexts. To expand on this is messy, so just unify the requirement to require grabbing an active reference and keep the superblock locked. Suggested-by: Christoph Hellwig <hch@infradead.org> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- block/bdev.c | 5 +++- fs/f2fs/gc.c | 5 ++++ fs/gfs2/super.c | 9 +++++-- fs/gfs2/sys.c | 6 +++++ fs/gfs2/util.c | 5 ++++ fs/ioctl.c | 12 ++++++++-- fs/super.c | 62 ++++++++++++++++++------------------------------- 7 files changed, 60 insertions(+), 44 deletions(-)