Re: [PATCH 3/5] super: let super_lock() wait for a superblock being shut down

[Jan Kara]

On Fri 23-10-26 11:41:03, Christian Brauner wrote:
> Everyone who needs an unmount to be over before they can carry on
> currently calls super_lock() and, if that fails, calls
> super_wait_shutdown() or super_wait_shutdown_killable() after it. That's
> six callers open-coding the same two step dance, and they have to make
> sure to pick the shutdown wait that matches the lock they took.
> 
> Add SUPER_LOCK_WAIT_SHUTDOWN which makes super_lock() wait for SB_DYING
> when it finds the superblock going away. It still returns -ENOENT after
> the wait. It sleeps in the same state as the rest of super_lock(), so
> SUPER_LOCK_KILLABLE now covers all of the waiting. get_active_super()
> passes the flags through for the freezing callers. Then remove
> super_wait_shutdown() and super_wait_shutdown_killable().
> 
> __iterate_supers() is the odd one out. It takes s_umount uninterruptibly
> but waits for the shutdown killably. Keep it that way for now and let it
> call super_wait() directly.
> 
> Note that the freezing callers used to wait for the shutdown even if
> super_lock() succeeded and atomic_inc_not_zero() in get_active_super()
> failed. That can't happen. The last active reference is dropped with
> s_umount held and s_umount stays held until SB_DYING is set. So once
> super_lock() succeeded there's always an active reference to take.
> 
> No functional changes.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Nice. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 118 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 45 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 0185c1f16ba3..0f3308fb99aa 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -58,6 +58,7 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = {
>  enum super_lock_flags_t {
>  	SUPER_LOCK_EXCL		= (1U << 0),
>  	SUPER_LOCK_KILLABLE	= (1U << 1),
> +	SUPER_LOCK_WAIT_SHUTDOWN = (1U << 2),
>  };
>  
>  static inline int __super_lock(struct super_block *sb,
> @@ -130,13 +131,19 @@ static int super_wait(struct super_block *sb, unsigned int sb_flags,
>   * creation will succeed and SB_BORN is set by vfs_get_tree() or we're
>   * woken and we'll see SB_DYING.
>   *
> - * A superblock which is being shut down is not waited for, see
> - * super_wait_shutdown() and super_wait_shutdown_killable() for that.
> - *
>   * With SUPER_LOCK_EXCL s_umount is taken for writing, otherwise for reading.
> - * With SUPER_LOCK_KILLABLE both the wait and taking s_umount can be cut short
> - * by a fatal signal.  That is only for callers which give up altogether when
> - * they are killed.
> + *
> + * A superblock which is being shut down is not waited for unless
> + * SUPER_LOCK_WAIT_SHUTDOWN is passed.  That is for those who have nothing to
> + * do on a superblock which is going away, but who promise their caller that
> + * what it had to write has been written, or who must not carry on while the
> + * filesystem is still being written out.  The wait is over once
> + * generic_shutdown_super() is done with @sb, and -ENOENT is still returned.
> + *
> + * With SUPER_LOCK_KILLABLE all the waiting can be cut short by a fatal
> + * signal.  That is only for callers which give up altogether when they are
> + * killed.  If what comes after the wait happens either way, the wait must
> + * not be cut short.
>   *
>   * The caller must have acquired a temporary reference on @sb->s_passive.
>   *
> @@ -162,7 +169,7 @@ static __must_check int super_lock(struct super_block *sb,
>  	 * and it can be a long one: the filesystem is written out first.
>  	 */
>  	if (!atomic_read(&sb->s_active))
> -		return -ENOENT;
> +		goto dying;
>  
>  	/* wait until the superblock is ready or dying */
>  	ret = super_wait(sb, SB_BORN | SB_DYING, state);
> @@ -171,7 +178,7 @@ static __must_check int super_lock(struct super_block *sb,
>  
>  	/* Don't pointlessly acquire s_umount. */
>  	if (super_flags(sb, SB_DYING))
> -		return -ENOENT;
> +		goto dying;
>  
>  	ret = __super_lock(sb, flags);
>  	if (ret)
> @@ -184,45 +191,19 @@ static __must_check int super_lock(struct super_block *sb,
>  	 */
>  	if (sb->s_flags & SB_DYING) {
>  		super_unlock(sb, flags);
> -		return -ENOENT;
> +		goto dying;
>  	}
>  
>  	WARN_ON_ONCE(!(sb->s_flags & SB_BORN));
>  	return 0;
> -}
>  
> -/**
> - * super_wait_shutdown_killable - wait for a superblock being shut down
> - * @sb: superblock super_lock() failed on
> - *
> - * For those who have nothing to do on a superblock which is going away, but
> - * who promise their caller that what it had to write has been written.
> - *
> - * Only for callers which give up altogether when they are killed.  If what
> - * comes after the wait happens either way, the wait must not be cut short.
> - *
> - * The caller must have acquired a temporary reference on @sb->s_passive.
> - *
> - * Return: 0 once generic_shutdown_super() is done with @sb, -ERESTARTSYS if
> - *         the task was killed first.
> - */
> -static int super_wait_shutdown_killable(struct super_block *sb)
> -{
> -	return super_wait(sb, SB_DYING, TASK_KILLABLE);
> -}
> -
> -/**
> - * super_wait_shutdown - wait for a superblock being shut down
> - * @sb: superblock super_lock() failed on
> - *
> - * For callers of which the rest happens whether they are killed or not, and
> - * must not happen while the filesystem is still being written out.
> - *
> - * The caller must have acquired a temporary reference on @sb->s_passive.
> - */
> -static void super_wait_shutdown(struct super_block *sb)
> -{
> -	super_wait(sb, SB_DYING, TASK_UNINTERRUPTIBLE);
> +dying:
> +	if (flags & SUPER_LOCK_WAIT_SHUTDOWN) {
> +		ret = super_wait(sb, SB_DYING, state);
> +		if (ret)
> +			return ret;
> +	}
> +	return -ENOENT;
>  }
>  
>  /* wake waiters */
> @@ -1055,7 +1036,8 @@ static int __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
>  			f(sb, arg);
>  			super_unlock_shared(sb);
>  		} else if (flags & SUPER_ITER_WAIT_SHUTDOWN) {
> -			ret = super_wait_shutdown_killable(sb);
> +			/* whoever shuts it down writes it out, wait for that */
> +			ret = super_wait(sb, SB_DYING, TASK_KILLABLE);
>  		}
>  
>  		if (p)
> @@ -1285,11 +1267,9 @@ static void do_emergency_remount_callback(struct super_block *sb, void *unused)
>  {
>  	struct fs_context *fc = NULL;
>  
> -	if (super_lock(sb, SUPER_LOCK_EXCL)) {
> -		/* the unmount writes the filesystem out, wait for that */
> -		super_wait_shutdown(sb);
> +	/* the unmount writes the filesystem out, wait for that */
> +	if (super_lock(sb, SUPER_LOCK_EXCL | SUPER_LOCK_WAIT_SHUTDOWN))
>  		return;
> -	}
>  
>  	/* Make sure no reference put under s_umount is the last one. */
>  	atomic_inc(&sb->s_active);
> @@ -1329,11 +1309,12 @@ void emergency_remount(void)
>  	}
>  }
>  
> -static inline bool get_active_super(struct super_block *sb)
> +static inline bool get_active_super(struct super_block *sb,
> +				    enum super_lock_flags_t flags)
>  {
>  	bool active = false;
>  
> -	if (!super_lock(sb, SUPER_LOCK_EXCL)) {
> +	if (!super_lock(sb, SUPER_LOCK_EXCL | flags)) {
>  		active = atomic_inc_not_zero(&sb->s_active);
>  		super_unlock_excl(sb);
>  	}
> @@ -1342,7 +1323,7 @@ static inline bool get_active_super(struct super_block *sb)
>  
>  static void do_thaw_all_callback(struct super_block *sb, void *unused)
>  {
> -	if (!get_active_super(sb))
> +	if (!get_active_super(sb, 0))
>  		return;
>  
>  	/* fs_bdev_thaw() acquires s_umount so it must not be held here */
> @@ -1388,11 +1369,9 @@ static void filesystems_freeze_callback(struct super_block *sb, void *freeze_all
>  	if (!freeze_all_ptr && !(sb->s_type->fs_flags & FS_POWER_FREEZE))
>  		return;
>  
> -	if (!get_active_super(sb)) {
> -		/* an unmount from a workqueue isn't stopped by the freezer */
> -		super_wait_shutdown(sb);
> +	/* an unmount from a workqueue isn't stopped by the freezer */
> +	if (!get_active_super(sb, SUPER_LOCK_WAIT_SHUTDOWN))
>  		return;
> -	}
>  
>  	if (sb->s_op->freeze_super)
>  		sb->s_op->freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> @@ -1419,7 +1398,7 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
>  	if (!sb->s_op->freeze_fs && !sb->s_op->freeze_super)
>  		return;
>  
> -	if (!get_active_super(sb))
> +	if (!get_active_super(sb, 0))
>  		return;
>  
>  	if (sb->s_op->thaw_super)
> @@ -1660,12 +1639,9 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
>  	for_each_dev_super(sb, &cursor) {
> -		if (super_lock(sb, 0)) {
> -			/* don't take the device away from under an unmount */
> -			if (!surprise)
> -				super_wait_shutdown(sb);
> +		/* don't take the device away from under an unmount */
> +		if (super_lock(sb, surprise ? 0 : SUPER_LOCK_WAIT_SHUTDOWN))
>  			continue;
> -		}
>  		if (sb->s_root && (sb->s_flags & SB_ACTIVE)) {
>  			if (!sb->s_op->remove_bdev ||
>  			    sb->s_op->remove_bdev(sb, bdev)) {
> @@ -1691,20 +1667,18 @@ static void fs_bdev_sync(struct block_device *bdev)
>  	for_each_dev_super_killable(sb, &cursor) {
>  		int ret;
>  
> -		ret = super_lock(sb, SUPER_LOCK_KILLABLE);
> -		if (!ret) {
> -			if (sb->s_root && (sb->s_flags & SB_ACTIVE))
> -				sync_filesystem(sb);
> -			super_unlock_shared(sb);
> -			continue;
> -		}
> +		ret = super_lock(sb, SUPER_LOCK_KILLABLE |
> +				     SUPER_LOCK_WAIT_SHUTDOWN);
>  		if (ret == -ENOENT)
> -			ret = super_wait_shutdown_killable(sb);
> +			continue;
>  		if (ret) {
>  			super_dev_walk_end(&cursor);
>  			put_super(sb);
>  			return;
>  		}
> +		if (sb->s_root && (sb->s_flags & SB_ACTIVE))
> +			sync_filesystem(sb);
> +		super_unlock_shared(sb);
>  	}
>  }
>  
> @@ -1733,11 +1707,9 @@ static int fs_bdev_freeze(struct block_device *bdev)
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
>  	for_each_dev_super(sb, &cursor) {
> -		if (!get_active_super(sb)) {
> -			/* an unmount writes to the device until it is done */
> -			super_wait_shutdown(sb);
> +		/* an unmount writes to the device until it is done */
> +		if (!get_active_super(sb, SUPER_LOCK_WAIT_SHUTDOWN))
>  			continue;
> -		}
>  		err = fs_super_freeze(sb);
>  		if (err && !error)
>  			error = err;
> @@ -1791,7 +1763,7 @@ static int fs_bdev_thaw(struct block_device *bdev)
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
>  	for_each_dev_super(sb, &cursor) {
> -		if (!get_active_super(sb))
> +		if (!get_active_super(sb, 0))
>  			continue;
>  		err = fs_super_thaw(sb);
>  		if (err && !error)
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR