Re: [PATCH 1/5] fs: make a mount waiting on another mount killable

[Jan Kara]

On Wed 21-10-26 16:46:57, Josef Bacik wrote:
> A superblock is put on the lists by sget_fc() and becomes usable when
> vfs_get_tree() sets SB_BORN, after the filesystem has read what it needs
> from its device or its server.  If that doesn't answer, this takes forever.
> 
> Somebody else mounting the same thing finds that superblock in sget_fc()
> and waits in grab_super() for it to be born, to share it, or to die, to
> make its own.  It has to.  But it does so in super_lock(), which can't be
> killed, so one stuck mount collects every later attempt.
> 
> mount(2) and fsconfig(2) can fail at this point, as they already do when
> they are killed while waiting for a superblock to die.
> 
> Add super_lock_killable(), in which both the wait for SB_BORN and taking
> s_umount can be killed, and use it in grab_super().  It tells a superblock
> which is going away from a wait which was cut short, so it returns an int:
> 0 with the lock held, -ENOENT, or the error from the wait.
> 
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

...

> +/**
> + * super_lock - wait for superblock to become ready and lock it
> + * @sb: superblock to wait for
> + * @excl: whether exclusive access is required
> + *
> + * If the superblock has neither passed through vfs_get_tree() or
> + * generic_shutdown_super() yet wait for it to happen. Either superblock
> + * 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.
> + *
> + * The caller must have acquired a temporary reference on @sb->s_passive.
> + *
> + * Return: The function returns true if SB_BORN was set and with
> + *         s_umount held. The function returns false if the superblock
> + *         is being shut down or SB_DYING was set and without s_umount
> + *         held.
> + */
> +static __must_check bool super_lock(struct super_block *sb, bool excl)
> +{
> +	return !super_lock_common(sb, excl, false);
> +}
> +
> +/**
> + * super_lock_killable - killable super_lock()
> + * @sb: superblock to wait for
> + * @excl: whether exclusive access is required
> + *
> + * Only for callers which give up altogether when they are killed.
> + *
> + * The caller must have acquired a temporary reference on @sb->s_passive.
> + *
> + * Return: 0 if SB_BORN was set and with s_umount held. Without s_umount held,
> + *         -ENOENT if the superblock is being shut down or SB_DYING was set,
> + *         -EINTR or -ERESTARTSYS if the task was killed.
> + */
> +static __must_check int super_lock_killable(struct super_block *sb, bool excl)
> +{
> +	return super_lock_common(sb, excl, true);
>  }

IMHO it is subtle that super_lock() and super_lock_killable() have
different convention for the return value (bool vs int). I'd just make both
return int and 0 on success. There aren't that many users so the change
won't even be that painful... Thanks.

								Honza
>  
>  /**
> @@ -674,25 +722,27 @@ EXPORT_SYMBOL(deactivate_super);
>   * sb->kill() and be marked as SB_DEAD.
>   *
>   * Return: 0 if an active reference could be acquired, -EAGAIN if the
> - *         superblock is gone and the caller should look again,
> - *         -ERESTARTSYS if the task was killed while waiting for that.
> + *         superblock is gone and the caller should look again, -EINTR or
> + *         -ERESTARTSYS if the task was killed while waiting for either.
>   */
>  static int grab_super(struct super_block *sb)
>  {
> -	bool locked;
>  	int ret;
>  
>  	refcount_inc(&sb->s_passive);
>  	spin_unlock(&sb_lock);
> -	locked = super_lock_excl(sb);
> -	if (locked) {
> +	ret = super_lock_killable(sb, true);
> +	if (!ret) {
>  		if (atomic_inc_not_zero(&sb->s_active)) {
>  			put_super(sb);
>  			return 0;
>  		}
>  		super_unlock_excl(sb);
> +		ret = -ENOENT;
>  	}
> -	ret = wait_var_event_killable(&sb->s_flags, super_flags(sb, SB_DEAD));
> +	if (ret == -ENOENT)
> +		ret = wait_var_event_killable(&sb->s_flags,
> +					      super_flags(sb, SB_DEAD));
>  	put_super(sb);
>  	return ret ? ret : -EAGAIN;
>  }
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR