| Message ID | 20231024-vfs-super-freeze-v2-0-599c19f4faac@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement freeze and thaw as holder operations | expand |
On Tue, 24 Oct 2023 15:01:06 +0200, Christian Brauner wrote: > Hey Christoph, > Hey Jan, > Hey Darrick, > > This is v2 and based on vfs.super. I'm sending this out right now > because frankly, shortly before the merge window is the time when I have > time to do something. Otherwise, I would've waited a bit. > > [...] for v6.8 --- Applied to the vfs.super branch of the vfs/vfs.git tree. Patches in the vfs.super branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.super [01/10] fs: massage locking helpers https://git.kernel.org/vfs/vfs/c/eebe374c4824 [02/10] bdev: rename freeze and thaw helpers https://git.kernel.org/vfs/vfs/c/48ebe79e5e5f [03/10] bdev: surface the error from sync_blockdev() https://git.kernel.org/vfs/vfs/c/7cb1d5f9e92c [04/10] bdev: add freeze and thaw holder operations https://git.kernel.org/vfs/vfs/c/bd05ce12dd8d [05/10] bdev: implement freeze and thaw holder operations https://git.kernel.org/vfs/vfs/c/8246b9ef23c3 [06/10] fs: remove get_active_super() https://git.kernel.org/vfs/vfs/c/085b0e223302 [07/10] super: remove bd_fsfreeze_sb https://git.kernel.org/vfs/vfs/c/36d253481290 [08/10] fs: remove unused helper https://git.kernel.org/vfs/vfs/c/350e08366980 [09/10] porting: document block device freeze and thaw changes https://git.kernel.org/vfs/vfs/c/c3e5c6b60a75 [10/10] blkdev: comment fs_holder_ops https://git.kernel.org/vfs/vfs/c/15d2068af6f4
Btw, while reviewing this I also noticed that thaw_super_locked feels unreasonably convoluted. Maybe something like this would be a good addition for the branch? --- From f5cbee13dcca6b025c82b365042bc5fab7ac6642 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Fri, 27 Oct 2023 08:36:04 +0200 Subject: fs: streamline thaw_super_locked Add a new out_unlock label to share code that just releases s_umount and returns an error, and rename and reuse the out label that deactivates the sb for one more case. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/super.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/fs/super.c b/fs/super.c index b26b302f870d24..38381c4b76f09e 100644 --- a/fs/super.c +++ b/fs/super.c @@ -2098,34 +2098,28 @@ EXPORT_SYMBOL(freeze_super); */ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) { - int error; + int error = -EINVAL; - if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) { - if (!(sb->s_writers.freeze_holders & who)) { - super_unlock_excl(sb); - return -EINVAL; - } + if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) + goto out_unlock; + if (!(sb->s_writers.freeze_holders & who)) + goto out_unlock; - /* - * Freeze is shared with someone else. Release our hold and - * drop the active ref that freeze_super assigned to the - * freezer. - */ - if (sb->s_writers.freeze_holders & ~who) { - sb->s_writers.freeze_holders &= ~who; - deactivate_locked_super(sb); - return 0; - } - } else { - super_unlock_excl(sb); - return -EINVAL; + /* + * Freeze is shared with someone else. Release our hold and drop the + * active ref that freeze_super assigned to the freezer. + */ + error = 0; + if (sb->s_writers.freeze_holders & ~who) { + sb->s_writers.freeze_holders &= ~who; + goto out_deactivate; } if (sb_rdonly(sb)) { sb->s_writers.freeze_holders &= ~who; sb->s_writers.frozen = SB_UNFROZEN; wake_up_var(&sb->s_writers.frozen); - goto out; + goto out_deactivate; } lockdep_sb_freeze_acquire(sb); @@ -2135,8 +2129,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) if (error) { printk(KERN_ERR "VFS:Filesystem thaw failed\n"); lockdep_sb_freeze_release(sb); - super_unlock_excl(sb); - return error; + goto out_unlock; } } @@ -2144,9 +2137,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) sb->s_writers.frozen = SB_UNFROZEN; wake_up_var(&sb->s_writers.frozen); sb_freeze_unlock(sb, SB_FREEZE_FS); -out: +out_deactivate: deactivate_locked_super(sb); return 0; + +out_unlock: + super_unlock_excl(sb); + return error; } /**
On Fri 27-10-23 08:40:01, Christoph Hellwig wrote: > Btw, while reviewing this I also noticed that thaw_super_locked feels > unreasonably convoluted. Maybe something like this would be a good > addition for the branch? > > > --- > From f5cbee13dcca6b025c82b365042bc5fab7ac6642 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Fri, 27 Oct 2023 08:36:04 +0200 > Subject: fs: streamline thaw_super_locked > > Add a new out_unlock label to share code that just releases s_umount > and returns an error, and rename and reuse the out label that deactivates > the sb for one more case. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks like a nice simplification. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/super.c | 43 ++++++++++++++++++++----------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/fs/super.c b/fs/super.c > index b26b302f870d24..38381c4b76f09e 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -2098,34 +2098,28 @@ EXPORT_SYMBOL(freeze_super); > */ > static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) > { > - int error; > + int error = -EINVAL; > > - if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) { > - if (!(sb->s_writers.freeze_holders & who)) { > - super_unlock_excl(sb); > - return -EINVAL; > - } > + if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) > + goto out_unlock; > + if (!(sb->s_writers.freeze_holders & who)) > + goto out_unlock; > > - /* > - * Freeze is shared with someone else. Release our hold and > - * drop the active ref that freeze_super assigned to the > - * freezer. > - */ > - if (sb->s_writers.freeze_holders & ~who) { > - sb->s_writers.freeze_holders &= ~who; > - deactivate_locked_super(sb); > - return 0; > - } > - } else { > - super_unlock_excl(sb); > - return -EINVAL; > + /* > + * Freeze is shared with someone else. Release our hold and drop the > + * active ref that freeze_super assigned to the freezer. > + */ > + error = 0; > + if (sb->s_writers.freeze_holders & ~who) { > + sb->s_writers.freeze_holders &= ~who; > + goto out_deactivate; > } > > if (sb_rdonly(sb)) { > sb->s_writers.freeze_holders &= ~who; > sb->s_writers.frozen = SB_UNFROZEN; > wake_up_var(&sb->s_writers.frozen); > - goto out; > + goto out_deactivate; > } > > lockdep_sb_freeze_acquire(sb); > @@ -2135,8 +2129,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) > if (error) { > printk(KERN_ERR "VFS:Filesystem thaw failed\n"); > lockdep_sb_freeze_release(sb); > - super_unlock_excl(sb); > - return error; > + goto out_unlock; > } > } > > @@ -2144,9 +2137,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) > sb->s_writers.frozen = SB_UNFROZEN; > wake_up_var(&sb->s_writers.frozen); > sb_freeze_unlock(sb, SB_FREEZE_FS); > -out: > +out_deactivate: > deactivate_locked_super(sb); > return 0; > + > +out_unlock: > + super_unlock_excl(sb); > + return error; > } > > /** > -- > 2.39.2 >
On Fri, 27 Oct 2023 08:40:01 +0200, Christoph Hellwig wrote: > Add a new out_unlock label to share code that just releases s_umount > and returns an error, and rename and reuse the out label that deactivates > the sb for one more case. > > Applied to the vfs.super branch of the vfs/vfs.git tree. Patches in the vfs.super branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.super [1/1] fs: streamline thaw_super_locked https://git.kernel.org/vfs/vfs/c/6d0acff564f2
Hey Christoph, Hey Jan, Hey Darrick, This is v2 and based on vfs.super. I'm sending this out right now because frankly, shortly before the merge window is the time when I have time to do something. Otherwise, I would've waited a bit. This implements block device freezing and thawing as holder operations. This allows us to extend block device freezing to all devices associated with a superblock and not just the main device. It also allows us to remove get_active_super() and thus another function that scans the global list of superblocks. Freezing via additional block devices only works if the filesystem chooses to use @fs_holder_ops for these additional devices as well. That currently only includes ext4 and xfs. Earlier releases switched get_tree_bdev() and mount_bdev() to use @fs_holder_ops. The remaining nilfs2 open-coded version of mount_bdev() has been converted to rely on @fs_holder_ops as well. So block device freezing for the main block device will continue to work as before. There should be no regressions in functionality. The only special case is btrfs where block device freezing for the main block device never worked because sb->s_bdev isn't set. Block device freezing for btrfs can be fixed once they can switch to @fs_holder_ops but that can happen whenever they're ready. This survives (1) xfstests: check -g quick (2) xfstests: check -g freeze (3) blktests: check Thanks! Christian Signed-off-by: Christian Brauner <brauner@kernel.org> Changes in v2: - Call sync_blockdev() consistently under bd_holder_lock. - Return early with error in fs_bdev_freeze() if we can't get an active reference and don't call sync_blockdev(). - Keep bd_fsfreeze_mutex now that we don't hold bd_holder_lock over freeze and thaw anymore. - Link to v1: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-0-ecc36d9ab4d9@kernel.org --- --- base-commit: 79ac81458fb58e1bac836450d6c68da1da9911d9 change-id: 20230927-vfs-super-freeze-eff650f66b06