Re: [PATCH 1/5] fs: make sync(2) wait for superblocks being shut down explicitly

[Jan Kara]

On Wed 21-10-26 00:56:35, Josef Bacik wrote:
> sync(2) walks the superblocks with iterate_supers().  For one which is
> being unmounted, super_lock() sleeps on s_umount until
> generic_shutdown_super() is done with it, finds SB_DYING set and fails, so
> the superblock isn't passed to the callback.
> 
> sync(2) does get something out of that sleep.  The unmount has written the
> filesystem out by the time s_umount is dropped, so that filesystem is on
> disk like all the others when sync(2) returns.  The same goes for the
> emergency sync and the emergency remount, after which one is supposed to
> be able to reset the machine.
> 
> Nothing says so in the code, and super_lock() is about to stop sleeping on
> a superblock which is being shut down.  Add
> super_wait_shutdown_killable(), which waits for SB_DYING, and
> a flag for __iterate_supers() to call it for each superblock super_lock()
> fails on.  iterate_supers_sync() passes that flag, for the two syncs, and
> so does the emergency remount.
> 
> If the task is killed the walk stops and ksys_sync() returns.  There is
> nobody left to make a promise to.  The emergency sync and remount and the
> sync before suspend and hibernation run from a workqueue, where that can't
> happen.
> 
> 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/internal.h |  1 +
>  fs/super.c    | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/sync.c     | 15 ++++++++-------
>  3 files changed, 57 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/internal.h b/fs/internal.h
> index e833c7e6e14f..71ab0ae11941 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -137,6 +137,7 @@ extern int reconfigure_super(struct fs_context *);
>  extern bool super_trylock_shared(struct super_block *sb);
>  struct super_block *user_get_super(dev_t, bool excl);
>  void put_super(struct super_block *sb);
> +int iterate_supers_sync(void (*f)(struct super_block *, void *), void *arg);
>  void __init super_dev_init(void);
>  extern bool mount_capable(struct fs_context *);
>  
> diff --git a/fs/super.c b/fs/super.c
> index a1dc4de1b657..c33981d3f7c0 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -137,6 +137,26 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
>  	return true;
>  }
>  
> +/**
> + * super_wait_shutdown_killable - wait for a superblock being shut down
> + * @sb: superblock super_lock() failed on
> + *
> + * For those who have nothing to do on a superblock which is going away, but
> + * who promise their caller that what it had to write has been written.
> + *
> + * Only for callers which give up altogether when they are killed.  If what
> + * comes after the wait happens either way, the wait must not be cut short.
> + *
> + * The caller must have acquired a temporary reference on @sb->s_passive.
> + *
> + * Return: 0 once generic_shutdown_super() is done with @sb, -ERESTARTSYS if
> + *         the task was killed first.
> + */
> +static int super_wait_shutdown_killable(struct super_block *sb)
> +{
> +	return wait_var_event_killable(&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)
>  {
> @@ -938,6 +958,7 @@ enum super_iter_flags_t {
>  	SUPER_ITER_EXCL		= (1U << 0),
>  	SUPER_ITER_UNLOCKED	= (1U << 1),
>  	SUPER_ITER_REVERSE	= (1U << 2),
> +	SUPER_ITER_WAIT_SHUTDOWN = (1U << 3),
>  };
>  
>  static inline struct super_block *first_super(enum super_iter_flags_t flags)
> @@ -955,11 +976,12 @@ static inline struct super_block *next_super(struct super_block *sb,
>  	return list_next_entry(sb, s_list);
>  }
>  
> -static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
> -			     enum super_iter_flags_t flags)
> +static int __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
> +			    enum super_iter_flags_t flags)
>  {
>  	struct super_block *sb, *p = NULL;
>  	bool excl = flags & SUPER_ITER_EXCL;
> +	int ret = 0;
>  
>  	spin_lock(&sb_lock);
>  
> @@ -979,16 +1001,21 @@ static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
>  		} else if (super_lock(sb, excl)) {
>  			f(sb, arg);
>  			super_unlock(sb, excl);
> +		} else if (flags & SUPER_ITER_WAIT_SHUTDOWN) {
> +			ret = super_wait_shutdown_killable(sb);
>  		}
>  
>  		if (p)
>  			put_super(p);
>  		p = sb;
>  		spin_lock(&sb_lock);
> +		if (ret)
> +			break;
>  	}
>  	spin_unlock(&sb_lock);
>  	if (p)
>  		put_super(p);
> +	return ret;
>  }
>  
>  void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> @@ -996,6 +1023,23 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
>  	__iterate_supers(f, arg, 0);
>  }
>  
> +/**
> + * iterate_supers_sync - iterate_supers() for syncing all filesystems
> + * @f: function to call
> + * @arg: argument to pass to it
> + *
> + * A superblock which is being shut down isn't passed to @f, whoever shuts it
> + * down writes it out.  Wait for that, so that it is on disk as well by the time
> + * the walk is over.
> + *
> + * Return: 0 if all the superblocks were seen, -ERESTARTSYS if the task was
> + *         killed while it waited for one.
> + */
> +int iterate_supers_sync(void (*f)(struct super_block *, void *), void *arg)
> +{
> +	return __iterate_supers(f, arg, SUPER_ITER_WAIT_SHUTDOWN);
> +}
> +
>  /**
>   *	iterate_supers_type - call function for superblocks of given type
>   *	@type: fs type
> @@ -1174,7 +1218,8 @@ static void do_emergency_remount_callback(struct super_block *sb, void *unused)
>  static void do_emergency_remount(struct work_struct *work)
>  {
>  	__iterate_supers(do_emergency_remount_callback, NULL,
> -			 SUPER_ITER_EXCL | SUPER_ITER_REVERSE);
> +			 SUPER_ITER_EXCL | SUPER_ITER_REVERSE |
> +			 SUPER_ITER_WAIT_SHUTDOWN);
>  	kfree(work);
>  	printk("Emergency Remount complete\n");
>  }
> diff --git a/fs/sync.c b/fs/sync.c
> index 4a84dd837b86..0681bfcb2da6 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -99,9 +99,10 @@ void ksys_sync(void)
>  	int nowait = 0, wait = 1;
>  
>  	wakeup_flusher_threads(WB_REASON_SYNC);
> -	iterate_supers(sync_inodes_one_sb, NULL);
> -	iterate_supers(sync_fs_one_sb, &nowait);
> -	iterate_supers(sync_fs_one_sb, &wait);
> +	if (iterate_supers_sync(sync_inodes_one_sb, NULL) ||
> +	    iterate_supers_sync(sync_fs_one_sb, &nowait) ||
> +	    iterate_supers_sync(sync_fs_one_sb, &wait))
> +		return;
>  	sync_bdevs(false);
>  	sync_bdevs(true);
>  }
> @@ -121,11 +122,11 @@ static void do_sync_work(struct work_struct *work)
>  	 * Sync twice to reduce the possibility we skipped some inodes / pages
>  	 * because they were temporarily locked
>  	 */
> -	iterate_supers(sync_inodes_one_sb, NULL);
> -	iterate_supers(sync_fs_one_sb, &nowait);
> +	iterate_supers_sync(sync_inodes_one_sb, NULL);
> +	iterate_supers_sync(sync_fs_one_sb, &nowait);
>  	sync_bdevs(false);
> -	iterate_supers(sync_inodes_one_sb, NULL);
> -	iterate_supers(sync_fs_one_sb, &wait);
> +	iterate_supers_sync(sync_inodes_one_sb, NULL);
> +	iterate_supers_sync(sync_fs_one_sb, &wait);
>  	sync_bdevs(false);
>  	printk("Emergency Sync complete\n");
>  	kfree(work);
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR