On Fri, Aug 18, 2023 at 02:26:09PM +0200, Jan Kara wrote:
> 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;
> }
Sounds good. Thanks for the suggestion.
>
> And just remove super_lock_dead() altogether? I don't expect more users of
> that functionality...
Famous last words... :)
