Re: [PATCH 4/5] fs: don't sleep on s_umount of a superblock being shut down

[Jan Kara]

On Wed 21-10-26 00:56:38, Josef Bacik wrote:
> A slow unmount holds up tasks which have no business with that filesystem.
> 
> deactivate_locked_super() drops the last active reference with s_umount
> held for writing.  generic_shutdown_super() then writes the filesystem
> out, evicts its inodes and calls ->put_super(), and only after all that
> sets SB_DYING and drops s_umount.  On a slow or unresponsive device this
> takes minutes, or forever.
> 
> super_lock() doesn't take s_umount if it sees SB_DYING, but for the whole
> of the above it isn't set yet.  So whoever walks the superblocks sleeps
> uninterruptibly on s_umount until the unmount is over, then finds SB_DYING
> set and is told that there is nothing here for them.  One stuck unmount
> this way takes along, among others, every
> 
>  - sync(2)
>  - write to /proc/sys/vm/drop_caches
>  - quotactl(Q_SYNC) without a device
>  - quotactl() and ustat() on that device
>  - BLKFLSBUF on that device
> 
> The outcome of that sleep is known when it starts.  s_active reaches zero
> only under s_umount, s_umount is then held until SB_DYING is set, and
> nobody gets a new active reference once it is zero.  Check s_active first
> and fail right away.
> 
> super_lock() and what is built on it are local to fs/super.c, so its users
> can be listed.
> 
> Those who need the unmount to be over wait for that on their own since the
> previous three patches:
> 
>  - sync(2) and BLKFLSBUF, which can now be killed while they wait
>  - the emergency sync and remount, from a workqueue
>  - fs_bdev_freeze(), fs_bdev_mark_dead() for an orderly removal and
>    filesystems_freeze(), for which nothing changes
> 
> sget_fc() waits for SB_DEAD in grab_super().
> 
> freeze_super(), thaw_super() and the second super_lock_excl() in
> do_thaw_all_callback() are called with an active reference held and never
> see such a superblock.
> 
> That leaves drop_caches, quotactl(Q_SYNC) and delayed_superblock_init() in
> selinux through iterate_supers(), cifs through iterate_supers_type(),
> quotactl() and ustat() through user_get_super(), fs_bdev_mark_dead() for a
> device which is already gone, and thawing: fs_bdev_thaw(),
> filesystems_thaw() and the emergency thaw.  They have nothing to do on a
> superblock which is going away and promise nothing about it.  They carry
> on with the next one as they always did, only sooner.
> 
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 9baaaa3256b2..9f76b399f3fb 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -104,16 +104,29 @@ static bool super_flags(const struct super_block *sb, unsigned int 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.
> + *
>   * 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 SB_DYING was
> - *         set and without s_umount held.
> + *         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)
>  {
>  	lockdep_assert_not_held(&sb->s_umount);
>  
> +	/*
> +	 * The last active reference is dropped with s_umount held, which
> +	 * stays held until SB_DYING is set, and there is no getting a new
> +	 * one.  All there is to find at the end of that wait is SB_DYING,
> +	 * and it can be a long one: the filesystem is written out first.
> +	 */
> +	if (!atomic_read(&sb->s_active))
> +		return false;
> +
>  	/* wait until the superblock is ready or dying */
>  	wait_var_event(&sb->s_flags, super_flags(sb, SB_BORN | SB_DYING));
>  
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR