Re: [PATCH 3/5] fs: make mount(2) remounts killable on s_umount

[Jan Kara]

On Fri 23-10-26 20:39:32, Josef Bacik wrote:
> mount(2) with MS_REMOUNT takes s_umount exclusively, with MS_BIND as well
> it takes it shared.  If the lock is held for a long time, by a syncfs(2) on
> a device which doesn't answer say, the remount sits in D state and can't be
> killed.
> 
> The one which wants the lock exclusively makes matters worse for everybody
> else: while it is queued, every new reader of s_umount queues up behind it.
> Killing it gets it out of the way, the rwsem wakes those behind a waiter
> which gives up.
> 
> Nothing has been changed yet when the lock is taken, and both can fail.
> Use the killable variants.
> 
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/namespace.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index e5feb4e48290..837f531e494b 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3485,7 +3485,9 @@ static int do_reconfigure_mnt(const struct path *path, unsigned int mnt_flags)
>  	 * We're only checking whether the superblock is read-only not
>  	 * changing it, so only take down_read(&sb->s_umount).
>  	 */
> -	down_read(&sb->s_umount);
> +	ret = down_read_killable(&sb->s_umount);
> +	if (ret)
> +		return ret;
>  	lock_mount_hash();
>  	ret = change_mount_ro_state(mnt, mnt_flags);
>  	if (ret == 0)
> @@ -3531,8 +3533,9 @@ static int do_remount(const struct path *path, int sb_flags,
>  	fc->oldapi = true;
>  
>  	err = parse_monolithic_mount_data(fc, data);
> +	if (!err)
> +		err = down_write_killable(&sb->s_umount);
>  	if (!err) {
> -		down_write(&sb->s_umount);
>  		err = -EPERM;
>  		if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {
>  			err = reconfigure_super(fc);
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR