Re: [PATCH v2 4/4] super: wait until we passed kill super

[Jan Kara <jack@suse.cz>]

On Fri 18-08-23 12:54:18, Christian Brauner wrote:
> Recent rework moved block device closing out of sb->put_super() and into
> sb->kill_sb() to avoid deadlocks as s_umount is held in put_super() and
> blkdev_put() can end up taking s_umount again.
> 
> That means we need to move the removal of the superblock from @fs_supers
> out of generic_shutdown_super() and into deactivate_locked_super() to
> ensure that concurrent mounters don't fail to open block devices that
> are still in use because blkdev_put() in sb->kill_sb() hasn't been
> called yet.
> 
> We can now do this as we can make iterators through @fs_super and
> @super_blocks wait without holding s_umount. Concurrent mounts will wait
> until a dying superblock is fully dead so until sb->kill_sb() has been
> called and SB_DEAD been set. Concurrent iterators can already discard
> any SB_DYING superblock.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

One nit below:

> +static inline bool wait_dead(struct super_block *sb)
> +{
> +	unsigned int flags;
> +
> +	/*
> +	 * Pairs with smp_store_release() in super_wake() and ensures
> +	 * that we see SB_DEAD after we're woken.
> +	 */
> +	flags = smp_load_acquire(&sb->s_flags);
> +	return flags & SB_DEAD;
> +}
> +
>  /**
>   * super_lock - wait for superblock to become ready
>   * @sb: superblock to wait for
> @@ -140,6 +152,33 @@ static bool super_lock(struct super_block *sb, bool excl)
>  	goto relock;
>  }
>  
> +/**
> + * super_lock_dead - wait for superblock to become ready or fully dead
> + * @sb: superblock to wait for
> + *
> + * Wait for a superblock to be SB_BORN or to be SB_DEAD. In other words,
> + * don't just wait for the superblock to be shutdown in
> + * generic_shutdown_super() but actually wait until sb->kill_sb() has
> + * finished.
> + *
> + * The caller must have acquired a temporary reference on @sb->s_count.
> + *
> + * Return: This returns true if SB_BORN was set, false if SB_DEAD was
> + *         set. The function acquires s_umount and returns with it held.
> + */
> +static bool super_lock_dead(struct super_block *sb)
> +{
> +	if (super_lock(sb, true))
> +		return true;
> +
> +	lockdep_assert_held(&sb->s_umount);
> +	super_unlock_excl(sb);
> +	/* If superblock is dying, wait for everything to be shutdown. */
> +	wait_var_event(&sb->s_flags, wait_dead(sb));
> +	__super_lock_excl(sb);
> +	return false;
> +}
> +
>  /* wait and acquire read-side of @sb->s_umount */
>  static inline bool super_lock_shared(struct super_block *sb)
>  {
> @@ -153,7 +192,7 @@ static inline bool super_lock_excl(struct super_block *sb)
>  }
>  
>  /* wake waiters */
> -#define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING)
> +#define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING | SB_DEAD)
>  static void super_wake(struct super_block *sb, unsigned int flag)
>  {
>  	unsigned int flags = sb->s_flags;
> @@ -169,6 +208,35 @@ static void super_wake(struct super_block *sb, unsigned int flag)
>  	wake_up_var(&sb->s_flags);
>  }
>  
> +/**
> + * grab_super_dead - acquire an active reference to a superblock
> + * @sb: superblock to acquire
> + *
> + * Acquire a temporary reference on a superblock and try to trade it for
> + * an active reference. This is used in sget{_fc}() to wait for a
> + * superblock to either become SB_BORN or for it to pass through
> + * sb->kill() and be marked as SB_DEAD.
> + *
> + * Return: This returns true if an active reference could be acquired,
> + *         false if not. The function acquires s_umount and returns with
> + *         it held.
> + */
> +static bool grab_super_dead(struct super_block *s) __releases(sb_lock)
> +{
> +	bool born;
> +
> +	s->s_count++;
> +	spin_unlock(&sb_lock);
> +	born = super_lock_dead(s);
> +	if (born && atomic_inc_not_zero(&s->s_active)) {
> +		put_super(s);
> +		return true;
> +	}
> +	up_write(&s->s_umount);
> +	put_super(s);
> +	return false;
> +}
> +

As I'm looking at it now, I'm wondering whether we are not overdoing it a
bit. Why not implement grab_super_dead() as:

static bool grab_super_dead(struct super_block *s) __releases(sb_lock)
{
	s->s_count++;
	if (grab_super(s))
		return true;
	wait_var_event(&sb->s_flags, wait_dead(sb));
	put_super(s);
	return false;
}

And just remove super_lock_dead() altogether? I don't expect more users of
that functionality...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR