Re: [PATCH 5/5] fs: let a mount waiting for the old superblock to die be killed

[Jan Kara]

On Wed 21-10-26 00:56:39, Josef Bacik wrote:
> When sget_fc() finds a superblock for what is being mounted and that
> superblock is being shut down, grab_super() waits for SB_DEAD and sget_fc()
> then looks again.  This one does have to wait: the old superblock owns the
> device until ->kill_sb() is done with it.
> 
> It doesn't have to be uninterruptible though.  If the unmount is stuck, so
> is every attempt to mount that device again, and none of them can be
> killed.  mount(2) and fsconfig(2) can fail at this point, nothing has been
> set up yet apart from a spare superblock which sget_fc() frees on its
> other ways out as well.
> 
> Make the wait killable.  grab_super() now has three outcomes, so have it
> return 0, -EAGAIN for "look again", or the error from the wait.
> 
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 9f76b399f3fb..36fdf88ef88e 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -673,12 +673,14 @@ EXPORT_SYMBOL(deactivate_super);
>   * superblock to either become SB_BORN or for it to pass through
>   * sb->kill() and be marked as SB_DEAD.
>   *
> - * Return: This returns true if an active reference could be acquired,
> - *         false if not.
> + * 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.
>   */
> -static bool grab_super(struct super_block *sb)
> +static int grab_super(struct super_block *sb)
>  {
>  	bool locked;
> +	int ret;
>  
>  	refcount_inc(&sb->s_passive);
>  	spin_unlock(&sb_lock);
> @@ -686,13 +688,13 @@ static bool grab_super(struct super_block *sb)
>  	if (locked) {
>  		if (atomic_inc_not_zero(&sb->s_active)) {
>  			put_super(sb);
> -			return true;
> +			return 0;
>  		}
>  		super_unlock_excl(sb);
>  	}
> -	wait_var_event(&sb->s_flags, super_flags(sb, SB_DEAD));
> +	ret = wait_var_event_killable(&sb->s_flags, super_flags(sb, SB_DEAD));
>  	put_super(sb);
> -	return false;
> +	return ret ? ret : -EAGAIN;
>  }
>  
>  /*
> @@ -960,9 +962,12 @@ struct super_block *sget_fc(struct fs_context *fc,
>  			warnfc(fc, "reusing existing filesystem in another namespace not allowed");
>  		return ERR_PTR(-EBUSY);
>  	}
> -	if (!grab_super(old))
> +	err = grab_super(old);
> +	if (err == -EAGAIN)
>  		goto retry;
>  	destroy_unused_super(s);
> +	if (err)
> +		return ERR_PTR(err);
>  	return old;
>  }
>  EXPORT_SYMBOL(sget_fc);
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR