Re: [PATCH 4/5] super: make sync(2)'s wait for s_umount killable

[Jan Kara]

On Fri 23-10-26 11:41:04, Christian Brauner wrote:
> iterate_supers_sync() waits killably for a superblock that's being shut
> down, but it takes s_umount of a live superblock uninterruptibly. So a
> freeze_super() or remount that's stuck under s_umount, in
> sync_filesystem() or ->freeze_fs() say, still hangs every sync(2) on
> the machine without anyone being able to kill it.
> 
> That's inconsistent with fs_bdev_sync() which already uses a killable
> super_lock() for both waits. And the reasoning is the same: if the task
> is killed the walk stops and ksys_sync() returns, there's nobody left to
> make a promise to. The emergency sync and the sync before suspend and
> hibernation run from a workqueue where that can't happen. The PROM sync
> on sparc32 runs in whatever task it interrupted but that one could
> already be cut short while it waited for a superblock being shut down.
> 
> Let __iterate_supers() pass SUPER_LOCK_KILLABLE together with
> SUPER_LOCK_WAIT_SHUTDOWN for SUPER_ITER_WAIT_SHUTDOWN. Only
> iterate_supers_sync() passes that flag.

This might get surprising in the future, I guess SUPER_ITER_KILLABLE flag
might be a more explicit way to control this but I can live with what we
have now.

> The wait for SB_BORN doesn't
> matter for sync(2) as iterate_supers_sync() skips unborn superblocks
> anyway.
> 
> This can be reproduced by freezing a filesystem on a suspended dm-linear
> device and calling sync(2) behind the freeze:
> 
>   truncate -s 512M img
>   DEV=$(losetup -f --show img)
>   mke2fs -q $DEV
>   dmsetup create t --table "0 $(blockdev --getsz $DEV) linear $DEV 0"
>   mount -t ext2 /dev/mapper/t /mnt
>   dd if=/dev/urandom of=/mnt/f bs=1M count=64
>   dmsetup suspend --nolockfs t
>   fsfreeze --freeze /mnt &
>   sleep 3
>   sync &
>   sleep 3
>   kill -9 $!
> 
> fsfreeze sits in sync_filesystem() with s_umount held for writing.
> Without this patch the sync stays in down_read() after the SIGKILL. With
> it the sync goes away.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 0f3308fb99aa..a0f9eadf0da7 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1010,9 +1010,13 @@ static inline struct super_block *next_super(struct super_block *sb,
>  static int __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
>  			    enum super_iter_flags_t flags)
>  {
> +	enum super_lock_flags_t lock_flags = 0;
>  	struct super_block *sb, *p = NULL;
>  	int ret = 0;
>  
> +	if (flags & SUPER_ITER_WAIT_SHUTDOWN)
> +		lock_flags |= SUPER_LOCK_KILLABLE | SUPER_LOCK_WAIT_SHUTDOWN;
> +
>  	spin_lock(&sb_lock);
>  
>  	for (sb = first_super(flags);
> @@ -1032,12 +1036,14 @@ static int __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
>  
>  		if (flags & SUPER_ITER_UNLOCKED) {
>  			f(sb, arg);
> -		} else if (!super_lock(sb, 0)) {
> -			f(sb, arg);
> -			super_unlock_shared(sb);
> -		} else if (flags & SUPER_ITER_WAIT_SHUTDOWN) {
> -			/* whoever shuts it down writes it out, wait for that */
> -			ret = super_wait(sb, SB_DYING, TASK_KILLABLE);
> +		} else {
> +			ret = super_lock(sb, lock_flags);
> +			if (!ret) {
> +				f(sb, arg);
> +				super_unlock_shared(sb);
> +			} else if (ret == -ENOENT) {
> +				ret = 0;
> +			}
>  		}
>  
>  		if (p)
> @@ -1085,8 +1091,8 @@ void iterate_supers_born(void (*f)(struct super_block *, void *), void *arg)
>   * A superblock which is still being set up isn't passed to @f either, and
>   * isn't waited for.  Nobody can have written to it yet.
>   *
> - * Return: 0 if all the superblocks were seen, -ERESTARTSYS if the task was
> - *         killed while it waited for one.
> + * Return: 0 if all the superblocks were seen, -EINTR or -ERESTARTSYS if the
> + *         task was killed while it waited for one.
>   */
>  int iterate_supers_sync(void (*f)(struct super_block *, void *), void *arg)
>  {
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR