Re: [PATCH v2 00/10] Implement freeze and thaw as holder operations

[Jan Kara <jack@suse.cz>]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR