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@xxxxxx>
> 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@xxxxxx>
Looks like a nice simplification. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
