Re: [PATCH v2 01/10] fs: massage locking helpers

[Christoph Hellwig <hch@lst.de>]

On Tue, Oct 24, 2023 at 03:01:07PM +0200, Christian Brauner wrote:
> Multiple people have balked at the the fact that
> super_lock{_shared,_excluse}() return booleans and even if they return
> false hold s_umount. So let's change them to only hold s_umount when
> true is returned and change the code accordingly.

With the fix suggested by Jan this looks good an a huge improvement:

Reviewed-by: Christoph Hellwig <hch@lst.de>

That being said, I find the {,__}super_{,un}lock{,_excl} helpers still
confusing as hell.

I'd prefer to remove the __super_lock and super_unlock and wrapping
the loking calls is just horrible confusing, and instead of just
looking at a trivial say:

	down_write(&sb->s_umount)

I now have to chase through two levels on indirections to figure out
what is going on, not helped by the boolean flag that is just
passed as true/false instead of clearly documenting what it does.

Below is what I'd do (on top of the whole series):

 - remove all wrappers
 - rename super_lock to lock_ready_super
 - add an enum telling if it locks exclusive or shared

I could probably live with 2 helpers taking/releasing s_umount
based on the same enum if we really want, but preferable only
for the case that actually handle both case.   But I think just
open coding them is easier to understand.

---
diff --git a/fs/super.c b/fs/super.c
index b26b302f870d24..b8ea32c7cd03e6 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -50,37 +50,6 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = {
 	"sb_internal",
 };
 
-static inline void __super_lock(struct super_block *sb, bool excl)
-{
-	if (excl)
-		down_write(&sb->s_umount);
-	else
-		down_read(&sb->s_umount);
-}
-
-static inline void super_unlock(struct super_block *sb, bool excl)
-{
-	if (excl)
-		up_write(&sb->s_umount);
-	else
-		up_read(&sb->s_umount);
-}
-
-static inline void __super_lock_excl(struct super_block *sb)
-{
-	__super_lock(sb, true);
-}
-
-static inline void super_unlock_excl(struct super_block *sb)
-{
-	super_unlock(sb, true);
-}
-
-static inline void super_unlock_shared(struct super_block *sb)
-{
-	super_unlock(sb, false);
-}
-
 static inline bool wait_born(struct super_block *sb)
 {
 	unsigned int flags;
@@ -93,10 +62,15 @@ static inline bool wait_born(struct super_block *sb)
 	return flags & (SB_BORN | SB_DYING);
 }
 
+enum lock_super_mode {
+	LOCK_SUPER_SHARED,
+	LOCK_SUPER_EXCL,
+};
+
 /**
- * super_lock - wait for superblock to become ready and lock it
+ * lock_ready_super - wait for superblock to become ready and lock it
  * @sb: superblock to wait for
- * @excl: whether exclusive access is required
+ * @mode: whether exclusive access is required
  *
  * If the superblock has neither passed through vfs_get_tree() or
  * generic_shutdown_super() yet wait for it to happen. Either superblock
@@ -109,29 +83,37 @@ static inline bool wait_born(struct super_block *sb)
  *         s_umount held. The function returns false if SB_DYING was
  *         set and without s_umount held.
  */
-static __must_check bool super_lock(struct super_block *sb, bool excl)
+static __must_check bool lock_ready_super(struct super_block *sb,
+		enum lock_super_mode mode)
 {
+	bool is_dying;
 
 	lockdep_assert_not_held(&sb->s_umount);
 
 relock:
-	__super_lock(sb, excl);
+	if (mode == LOCK_SUPER_EXCL)
+		down_write(&sb->s_umount);
+	else
+		down_read(&sb->s_umount);
+
+	/* Has called ->get_tree() successfully. */
+	if ((sb->s_flags & (SB_BORN | SB_DYING)) == SB_BORN)
+		return true;
 
 	/*
 	 * Has gone through generic_shutdown_super() in the meantime.
 	 * @sb->s_root is NULL and @sb->s_active is 0. No one needs to
 	 * grab a reference to this. Tell them so.
 	 */
-	if (sb->s_flags & SB_DYING) {
-		super_unlock(sb, excl);
-		return false;
-	}
+	is_dying = sb->s_flags & SB_DYING;
 
-	/* Has called ->get_tree() successfully. */
-	if (sb->s_flags & SB_BORN)
-		return true;
+	if (mode == LOCK_SUPER_EXCL)
+		up_write(&sb->s_umount);
+	else
+		up_read(&sb->s_umount);
 
-	super_unlock(sb, excl);
+	if (is_dying)
+		return false;
 
 	/* wait until the superblock is ready or dying */
 	wait_var_event(&sb->s_flags, wait_born(sb));
@@ -143,18 +125,6 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
 	goto relock;
 }
 
-/* wait and try to acquire read-side of @sb->s_umount */
-static inline bool super_lock_shared(struct super_block *sb)
-{
-	return super_lock(sb, false);
-}
-
-/* wait and try to acquire write-side of @sb->s_umount */
-static inline bool super_lock_excl(struct super_block *sb)
-{
-	return super_lock(sb, true);
-}
-
 /* wake waiters */
 #define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING | SB_DEAD)
 static void super_wake(struct super_block *sb, unsigned int flag)
@@ -163,7 +133,7 @@ static void super_wake(struct super_block *sb, unsigned int flag)
 	WARN_ON_ONCE(hweight32(flag & SUPER_WAKE_FLAGS) > 1);
 
 	/*
-	 * Pairs with smp_load_acquire() in super_lock() to make sure
+	 * Pairs with smp_load_acquire() in lock_ready_super() to make sure
 	 * all initializations in the superblock are seen by the user
 	 * seeing SB_BORN sent.
 	 */
@@ -237,7 +207,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 		freed += sb->s_op->free_cached_objects(sb, sc);
 	}
 
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 	return freed;
 }
 
@@ -303,7 +273,7 @@ static void destroy_unused_super(struct super_block *s)
 {
 	if (!s)
 		return;
-	super_unlock_excl(s);
+	up_write(&s->s_umount);
 	list_lru_destroy(&s->s_dentry_lru);
 	list_lru_destroy(&s->s_inode_lru);
 	security_sb_free(s);
@@ -496,7 +466,7 @@ void deactivate_locked_super(struct super_block *s)
 		put_filesystem(fs);
 		put_super(s);
 	} else {
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 	}
 }
 
@@ -513,7 +483,7 @@ EXPORT_SYMBOL(deactivate_locked_super);
 void deactivate_super(struct super_block *s)
 {
 	if (!atomic_add_unless(&s->s_active, -1, 1)) {
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		deactivate_locked_super(s);
 	}
 }
@@ -550,13 +520,13 @@ static bool grab_super(struct super_block *sb)
 
 	sb->s_count++;
 	spin_unlock(&sb_lock);
-	locked = super_lock_excl(sb);
+	locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 	if (locked) {
 		if (atomic_inc_not_zero(&sb->s_active)) {
 			put_super(sb);
 			return true;
 		}
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 	}
 	wait_var_event(&sb->s_flags, wait_dead(sb));
 	put_super(sb);
@@ -586,7 +556,7 @@ bool super_trylock_shared(struct super_block *sb)
 		if (!(sb->s_flags & SB_DYING) && sb->s_root &&
 		    (sb->s_flags & SB_BORN))
 			return true;
-		super_unlock_shared(sb);
+		up_read(&sb->s_umount);
 	}
 
 	return false;
@@ -611,13 +581,13 @@ bool super_trylock_shared(struct super_block *sb)
 void retire_super(struct super_block *sb)
 {
 	WARN_ON(!sb->s_bdev);
-	__super_lock_excl(sb);
+	down_write(&sb->s_umount);
 	if (sb->s_iflags & SB_I_PERSB_BDI) {
 		bdi_unregister(sb->s_bdi);
 		sb->s_iflags &= ~SB_I_PERSB_BDI;
 	}
 	sb->s_iflags |= SB_I_RETIRED;
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 }
 EXPORT_SYMBOL(retire_super);
 
@@ -699,7 +669,7 @@ void generic_shutdown_super(struct super_block *sb)
 	 * sget{_fc}() until we passed sb->kill_sb().
 	 */
 	super_wake(sb, SB_DYING);
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	if (sb->s_bdi != &noop_backing_dev_info) {
 		if (sb->s_iflags & SB_I_PERSB_BDI)
 			bdi_unregister(sb->s_bdi);
@@ -886,7 +856,7 @@ EXPORT_SYMBOL(sget);
 
 void drop_super(struct super_block *sb)
 {
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 	put_super(sb);
 }
 
@@ -894,7 +864,7 @@ EXPORT_SYMBOL(drop_super);
 
 void drop_super_exclusive(struct super_block *sb)
 {
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	put_super(sb);
 }
 EXPORT_SYMBOL(drop_super_exclusive);
@@ -941,11 +911,11 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		locked = super_lock_shared(sb);
+		locked = lock_ready_super(sb, LOCK_SUPER_SHARED);
 		if (locked) {
 			if (sb->s_root)
 				f(sb, arg);
-			super_unlock_shared(sb);
+			up_read(&sb->s_umount);
 		}
 
 		spin_lock(&sb_lock);
@@ -979,11 +949,11 @@ void iterate_supers_type(struct file_system_type *type,
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		locked = super_lock_shared(sb);
+		locked = lock_ready_super(sb, LOCK_SUPER_SHARED);
 		if (locked) {
 			if (sb->s_root)
 				f(sb, arg);
-			super_unlock_shared(sb);
+			up_read(&sb->s_umount);
 		}
 
 		spin_lock(&sb_lock);
@@ -1010,11 +980,14 @@ struct super_block *user_get_super(dev_t dev, bool excl)
 			sb->s_count++;
 			spin_unlock(&sb_lock);
 			/* still alive? */
-			locked = super_lock(sb, excl);
+			locked = lock_ready_super(sb, excl);
 			if (locked) {
 				if (sb->s_root)
 					return sb;
-				super_unlock(sb, excl);
+				if (excl)
+					up_write(&sb->s_umount);
+				else
+					up_read(&sb->s_umount);
 			}
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
@@ -1061,9 +1034,9 @@ int reconfigure_super(struct fs_context *fc)
 
 	if (remount_ro) {
 		if (!hlist_empty(&sb->s_pins)) {
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			group_pin_kill(&sb->s_pins);
-			__super_lock_excl(sb);
+			down_write(&sb->s_umount);
 			if (!sb->s_root)
 				return 0;
 			if (sb->s_writers.frozen != SB_UNFROZEN)
@@ -1126,7 +1099,7 @@ int reconfigure_super(struct fs_context *fc)
 
 static void do_emergency_remount_callback(struct super_block *sb)
 {
-	bool locked = super_lock_excl(sb);
+	bool locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 
 	if (locked && sb->s_root && sb->s_bdev && !sb_rdonly(sb)) {
 		struct fs_context *fc;
@@ -1140,7 +1113,7 @@ static void do_emergency_remount_callback(struct super_block *sb)
 		}
 	}
 	if (locked)
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 }
 
 static void do_emergency_remount(struct work_struct *work)
@@ -1163,7 +1136,7 @@ void emergency_remount(void)
 
 static void do_thaw_all_callback(struct super_block *sb)
 {
-	bool locked = super_lock_excl(sb);
+	bool locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 
 	if (locked && sb->s_root) {
 		if (IS_ENABLED(CONFIG_BLOCK))
@@ -1173,7 +1146,7 @@ static void do_thaw_all_callback(struct super_block *sb)
 		return;
 	}
 	if (locked)
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 }
 
 static void do_thaw_all(struct work_struct *work)
@@ -1394,7 +1367,7 @@ static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
 
 	mutex_unlock(&bdev->bd_holder_lock);
 
-	locked = super_lock(sb, excl);
+	locked = lock_ready_super(sb, excl);
 	put_super(sb);
 	if (!locked)
 		return NULL;
@@ -1418,7 +1391,7 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 		return NULL;
 
 	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
-		super_unlock_shared(sb);
+		up_read(&sb->s_umount);
 		return NULL;
 	}
 
@@ -1440,7 +1413,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 	if (sb->s_op->shutdown)
 		sb->s_op->shutdown(sb);
 
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 }
 
 static void fs_bdev_sync(struct block_device *bdev)
@@ -1451,7 +1424,7 @@ static void fs_bdev_sync(struct block_device *bdev)
 	if (!sb)
 		return;
 	sync_filesystem(sb);
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 }
 
 static struct super_block *get_bdev_super(struct block_device *bdev)
@@ -1462,7 +1435,7 @@ static struct super_block *get_bdev_super(struct block_device *bdev)
 	sb = bdev_super_lock(bdev, true);
 	if (sb) {
 		active = atomic_inc_not_zero(&sb->s_active);
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 	}
 	if (!active)
 		return NULL;
@@ -1619,9 +1592,9 @@ int get_tree_bdev(struct fs_context *fc,
 		 * bdev_mark_dead()). It is safe because we have active sb
 		 * reference and SB_BORN is not set yet.
 		 */
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 		error = setup_bdev_super(s, fc->sb_flags, fc);
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		if (!error)
 			error = fill_super(s, fc);
 		if (error) {
@@ -1671,9 +1644,9 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 		 * bdev_mark_dead()). It is safe because we have active sb
 		 * reference and SB_BORN is not set yet.
 		 */
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 		error = setup_bdev_super(s, flags, NULL);
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		if (!error)
 			error = fill_super(s, data, flags & SB_SILENT ? 1 : 0);
 		if (error) {
@@ -1990,7 +1963,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 {
 	int ret;
 
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
@@ -2010,7 +1983,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 		 * freeze and assign the active ref to the freeze.
 		 */
 		sb->s_writers.freeze_holders |= who;
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;
 	}
 
@@ -2025,7 +1998,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	}
 
 	if (!(sb->s_flags & SB_BORN)) {
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;	/* sic - it's "nothing to do" */
 	}
 
@@ -2034,15 +2007,15 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 		sb->s_writers.freeze_holders |= who;
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 		wake_up_var(&sb->s_writers.frozen);
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;
 	}
 
 	sb->s_writers.frozen = SB_FREEZE_WRITE;
 	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	sb_wait_write(sb, SB_FREEZE_WRITE);
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
@@ -2085,7 +2058,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	return 0;
 }
 EXPORT_SYMBOL(freeze_super);
@@ -2102,7 +2075,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
 		if (!(sb->s_writers.freeze_holders & who)) {
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			return -EINVAL;
 		}
 
@@ -2117,7 +2090,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 			return 0;
 		}
 	} else {
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return -EINVAL;
 	}
 
@@ -2135,7 +2108,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 		if (error) {
 			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			return error;
 		}
 	}
@@ -2163,7 +2136,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who)
 {
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while thawing!");
 		return -EINVAL;
 	}