Re: [PATCH 2/5] fs: make freezing for suspend wait for superblocks being shut down

[Jan Kara]

On Wed 21-10-26 00:56:36, Josef Bacik wrote:
> filesystems_freeze() freezes the filesystems ahead of suspend and
> hibernation, so that none of them changes on disk behind the back of the
> memory image.  A superblock which is being unmounted can't be frozen, but
> get_active_super() sleeps on s_umount until generic_shutdown_super() is
> done with it, so it has stopped writing by the time the walk moves on.
> 
> That matters.  The freezer doesn't take care of it: the final mntput() of
> a kernel thread is handed to delayed_mntput_work, and the workqueue it
> runs on is not freezable.  Were the walk to skip the superblock, the
> hibernation image could be taken in the middle of that unmount, which then
> carries on after the image is written, and again from the middle after
> resume.
> 
> Wait for SB_DYING explicitly, ahead of super_lock() not sleeping on such a
> superblock any more.  Suspend goes ahead whether this task is killed or
> not, so this gets a wait of its own, super_wait_shutdown(), which can't be
> killed, as it can't be today.
> 
> For now SB_DYING is always set by the time super_lock() fails, so nothing
> changes yet.
> 
> 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, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index c33981d3f7c0..76101307da01 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -157,6 +157,20 @@ static int super_wait_shutdown_killable(struct super_block *sb)
>  	return wait_var_event_killable(&sb->s_flags, super_flags(sb, SB_DYING));
>  }
>  
> +/**
> + * 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)
> +{
> +	wait_var_event(&sb->s_flags, super_flags(sb, SB_DYING));
> +}
> +
>  /* wait and try to acquire read-side of @sb->s_umount */
>  static inline bool super_lock_shared(struct super_block *sb)
>  {
> @@ -1294,8 +1308,11 @@ 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))
> +	if (!get_active_super(sb)) {
> +		/* an unmount from a workqueue isn't stopped by the freezer */
> +		super_wait_shutdown(sb);
>  		return;
> +	}
>  
>  	if (sb->s_op->freeze_super)
>  		sb->s_op->freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR