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@xxxxxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
